Fix multiple issues with SDL 2 window creation #251
Conversation
|
On Linux, it does correctly start the game on the primary monitor when the primary monitor has a lower resolution (so, the issue in #201). However, with either a single or multiple displays, changing the resolution in-game triggers an infinite loop of the screen going blank and returning to the desktop. (With my laptop set to only display on the external display, it just went into power saving mode when I changed resolution. Might be repeatedly changing resolutions and just confused that monitor.) |
|
@macsforme mentioned to me that the resolution change infinite loop on Linux is happening in the 2.4.20 release, so it seems that may be a regression there. I'll be checking into that tomorrow. |
|
I tested older BZFlag versions with SDL 2.0.9, back to 2.4.8, and they're also showing the infinite loop issue, so I guess it's not a recent regression, at least with our code. I will do some further testing. |
|
Trying to summarize the remaining issues that have been observed:
Additionally, we did mark #245 resolved after updating SDL seemed to fix the issue, but there was a comment from the original reporting user that there are still some issues with retina scaling. I do not know of any other complaints, and I do not have the needed hardware to test further. If someone wants to try running the game on macOS Catalina under several different retina scaling settings just so we can do our due diligence, I would consider that matter fully addressed for the time being. |
| setGamma(origGamma); | ||
|
|
||
| if (windowId != NULL) | ||
| SDL_DestroyWindow(windowId); |
There was a problem hiding this comment.
if you destroy the window, shouldn't you set it to null? (or better, nullptr)
src/platform/SDL2Window.cxx
Outdated
| const int maxRunawayFPS = 65; | ||
| int maxRunawayFPS = 65; | ||
| SDL_DisplayMode desktopDisplayMode; | ||
| if(SDL_GetDesktopDisplayMode(0, &desktopDisplayMode) == 0) |
There was a problem hiding this comment.
| if(SDL_GetDesktopDisplayMode(0, &desktopDisplayMode) == 0) | |
| if (SDL_GetDesktopDisplayMode(0, &desktopDisplayMode) == 0) |
There was a problem hiding this comment.
Agreed. I'll make a mental note to fix this when I do a final interactive rebase to fix and clean up a few things prior to merging.
| // For some reason, when creating a new fullscreen window on Linux with a different resolution than before, SDL throws | ||
| // a resize event with the old window resolution, which is not what we want. This function is called to filter SDL | ||
| // window resize events right after the resolution change and adjust the resolution to the correct one. | ||
| #ifdef __linux__ |
There was a problem hiding this comment.
does it need to be restricted to Linux?
There was a problem hiding this comment.
Not sure. In general, I would like to keep platform-specific workarounds within #ifdef guards, because we have a tendency to break other platforms while fixing an issue on one of them. Keeping the unknowns to a minimum is a plus in my opinion.
| // Depending on the distribution (or possibly the window manager), SDL will not recognize the new window resolution | ||
| // and will keep returning the previous resolution when SDL_GetWindowSize() is called, until a period of time has | ||
| // passed and events have been pumped. Wait up to two seconds for the correct resolution to start being returned, | ||
| // checking every quarter second, to avoid repeating the window destruction/re-creation process based on bad data. |
There was a problem hiding this comment.
I wonder if there is a way to flush the queue? This seems like an exceptionally awkward work-around. Or maybe we don't understand the resize event logic?
There was a problem hiding this comment.
Completely agreed that the workaround is awkward. SDL_PumpEvents() is flushing the queue in this case. After extensive testing, it appeared to me that both a time delay and pumping events are required in order to reach the point where the correct window resolution is returned. My best guess is that the window creation process in SDL returns before the modeset is complete. It took extensive testing to get to this point. However, if anyone else has a better solution, I'd be all for it.
|
The following additional issues have now been resolved:
Some of these fixes (specifically the F4/application switching fixes on Linux) required changes to logic outside of the platform module. Accordingly, this could use some extensive testing, especially related to launching in windowed versus fullscreen mode, using scaled versus native fullscreen resolutions, changing resolution, application switching, iconifying, window resizing, and various combinations/orders of the above, with various monitor counts and arrangements. I did hammer on macOS and Linux (Ubuntu) pretty hard, but Windows especially still needs to be checked. I also want to do one final interactive rebase to squash a few commits together and fix a few other things. Other than that, if no further issues are discovered, I think this is good to merge. |
|
I've finished my testing on Windows 7 (and a bit on 10), Linux (Xorg and Wayland), and macOS and I have found no regressions and it has fixed the majority of issues with our SDL2 code. I agree that this is ready to merge. |
I managed to borrow a 15" Retina MacBook Pro running Catalina. After building this PR, I tried every display scaling setting, and within those I tried launching the game with and without a resolution set in the configuration. Everything appeared to work fine regardless of the scaling setting. I did observe that the default resolution in the game would scale down with the more zoomed in scaling settings, and at the standard scaling setting the default resolution was half (so not high-DPI, but rather matching the "logical" resolution). To me, this is the expected and desired behavior, and users can always manually set a higher resolution if they want that. I will consider this concern resolved. @Ashvala, feel free to comment if you run into any more issues once this is merged. |
…ow and back to windowed mode, revert to the original windowed mode resolution. Fixes BZFlag-Dev#241.
…nfinite loop of incorrect window creation. Appears to fix BZFlag-Dev#201. Credit to @blast007 for the heavy lifting on this, and to @jose1711 for reporting the bug.
…structed on macOS, limit the framerate to around the actual monitor refresh rate rather than a constant value.
…window creation when attempting to change the fullscreen resolution.
…n the correct info right away when using a scaled resolution, resulting in SDLWindow::create() destroying and re-creating the window many times during the program startup.
…ginal desktop resolution is restored on Linux.
…other cases where no resize event is thrown.
…fluous calls to BzfWindow::resize() when the game itself initiated the resize. This should fix some issues on Linux that prevented iconifying or switching applications when running at a scaled fullscreen resolution.
…ux, and another issue where the window(ed) resolution would be lost by application switching or iconifying while using a scaled fullscreen resolution.
…solution when the game is started windowed. Fixed an issue where the custom fullscreen resolution would be the first time the game switches to fullscreen mode, but not in subsequent switches, when the game was started windowed.
macsforme
force-pushed
the
sdl2_window_fixes
branch
from
74268df to
0e5db0e
Compare
Issue #246 lists several bugs with our SDL 2 window creation process. While digging into several of those, I also discovered several other regressions (changes that broke other features, code that was transplanted but not entirely, etc.) introduced into our window creation process over several commits. This fixes several issues with our window creation process, including #201 (this is basically a rework of #238) for the multiple monitor Linux issue, #241 for the fullscreen toggle issue on macOS, and some other issues I discovered along the way.
I still haven't figured out #245 yet.