port to SDL3#7220
Conversation
|
This is the basic port, and other SDL3-related feature PRs will come after this is merged. One notable issue is that setting of gamma needs to be done by shaders as the gamma feature was removed from SDL3. Someone with a much better understanding of the current graphics code, and shaders in particular, should probably take care of that. |
wookieejedi
added
the
refactor
|
@BMagnu is the Gamma shader aspect mentioned above something that you would be willing to comment on? |
|
If I start in windowed mode, then switch to fullscreen mode the screen flickers. It contines to flicker if in fullscreen mode upon game restart as well. This does not happen via windowed mode, so it is likely that the SDL 3 PR inadvertently reverted a bug fix we have in master for this screen flickering |
I don't remember seeing this, but I rarely use fullscreen mode. Or it simply might not happen on Mac. I'll check it out when I get the chance. PR #6767 is still in there however, and wasn't altered for the port. |
|
Oh indeed, hmm yeah it's odd. At the very least it is always reproducible for me, so happy to do any local testing on Windows as we might need. |
notimaginative
force-pushed
the
sdl3_port
branch
2 times, most recently
from
3184bad to
37618ee
Compare
|
Squashed in fixes for the fullscreen bug as well as disabling the changing of display resolution at runtime. Both commits are available individually as part of #6735 for anyone that would like to see them without all the rest of the changes. |
notimaginative
force-pushed
the
sdl3_port
branch
from
37618ee to
de41772
Compare
Basic port from SDL2 to SDL3. Also updates imgui to 1.92.5.
| #endif | ||
| // This isn't reliable. Docs say to multiply by 160 for mobile | ||
| // and 96 for everything else. | ||
| return SDL_GetWindowDisplayScale(os::getSDLMainWindow()) * 96.0f; |
There was a problem hiding this comment.
Since ShivanSPS is working on mobile ports, we may need to mark or make an issue, so that he knows to work on this after this is merged.
There was a problem hiding this comment.
I may go ahead and add a change for this. Where if it's android or ios it will use 160 and otherwise use 96. That way @Shivansps doesn't have to worry about dealing with it.
There was a problem hiding this comment.
you probably know, but just in case, you can to use from anywhere in the code.
#ifdef __ANDROID__
There was a problem hiding this comment.
Added it using SDL_PLATFORM defines for simplicity, but with a comment to replace that with a much simpler check when we move to SDL 3.6+ since it offers a way to check specifically for phones. That way we aren't mistakenly using the incorrect DPI scale on non-mobile devices (like TVs or desktop Android).
| vals.push_back(i); | ||
| // a display id of zero is invalid in SDL3 but we need to support older | ||
| // builds so reduce by 1 and hope it works out | ||
| return json_pack("i", static_cast<int>(value)-1); |
There was a problem hiding this comment.
Are there any other options for this? Sounds risky
There was a problem hiding this comment.
In hindsight that comment wasn't appropriate. It's referring to new behavior in SDL3 where it doesn't reuse IDs during a session. For instance, if you have two monitors attached when FSO starts then SDL3 will assign them IDs of 1 and 2. If you disconnect the monitor with ID 1 and reconnect it while FSO is running then SDL3 assigns it a new ID of 3. So when FSO started it had two monitors with IDs of 1 and 2. Later those same two monitors have IDs of 2 and 3, where ID 3 refers to the monitor that used to be ID 1. If you update settings afterwards then the order of the monitors has now changed when FSO next loads.
So the "hope it works out" is referring to the likelihood that monitors would be connected, removed, or reconnected while FSO is running and hoping that it either doesn't ever happen or happens so infrequently that no one notices the issue. Not smart, true, but I considered the workarounds (like caching the device list for the duration of the game so it's always the same) to be properly dumb.
Basically IDs shouldn't be used to reference anything between sessions. It was that way in SDL2 as well, but IDs and indexes got used to reference devices because SDL2 maintained a fairly consistent list order (a hold-over from bad SDL1 design). SDL3 dropped that bad behavior.
The proper way to deal with it (even in SDL2, honestly) is to use device names as reference. That's something I'd like to address, but it means breaking compatibility with older builds, which isn't something I was prepared to do here. (That's better suited to it's own discussion, issue, and PR)
There was a problem hiding this comment.
Got it. Sounds like we need to create three issues as follow ups to this - Mobile DPI, Gamma, and Monitor Referencing. Thanks for looking through everything I mentioned!
There was a problem hiding this comment.
This issue here isn't specific to monitors though, it's all devices. Joysticks are another big one that required a bit of nuance, since they are referenced by index, but they're also referenced by GUID which allowed me to deal with it better (the joystick code now ignores the index and/or sets it to -1). Though I would like to change it to use both device name and GUID for joysticks at some point when/if we can agree to break config compatibility.
But gamma definitely does need a separate issue so that a graphics coder can tackle it.
There was a problem hiding this comment.
I'll go ahead make them.
JohnAFernandez
left a comment
JohnAFernandez
left a comment
There was a problem hiding this comment.
That's my stuff handled. Thanks for taking the time to handle it!
BMagnu
added
the
Waiting for Stable
5 participants
Basic port from SDL2 to SDL3. Also updates imgui to 1.92.5.