Doesn't the SDL source have regression tests in it?
Well, none that I know of. However, there is indeed a "test" directory in the SDL2 source repository, but as far as I can tell, these are merely meant to illustrate the use of various functionality provided by the SDL library, but not as automated (regression) tests (like my automated tape tests for the various R'n'D game engines).
If not, trying to add one is probably out of scope (they won't accept); but if it does, can you provide them with a regression test in suitable form (i.e. in the right directory structure, right buildable form, mentioned in necessary Makefiles or whatever) -- so it'll be caught next time?
Yes, that would be my "minimal code example" I've attached to the bug report (and that I could reference again this time without any changes to illustrate the bug). Running this test only requires extracting the tar/gz file and running "make" inside the extracted directory, and it still works without errors or warnings with the current SDL library, showing the broken transparency and, this time, even prints out an appropriate error message:
Code: Select all
holger@eriador:~/src/sdl_colorkey_problem$ make
gcc -g -o sdl_colorkey_problem sdl_colorkey_problem.c `sdl2-config --cflags` `sdl2-config --libs` -lSDL2_image
./sdl_colorkey_problem
::: GetColorKey() BEFORE SDL_ConvertSurface(): 0x00000000
SDL_GetColorKey() failed: Surface doesn't have a colorkey
make: *** [run] Error 10
holger@eriador:~/src/sdl_colorkey_problem$
When skipping the function to show the color key, the program runs "normally", but shows the missing transparency.
So, yes indeed, the test would be ready to use as part of a regression test suite (this time even with an automated result "OK" or "ERROR", which was not the case the last time the bug occurred).
But I'm afraid that making this suggestion does not help much, as there seems to be no such regression test suite. But I will do that suggestion in the bug tracker nevertheless!
Looking at the bug history, seems like they've had this at least 3x, maybe more.
Yes, this is indeed the third time that this bug occurred.
Plus, at the same time, add to RnD code which implements the same regression test at startup and pops up a dialog on startup:
As this bug is "detectable" this time (it wasn't the last time), I could indeed check for it and spit out a warning. I think this is indeed a good idea, as this version of the library will still be around for quite a while, unfortunately.
... meanwhile, you probably could fix it by shimming the PNG loader to convert color-key to alpha channel. Hmmm, in fact, it looks like you're already doing something similar in src/libgame/sdl.c:SDLLoadImage(), just need to 'upgrade' it a bit to defend against this class of bug?
Not sure if this is possible, but maybe yes (and this would indeed be a great solution, errm, nasty workaround for this situation). The problem is here (in src/libgame/sdl.c:SDLLoadImage(), just as you wrote):
Code: Select all
// set black pixel to transparent if no alpha channel / transparent color
if (!SDLHasAlpha(sdl_image_tmp) &&
!SDLHasColorKey(sdl_image_tmp))
SDL_SetColorKey(sdl_image_tmp, SET_TRANSPARENT_PIXEL,
SDL_MapRGB(sdl_image_tmp->format, 0x00, 0x00, 0x00));
There are the following three cases to achieve transparency in images loaded by R'n'D:
* alpha channel (as part of the image file)
* color key (color palette entry marked as transparent in the image file)
* nothing of the above, so we make all black pixels (#000000) transparent using "SDL_SetColorKey()"
So far I have no idea to internally convert affected images (well, that are at least all classic/default R'n'D image files) to alpha channel transparency (that's what SDL_SetColorKey() is used for), but I'll have a look.
And maybe change SDLSetWindowIcon() to call SDLLoadImage() so it's also protected...
Yes, good point.