KrampusHack 2020: Odd Results With Color Mapping Functions

no-reply@allegro.cc (bamccaig) — Mon, 28 Dec 2020 17:03:15 +0000

For my hack entry I decided to expand my wrapper library's asset manager with color management so that I could register a color by name and easily access it later on throughout the application (where it has access to assets).

Alas, I'm having a strange situation where the color being created does not appear to be initialized correctly. This has blocked me for a couple of days now. I'm not sure if the way I'm using Allegro is wrong, or if my type management is incorrect, or if this is somehow a bug in my build of Allegro.

Note: I'm casting between int and unsigned char, knowing the values should fit within an unsigned char, and assuming that the appropriate conversion will be applied by the C++ compiler. Even though I'm confident that this is fine, this feels like my most likely explanation for the flakiness if only because it's the only thing I can think of.

deps/al5poly/src/AssetManager.cpp#SelectExpand
  ALLEGRO_COLOR AssetManager::createColor(
          const std::string & name,
          unsigned char red,
          unsigned char green,
          unsigned char blue,
          unsigned char alpha)
  {
      ALLEGRO_COLOR value = al_map_rgba(red, green, blue, alpha);
 245
      fprintf(stderr, "LIBAL5POLY DEBUG: {\"r\": %d, \"g\": %d, \"b\": %d, \"a\": %d} <=> %s\n",
              (int)red, (int)green, (int)blue, (int)alpha, AssetManager::printColor(value));
 248
      this->colors_.insert(std::make_pair(name, value));
 250
      return value;
  }

deps/al5poly/src/AssetManager.cpp#SelectExpand
 312    const char * AssetManager::printColor(ALLEGRO_COLOR color)
 313    {
 314        static char buffer[50] = {0};
 315
 316        unsigned char red, green, blue, alpha;
 317
 318        al_unmap_rgba(color, &red, &green, &blue, &alpha);
 319
 320        snprintf(buffer, 50, "{\"r\": %d, \"g\": %d, \"b\": %d, \"a\": %d}",
 321                (int)red, (int)green, (int)blue, (int)alpha);
 322
 323        return buffer;
 324    }

Outputs things like this:

LIBAL5POLY DEBUG: {"r": 114, "g": 63, "b": 32, "a": 255} <=> {"r": 0, "g": 0, "b": 0, "a": 0}

Which makes it appear like the color object I just created is uninitialized/zeroed. And that appears to be correct too because when drawn the color is invisible regardless of background color (so the alpha=0 appears to be true at least). Any ideas what I'm doing wrong or ways to figure it out? Cookies for constructive posts that lead up to a solution.

(I'm banned from the IRC channel ATM so I can't ask there... )

no-reply@allegro.cc (torhu) — Mon, 28 Dec 2020 17:11:54 +0000

I think you need to initialize Allegro and create a display before you call those functions, are you doing that?

no-reply@allegro.cc (bamccaig) — Mon, 28 Dec 2020 18:22:37 +0000

Yes I am. This is only happening after Allegro has been initialized and a display created (and all errors should be handled).

#SelectExpand
  1void initializeAllegro5(
  2        al5poly::ALLEGRO_DISPLAY_Ptr & display,
  3        al5poly::ALLEGRO_TIMER_Ptr & timer,
  4        al5poly::ALLEGRO_EVENT_QUEUE_Ptr & eventQueue)
  5{
  6    const int FPS = 30;
  7
  8    if(!al_init())
  9        al5poly::Exception("Failed to initialize Allegro 5!").raise();
 10
 11    al_set_new_display_flags(ALLEGRO_WINDOWED);
 12
 13    al5poly::ALLEGRO_DISPLAY_Ptr d(
 14            al_create_display(SCREEN_W, SCREEN_H),
 15            al_destroy_display);
 16
 17    if(!d)
 18        al5poly::Exception("Failed to create Allegro 5 display!").raise();
 19
 20    display = d;
 21
 22    if(!al_install_keyboard())
 23        al5poly::Exception("Failed to install Allegro 5 keyboard!").raise();
 24
 25    if(!al_init_image_addon())
 26        al5poly::Exception("Failed to initialize image addon.").raise();
 27
 28    if(!al_init_primitives_addon())
 29        al5poly::Exception("Failed to initialize primitives addon.").raise();
 30
 31    al5poly::ALLEGRO_TIMER_Ptr t(
 32            al_create_timer(1.0 / FPS),
 33            al_destroy_timer);
 34
 35    if(!t)
 36        al5poly::Exception("Failed to create Allegro 5 timer!").raise();
 37
 38    timer = t;
 39
 40    al5poly::ALLEGRO_EVENT_QUEUE_Ptr eQ(
 41            al_create_event_queue(),
 42            al_destroy_event_queue);
 43
 44    if(!eQ)
 45        al5poly::Exception("Failed to create Allegro 5 event queue!").raise();
 46
 47    eventQueue = eQ;
 48
 49    al_register_event_source(
 50            eventQueue.get(),
 51            al_get_display_event_source(display.get()));
 52
 53    al_register_event_source(
 54            eventQueue.get(),
 55            al_get_keyboard_event_source());
 56
 57    al_register_event_source(
 58            eventQueue.get(),
 59            al_get_timer_event_source(timer.get()));
 60}

no-reply@allegro.cc (Elias) — Mon, 28 Dec 2020 23:12:48 +0000

Do you have a compileable and runnable testcase?

no-reply@allegro.cc (Edgar Reynaldo) — Tue, 29 Dec 2020 00:57:35 +0000

The only way al_map_rgb* fails is if al_init hasn't been run yet. Check again. Check for globals / singletons / etc....

EDIT
IF I have to give you the globals talk, I'm never gonna let you get over it.

EDIT2
static char buffers? Oh boy, you're gonna get it. NOT THREAD SAFE.

no-reply@allegro.cc (Peter Hull) — Tue, 29 Dec 2020 14:51:38 +0000

I'm pretty sure Edgar's got it:

Edgar Reynaldo said:

The only way al_map_rgb* fails is if al_init hasn't been run yet.

This looks most likely to me; if I make a test prog with your code and run it without al_init, I get exactly the (0,0,0,0) result that you do. Try printing the result of al_is_system_installed in your LIBAL5POLY DEBUG: print-out.
I bet you're calling AssetManager::createColor from a global or static initializer.

no-reply@allegro.cc (bamccaig) — Tue, 29 Dec 2020 15:06:07 +0000

Edgar Reynaldo said:

IF I have to give you the globals talk, I'm never gonna let you get over it.

State that exists globally (i.e,. throughout the lifetime of the application) is OK. State that is globally readable and writeable is not though. A closed static makes good sense here I think.

Edgar Reynaldo said:

static char buffers? Oh boy, you're gonna get it. NOT THREAD SAFE.

Good feedback. I appreciate that. What do I need to make that thread-safe? static thread_local char[50]? What's the "correct" way? Perhaps I could try to implement a library buffer that can be used for various things.

I attempted once to implement a ring buffer^[1] and apparently failed. I want to remedy that some day too. :(" /> So I guess I'd need at least two buffer implementations.

Elias said:

Do you have a compileable and runnable testcase?

https://github.com/bambams/al5poly-color-test

git clone --recursive git://github.com/bambams/al5poly-color-test.git && make -C al5poly-color-test run

Note: it references my library, which will contain the bulk of code, and obviously you wouldn't want to go through all of it... But I think it shouldn't be hard to track down where we call al_init(), and verify that it is all setup before we call al_map_rgba().

It's important to note that calling al_map_rgba() works fine. It's using my library's helper method that fails. There's no bug in Allegro. I just can't figure out the bug in my own code.

References

https://github.com/bambams/libbam/blob/master/src/bam_circbuf.c

no-reply@allegro.cc (Peter Hull) — Tue, 29 Dec 2020 20:55:39 +0000

    std::string AssetManager::printColor(ALLEGRO_COLOR color)
    {
        char buffer[50];

        unsigned char red, green, blue, alpha;

        al_unmap_rgba(color, &red, &green, &blue, &alpha);

        snprintf(buffer, 50, "{\"r\": %d, \"g\": %d, \"b\": %d, \"a\": %d}",
                (int)red, (int)green, (int)blue, (int)alpha);

        return std::string(buffer);
    }

would be thread-safe, obviously you'd have to change the places where you'd used it and c_str() or whatever.

BTW I tried your example code from github and everything seemed to work OK. So that is weird.

no-reply@allegro.cc (bamccaig) — Wed, 30 Dec 2020 22:27:43 +0000

Peter Hull said:

...would be thread-safe, obviously you'd have to change the places where you'd used it and c_str() or whatever.

Yes, that's an easy way to make it thread-safe, but then it has the added cost of allocating the string and copying the buffer into it... Which I was trying to avoid just because it should be relatively simple... But I guess C++ over-complicates matters. E.g., https://devblogs.microsoft.com/oldnewthing/20040308-00/?p=40363.

Apparently initialization of the static local is guaranteed thread-safe since C++0x, but that's all. It still sounds like adding thread_local to it will make it per-thread, which by definition should be thread safe, I guess? Which I guess is still a reasonable solution here instead of returning the copy, assuming it compiles OK.

Peter Hull said:

BTW I tried your example code from github and everything seemed to work OK. So that is weird.

Good to know. So perhaps I'm using Allegro properly, and there's a bug with the binaries? Did you test on MinGW/Windows or some other platform?

Edgar, fixit fixit fixit? Any ideas what I can do to narrow down the cause?

no-reply@allegro.cc (Edgar Reynaldo) — Thu, 31 Dec 2020 04:41:41 +0000

bamccaig said:

Edgar, fixit fixit fixit? Any ideas what I can do to narrow down the cause?

If your color manager is a singleton, and it's constructed outside of main, that would explain it. You may not believe it but you're calling al_map_rgb before al_init otherwise it would work.

no-reply@allegro.cc (bamccaig) — Thu, 31 Dec 2020 10:54:57 +0000

No, I have very minimal global state. The only reason the char buffer is global is because it's still local to that function, making its storage global, but it's access through the language is very narrow.

And apparently the code worked fine with Peter's environment. Which suggests that maybe the bug isn't in my code after all.

no-reply@allegro.cc (Edgar Reynaldo) — Fri, 01 Jan 2021 04:29:04 +0000

bambams - your example code looks fine. It doesn't work for you? Which binaries are you using (I have several versions), the ones from Bitbucket (whose upload is broken btw) or Github (Allegro 5.2.6x from GIT)?

no-reply@allegro.cc (bamccaig) — Sat, 02 Jan 2021 12:12:56 +0000

I can't remember where I downloaded them from.

6ba400e3ecbf3bc7f468a2f65ffc0f9d540233c9 *Allegro525_GCC81_MinGW_W64_i686_posix_dwarf.7z

Append:

Maybe from BitBucket?

no-reply@allegro.cc (Edgar Reynaldo) — Sat, 02 Jan 2021 19:28:41 +0000

Yes, those are the ones from bitbucket. I have a newer version on github released with eagle.

Get it here :

https://github.com/EdgarReynaldo/EagleGUI/releases

0pt8pt1 is a monolith download.

0pt8pt0 has a separate download for Allegro 526x that and its deps.

EDIT
Try this code :

#SelectExpand
  1#include "allegro5/allegro.h"
  2
  3class Color {
  4public :
  5   ALLEGRO_COLOR c;
  6   Color(int r , int g , int b , int a) : c(al_map_rgba(r,g,b,a) {}   
  7   void Print() {
  8      unsigned char r,g,b,a;
  9      al_unmap_rgba(c);
 10      printf("%c %c %c %c\n" , r , g , b , a);
 11   }
 12};
 13Color global(1,2,3,4);
 14int main(int argc , char** argv) {
 15   Color toosoon(5,6,7,8);
 16   if (!al_init()) {return -1;}
 17   Color okay(9,10,11,12);
 18   global.Print();
 19   toosoon.Print();
 20   okay.Print();
 21   return 0;
 22}

no-reply@allegro.cc (bamccaig) — Mon, 04 Jan 2021 12:12:10 +0000

The updated library appears to work now. I'm getting a correct brown circle. You might want to pull those 525 binaries down since there appears to be some kind of bug in them (and leave a notice redirecting to latest or something).

no-reply@allegro.cc (Edgar Reynaldo) — Mon, 04 Jan 2021 23:39:11 +0000

I tested the simple code I posted above with both 525 from bitbucket and 526x from github. Both work equally well. I don't think there is a bug in the binaries.

no-reply@allegro.cc (Peter Hull) — Tue, 05 Jan 2021 02:19:29 +0000

Your makefile is quite complicated, I wonder if maybe at some point it was rebuilding with a stale version of something or other? (vague, I know )

no-reply@allegro.cc (bamccaig) — Tue, 05 Jan 2021 03:33:45 +0000

Yeah, but it's pretty easy to see that I'm also initializing Allegro before I'm creating my color:

https://github.com/bambams/al5poly-color-test/blob/main/src/main.cpp#L26

Quote:

src/main.cpp#SelectExpand
 26int main(int argc, char ** argv) try 
 27{ 28    al5poly::ALLEGRO_DISPLAY_Ptr display; 29    al5poly::ALLEGRO_TIMER_Ptr timer; 30    al5poly::ALLEGRO_EVENT_QUEUE_Ptr eventQueue; 31 32    initializeAllegro5(display, timer, eventQueue); 

(Note: these *_Ptr types are just boost::shared_ptr<T> typedefs)

Quote:

src/main.cpp#SelectExpand
 125void initializeAllegro5( 
 126        al5poly::ALLEGRO_DISPLAY_Ptr & display,
 127        al5poly::ALLEGRO_TIMER_Ptr & timer,
 128        al5poly::ALLEGRO_EVENT_QUEUE_Ptr & eventQueue) 129{ 130constint FPS =30; 131 132    if(!al_init()) 
 133        al5poly::Exception("Failed to initialize Allegro 5!").raise();

Quote:

src/main.cpp#SelectExpand
  ALLEGRO_COLOR brown_level2 = assMan.createColor("squirrel-brown", 114, 63, 32); 
44while(true) 45{ 46ALLEGRO_EVENT event; 47bool tick =false; 48 49al_wait_for_event(eventQueue.get(), &event); 50 51if(event.type == ALLEGRO_EVENT_TIMER) 52{ 53            tick =true; 54} 55elseif(event.type == ALLEGRO_EVENT_KEY_DOWN) 56{ 57int keycode = event.keyboard.keycode; 58 59if(keycode == ALLEGRO_KEY_ESCAPE) 60{ 61break; 62} 63} 64elseif(event.type == ALLEGRO_EVENT_DISPLAY_CLOSE) 65{ 66break; 67} 68 69// Drawing. 70if(tick) 71{ 72try 73{ 74                ALLEGRO_COLOR brown_level1 = assMan.getColor("squirrel-brown"); 
              ALLEGRO_COLOR brown_level0 = al_map_rgb(114, 63, 32); 
77fprintf(stderr, "brown (level3): %s\n", assMan.printColor("squirrel-brown")); 78fprintf(stderr, "brown (level2): %s\n", assMan.printColor(brown_level2)); 79fprintf(stderr, "brown (level1): %s\n", assMan.printColor(brown_level1)); 80fprintf(stderr, "brown (level0): %s\n", assMan.printColor(brown_level0)); 81 82al_draw_filled_circle(200, 150, 100, al_map_rgb(255, 0, 0)); 83al_draw_filled_circle(200, 150, 100, brown_level0); 84al_draw_filled_circle(400, 150, 100, al_map_rgb(255, 0, 0)); 85al_draw_filled_circle(400, 150, 100, brown_level1); 86al_draw_filled_circle(200, 350, 100, al_map_rgb(255, 0, 0)); 87al_draw_filled_circle(200, 350, 100, brown_level2); 88 89                renderer.paint(al_map_rgb(255, 255, 255)); 90} 91catch(al5poly::IException & ex) 92{ 93                std::cerr <<"Unhandled exception in main loop drawing block: "<< ex.getMessage()<< std::endl; 94} 95} 96}

Quote:

#SelectExpand
 244void AssetManager::addColor(
          const std::string & name,
          ALLEGRO_COLOR color)
  {
      auto insertion = this->colors_.insert(std::make_pair(name, color));
 249
      if (!insertion.second)
      {
          al5poly::AssetManagerException(
                  std::string("Failed to add color by key ") +
                          name +
                          ". Duplicate exists?")
              .raise();
      }
  }
 259
  ALLEGRO_COLOR AssetManager::createColor(
          const std::string & name,
          unsigned char red,
          unsigned char green,
          unsigned char blue,
          unsigned char alpha)
  {
      ALLEGRO_COLOR value = al_map_rgba(red, green, blue, alpha);
 268
      //fprintf(stderr, "LIBAL5POLY DEBUG: {\"r\": %d, \"g\": %d, \"b\": %d, \"a\": %d} <=> %s\n",
      //        (int)red, (int)green, (int)blue, (int)alpha, AssetManager::printColor(value));
 271
      this->addColor(name, value);
 273
      return value;
  }
 276
  ALLEGRO_COLOR AssetManager::createColor(
          const std::string & name,
          int red,
          int green,
          int blue,
          int alpha)
  {
      return this->createColor(
              name,
              (unsigned char)red,
              (unsigned char)green,
              (unsigned char)blue);
  }
 290
  ALLEGRO_COLOR AssetManager::createColor(
          const std::string & name,
          float red,
          float green,
          float blue,
          float alpha)
  {
      ALLEGRO_COLOR value = al_map_rgba_f(red, green, blue, alpha);
 299
      this->colors_.insert(std::make_pair(name, value));
 301
      return value;
  }
 304
  ALLEGRO_COLOR AssetManager::getColor(
          const std::string & name,
          bool throwOnMismatch) const
  {
      ColorMap::const_iterator it = this->colors_.find(name);
 310
      if(it == this->colors_.end())
      {
          std::string msg = "Color asset not found: " + name;
 314
          if (throwOnMismatch)
          {
              AssetManagerException(msg).raise();
          }
          else
          {
              //fprintf(stderr, "%s. Falling back on AL5POLY_ERROR_COLOR. ;)\n", msg.c_str());
              return AL5POLY_ERROR_COLOR;
          }
      }
 325
      return (*it).second;
  }

I need to look more into this. It may well be that the build is fine, but I cannot yet see the bug in my code. The color test program appears to still be failing with the 526 binaries though.

no-reply@allegro.cc (Edgar Reynaldo) — Tue, 05 Jan 2021 07:55:41 +0000

Bambams it could be ABI related if you have old 5.2 dlls laying around on the path somewhere. Did you compile statically or dynamically?