Start getting base module ready for SDL3 #3171
Conversation
I think the fix for this should be easy enough to get done with in this PR. I'd like to see The callers can be fixed in future PRs on a submodule basis, just the function definition can be fixed now. And we can use our usual strategy of not changing the SDL2 codepath at all. |
|
Not changing the SDL2 codepath is all well and good, but I'd also like to avoid completely separate SDL2/SDL3 codepaths. I hadn't yet figured out whether to store the enum or the details in SDL3 or in SDL2, or how the consumer could be refitted in SDL2 to use enum instead of mask values. Earlier in the porting effort I moved almost everything over from mask style to enum style, this is one I didn't touch. My thought was that this PR contains simple enough code changes to go through without burdening the review of the pixelformat stuff (which could get complicated!) |
|
I'd like some solution that gets |
No longer exists in SDL3, so simple compat is to mock it out to a macro that is zero in SDL3.
SDL_getenv now returns a const char* instead of a char*, so rather than worry about propagating that through or casting it away, lets just have pg_EnvShouldBlendAlphaSDL2 return an int, and have the callers evaluate that instead of evaluating whether a char* is NULL or not. This requires no changes to the callers.
Starbuck5
force-pushed
the
start-base-sdl3
branch
2 times, most recently
from
e1cc0ec
to
de257d7
Compare
|
As requested I have made the module compile fully, and as predicted it completely blows up the scope of this PR. PR now changes the SDL2 codepath, includes changes to other modules, and the diff significantly larger. If I were reviewing this, I would want it in a separate PR. Here is an infographic to help explain the changes: |
| #define PG_ConvertSurfaceFormat SDL_ConvertSurfaceFormat | ||
| #define PG_ConvertSurfaceFormat SDL_ConvertSurface | ||
|
|
||
| #define PG_PixelFormatEnum SDL_PixelFormat |
There was a problem hiding this comment.
I like the addition of this macro. I believe defining another macro like
#if SDL_VERSION_ATLEAST(3, 0, 0)
#define PG_SURF_format(s) ((s)->format)
#else
#define PG_SURF_format(s) ((s)->format->format)
#endif
would also be useful
There was a problem hiding this comment.
I would like to wait a bit on that, see what abstractions evolve out of porting other modules for that case. My preliminary work has shown a big challenge in getting colorkey data and keeping it with the format, so maybe the bare format enum will be a rarity.
| if (pg_default_screen) { | ||
| return pg_default_screen->surf->format->format; | ||
| } | ||
| #endif |
There was a problem hiding this comment.
This whole block of code can be return PG_SURF_format(pg_default_screen->surf); after implementing the macro
| return RAISE(pgExc_SDLError, SDL_GetError()); | ||
| } | ||
| if (pg_GetDefaultConvertFormat() == 0) { | ||
| pg_SetDefaultConvertFormat(_surf->format->format); |
There was a problem hiding this comment.
| pg_SetDefaultConvertFormat(_surf->format->format); | |
| pg_SetDefaultConvertFormat(PG_SURF_format(_surf)); |
There was a problem hiding this comment.
I think this PR is ready, except that this minor issue has not been addressed yet. Adding the abstraction would fix this code for the SDL3 case, now it would error.
I suggested the abstraction because I can see it being used twice in this PR itself, so I assumed it could be potentially useful in the future.
There was a problem hiding this comment.
Well in my defense this PR is to port base, not window.
There was a problem hiding this comment.
Fair, we can deal with it later then. Anyways it is an issue that a compiler can catch, so it won't go unnoticed
There was a problem hiding this comment.
Yeah when you left your comment I stared at the line and was like how did the compiler not catch that. It took me a sec to realize the compiler isn't catching it because Window.c is not being compiled.
| break; | ||
| default: /* ARGB8888 */ | ||
|
|
||
| default: |
There was a problem hiding this comment.
I like the explanative diagram, with that I can see the equivalence of this code with the old code.
Now that we are moving to pixelformats here, we could probably further improve this logic to be simpler and more correct, by dealing with the SDL_PIXELORDER macro instead, though that would definitely be for a future PR, I think imma make an attempt at that after merging this one.
There was a problem hiding this comment.
I figured my comment was not clear, and I got the itch to work on it now. So what I'm essentially proposing is:
static SDL_Surface *
pg_DisplayFormatAlpha(SDL_Surface *surface)
{
PG_PixelFormatEnum pfe = SDL_PIXELFORMAT_ARGB8888;
PG_PixelFormatEnum dformat = pg_GetDefaultConvertFormat();
if (!dformat) {
SDL_SetError(
"No convert format has been set, try display.set_mode()"
" or Window.get_surface().");
return NULL;
}
if (SDL_ISPIXELFORMAT_ALPHA(dformat)) {
/* Display surface already has alpha, use the exact same format */
pfe = dformat;
}
else if (SDL_ISPIXELFORMAT_ARRAY(dformat)) {
/* In SDL2 this path is only triggered for
* SDL_PIXELFORMAT_RGB24/SDL_PIXELFORMAT_BGR24 */
switch (SDL_PIXELORDER(dformat)) {
#if SDL_BYTEORDER == SDL_BIG_ENDIAN
case SDL_ARRAYORDER_RGB:
pfe = SDL_PIXELFORMAT_ARGB8888;
break;
case SDL_ARRAYORDER_BGR:
pfe = SDL_PIXELFORMAT_BGRA8888;
break;
#else
case SDL_ARRAYORDER_RGB:
pfe = SDL_PIXELFORMAT_BGRA8888;
break;
case SDL_ARRAYORDER_BGR:
pfe = SDL_PIXELFORMAT_ARGB8888;
break;
#endif
default:
break;
}
}
else if (SDL_ISPIXELFORMAT_PACKED(dformat)) {
switch (SDL_PIXELORDER(dformat)) {
case SDL_PACKEDORDER_XRGB:
pfe = SDL_PIXELFORMAT_ARGB8888;
break;
case SDL_PACKEDORDER_RGBX:
pfe = SDL_PIXELFORMAT_RGBA8888;
break;
case SDL_PACKEDORDER_XBGR:
pfe = SDL_PIXELFORMAT_ABGR8888;
break;
case SDL_PACKEDORDER_BGRX:
pfe = SDL_PIXELFORMAT_BGRA8888;
break;
default:
break;
}
}
return PG_ConvertSurfaceFormat(surface, pfe);
}Still gonna PR it later so it can be independently reviewed
|
I consider this PR done, unless you insist on the macro Ankith. Left the last commit unsquashed so the recent changes are viewable on their own. I would squash the last couple commits before merging. |
This reworks the functions to work on pixel format enums instead of structs, because in SDL3 the enums are much easier to get than the structs. This required involved changes to surface.c, but I think the previous mask logic is much clearer now that it is expressed using format enums.
Starbuck5
force-pushed
the
start-base-sdl3
branch
from
42940ba
to
cc260b3
Compare
|
(Just squashed the last 3 commits together) |
Before this PR there are 3 issues preventing base from compiling on SDL3, this PR handles
2 of them (the easy ones).all 3.The remaining issue is the pixelformat changes interacting with get and set default pixelformat.Wanted to get this PR out so these two simpler changes can go through before worrying about what will need to happen with the pixelformat.