Allegro.cc - Online Community

Allegro.cc Forums » Programming Questions » KrampusHack 2020: Odd Results With Color Mapping Functions

This thread is locked; no one can reply to it. rss feed Print
KrampusHack 2020: Odd Results With Color Mapping Functions
bamccaig
Member #7,536
July 2006
avatar

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
237 ALLEGRO_COLOR AssetManager::createColor( 238 const std::string & name, 239 unsigned char red, 240 unsigned char green, 241 unsigned char blue, 242 unsigned char alpha) 243 { 244 ALLEGRO_COLOR value = al_map_rgba(red, green, blue, alpha); 245 246 fprintf(stderr, "LIBAL5POLY DEBUG: {\"r\": %d, \"g\": %d, \"b\": %d, \"a\": %d} <=> %s\n", 247 (int)red, (int)green, (int)blue, (int)alpha, AssetManager::printColor(value)); 248 249 this->colors_.insert(std::make_pair(name, value)); 250 251 return value; 252 }

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... :-X)

torhu
Member #2,727
September 2002
avatar

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

bamccaig
Member #7,536
July 2006
avatar

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}

Elias
Member #358
May 2000

Do you have a compileable and runnable testcase?

--
"Either help out or stop whining" - Evert

Edgar Reynaldo
Major Reynaldo
May 2007
avatar

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.

8-)

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

Peter Hull
Member #1,136
March 2001

I'm pretty sure Edgar's got it:

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.

bamccaig
Member #7,536
July 2006
avatar

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

8-)

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.

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]? :P 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. :-[

Peter Hull
Member #1,136
March 2001

    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.

bamccaig
Member #7,536
July 2006
avatar

...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. :P 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.

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? :D Any ideas what I can do to narrow down the cause?

Edgar Reynaldo
Major Reynaldo
May 2007
avatar

bamccaig said:

Edgar, fixit fixit fixit? :D 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.

bamccaig
Member #7,536
July 2006
avatar

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. :P

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

Edgar Reynaldo
Major Reynaldo
May 2007
avatar

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)?

bamccaig
Member #7,536
July 2006
avatar

I can't remember where I downloaded them from. :P

6ba400e3ecbf3bc7f468a2f65ffc0f9d540233c9 *Allegro525_GCC81_MinGW_W64_i686_posix_dwarf.7z

Append:

Maybe from BitBucket?

Edgar Reynaldo
Major Reynaldo
May 2007
avatar

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}

bamccaig
Member #7,536
July 2006
avatar

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).

Edgar Reynaldo
Major Reynaldo
May 2007
avatar

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.

Peter Hull
Member #1,136
March 2001

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 :) )

bamccaig
Member #7,536
July 2006
avatar

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

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)

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{ 130 const int FPS = 30; 131
132 if(!al_init())
133 al5poly::Exception("Failed to initialize Allegro 5!").raise();

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

#SelectExpand
244void AssetManager::addColor( 245 const std::string & name, 246 ALLEGRO_COLOR color) 247 { 248 auto insertion = this->colors_.insert(std::make_pair(name, color)); 249 250 if (!insertion.second) 251 { 252 al5poly::AssetManagerException( 253 std::string("Failed to add color by key ") + 254 name + 255 ". Duplicate exists?") 256 .raise(); 257 } 258 } 259 260 ALLEGRO_COLOR AssetManager::createColor( 261 const std::string & name, 262 unsigned char red, 263 unsigned char green, 264 unsigned char blue, 265 unsigned char alpha) 266 { 267 ALLEGRO_COLOR value = al_map_rgba(red, green, blue, alpha); 268 269 //fprintf(stderr, "LIBAL5POLY DEBUG: {\"r\": %d, \"g\": %d, \"b\": %d, \"a\": %d} <=> %s\n", 270 // (int)red, (int)green, (int)blue, (int)alpha, AssetManager::printColor(value)); 271 272 this->addColor(name, value); 273 274 return value; 275 } 276 277 ALLEGRO_COLOR AssetManager::createColor( 278 const std::string & name, 279 int red, 280 int green, 281 int blue, 282 int alpha) 283 { 284 return this->createColor( 285 name, 286 (unsigned char)red, 287 (unsigned char)green, 288 (unsigned char)blue); 289 } 290 291 ALLEGRO_COLOR AssetManager::createColor( 292 const std::string & name, 293 float red, 294 float green, 295 float blue, 296 float alpha) 297 { 298 ALLEGRO_COLOR value = al_map_rgba_f(red, green, blue, alpha); 299 300 this->colors_.insert(std::make_pair(name, value)); 301 302 return value; 303 } 304 305 ALLEGRO_COLOR AssetManager::getColor( 306 const std::string & name, 307 bool throwOnMismatch) const 308 { 309 ColorMap::const_iterator it = this->colors_.find(name); 310 311 if(it == this->colors_.end()) 312 { 313 std::string msg = "Color asset not found: " + name; 314 315 if (throwOnMismatch) 316 { 317 AssetManagerException(msg).raise(); 318 } 319 else 320 { 321 //fprintf(stderr, "%s. Falling back on AL5POLY_ERROR_COLOR. ;)\n", msg.c_str()); 322 return AL5POLY_ERROR_COLOR; 323 } 324 } 325 326 return (*it).second; 327 }

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.

Edgar Reynaldo
Major Reynaldo
May 2007
avatar

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?

Go to: