Image module SDL3 support #3166
Conversation
| @@ -138,7 +138,7 @@ PG_UnlockMutex(SDL_mutex *mutex) | |||
|
|
|||
| #define PG_CreateSurface(width, height, format) \ | |||
| SDL_CreateRGBSurfaceWithFormat(0, width, height, 0, format) | |||
| #define PG_CreateSurfaceFrom(pixels, width, height, pitch, format) \ | |||
| #define PG_CreateSurfaceFrom(width, height, format, pixels, pitch) \ | |||
There was a problem hiding this comment.
I'm not sure to understand what is happening here. Why the arguments are in a whole new order ?
There was a problem hiding this comment.
SDL3 changed the order of SDL_CreateSurfaceFrom, so this macro is being updated to match that order
There was a problem hiding this comment.
But then it's not the macro argument order that needs to be changed, but the function arg order that's called.
There was a problem hiding this comment.
This line is only compiled on SDL2, it defines a macro that re-orders the arguments from SDL3 style into the SDL2 function.
| #else | ||
| if (SDL_SetSurfacePalette(linebuf, surf_palette) < 0) | ||
| #endif | ||
| goto error; /* SDL error already set. */ |
There was a problem hiding this comment.
This seems like the only somewhat concerning part of the PR to me. Why is the SDL2 codepath being changed to use different API in this case. Or rather, why does the current code use SDL_SetPaletteColors instead of SDL_SetSurfacePalette, could there be a reason for it?
There was a problem hiding this comment.
Yeah so my thought there is that setting a surface's palette is more simple than setting every color of a surface's palette to another surface's palette.
I know codepath changes make these harder to review so sorry about that.
In terms of reason why, I bet it's because SDL1's set palette does it color by color, it was probably ported from this. https://www.libsdl.org/release/SDL-1.2.15/docs/html/sdlsetpalette.html
#if IS_SDLv1
SDL_SetColors (linebuf, surface->format->palette->colors, 0,
surface->format->palette->ncolors);
#else /* IS_SDLv2 */
SDL_SetPaletteColors (linebuf->format->palette,
surface->format->palette->colors,
0, surface->format->palette->ncolors);Ported from https://www.libsdl.org/release/SDL-1.2.15/docs/html/sdlsetcolors.html
There was a problem hiding this comment.
I added a test for TGA saving to hope to reassure you (and me!) that this works the same way, and now the tests are segfaulting on two of the Linux builds. I'm unclear if it's because of my changes or whether it would've segfaulted on the old implementation too.
There was a problem hiding this comment.
The TGA test and the fix for that segfault went through in #3169
| @@ -138,7 +138,7 @@ PG_UnlockMutex(SDL_mutex *mutex) | |||
|
|
|||
| #define PG_CreateSurface(width, height, format) \ | |||
| SDL_CreateRGBSurfaceWithFormat(0, width, height, 0, format) | |||
| #define PG_CreateSurfaceFrom(pixels, width, height, pitch, format) \ | |||
| #define PG_CreateSurfaceFrom(width, height, format, pixels, pitch) \ | |||
There was a problem hiding this comment.
SDL3 changed the order of SDL_CreateSurfaceFrom, so this macro is being updated to match that order
- Rework argument order of PG_CreateSurfaceFrom to match newest SDL3 (they changed it) - Deal with all the pixelformat fallout throughout the file - Fix TGA saving routine file writes - Switch out SDL_SetPaletteColors (all colors) for SDL_SetSurfacePalette
If desired, I could split this out into a couple smaller PRs.