Surface module compiling in SDL3 #3435
Conversation
Starbuck5
force-pushed
the
surface-sdl3-comp
branch
2 times, most recently
from
2025f7e to
a756521
Compare
Starbuck5
force-pushed
the
surface-sdl3-comp
branch
from
a756521 to
c44f364
Compare
Starbuck5
changed the title
| #if SDL_VERSION_ATLEAST(3, 0, 0) | ||
| if (SDL_SurfaceHasRLE(surf) && (!(flags & PGS_RLEACCEL))) | ||
| #else | ||
| if ((surf->flags & SDL_RLEACCEL) && (!(flags & PGS_RLEACCEL))) |
There was a problem hiding this comment.
SDL_SurfaceHasRLE basically checks if RLE was requested on a surface (and in this context should always be equivalent to (flags & PGS_RLEACCEL) because of the lines above this) which would basically make this whole block a if (false) thing that never runs.
surf->flags & SDL_RLEACCEL OTOH checks if the surface actually got RLE.
The problem here is that SDL doesn't expose the equivalent of (surf->flags & SDL_RLEACCEL) anymore. This is something that bites us with sdl2-compat as well (ref: libsdl-org/sdl2-compat#476) and as we have unit tests that fail because of this, we can tell that they don't have the same behavior.
As for this specific block, I think a more appropriate thing that reduces confusion would be to simply ifdef out the whole block in SDL3 and add a comment explaining that there's no way to do the check anymore (and its probably not even required in SDL3)
There was a problem hiding this comment.
I'm unaware of any changes to RLE internals in SDL3, so if this block isn't necessary in SDL3 is it necessary in SDL2?
There was a problem hiding this comment.
Added a print statement inside this block and ran the tests, it got into that path once. Found out that we have one test case that depends on this behaviour: test_solarwolf_rle_usage_2. So seems like this logic is necessary actually.
There was a problem hiding this comment.
I think that using SDL_MUSTLOCK (which is equivalent to (((S)->flags & SDL_RLEACCEL) != 0) on SDL2) does the same job on SDL3 as well, as an implementation detail
NVM SDL_MUSTLOCK is also now equivalent to SDL_SurfaceHasRLE apparently in SDL3
This PR consists of 4 commits, and it gets the surface module to compile with SDL3. It still fails lots of tests when run in SDL3 mode, but there is lots of SDL-function porting to be done with int->bool stuff.
Commit 1 focuses on Surface.c
This commit does not change the SDL2 codepath at all, except for a cliprect thing similar to what I've done in other PRs.
The RLEaccel stuff is confusing, even digging through the SDL2 source it's hard to understand. Our code uses
surf->flags & SDL_RLEACCELto check, interchangeably withSDL_SurfaceHasRLE(surf), but if you check the source SDL_SurfaceHasRLE does not check the same flags. However, in testing it seems to be equivalent, somehow.I also just declined to port a whole section that covers a blitting edge case, we'll get to it later, for now I think it will be valuable to have people able to compile at all.
Commit 2 and 3 focus on filling and blitting respectively
There are a lot of lines changed but it's just passing formats around, nothing complex
Commit 4 makes intrinsics compile properly
I took Ankith's prior work on this and moved it up to _surface.h, as well as making sure it defines SSE2 and AVX2 itself if necessary. On my (Windows) system it doesn't compile at least the SSE2 stuff without this, so in SDL2 we must be getting that define from SDL itself.