SDL2: Only create the window once #238
Conversation
|
On macOS, these work:
These do not work:
|
There was a problem hiding this comment.
Feel free to ignore any/all of my comments; they are generally educational rather than prescriptive.
I'm not familiar enough with SDL or the graphics code in general to have an opinion whether this is a good, necessary, or sufficient change, but it doesn't look evil to me.
| window->makeCurrent(); | ||
| if (!window->create()) | ||
| faulting = true; | ||
| window->getSize(trueWidth, trueHeight); |
There was a problem hiding this comment.
it's not at all clear what this change does. Some comments would be awesome
| if (windowId != NULL) | ||
| // If fullscreen, the target size will be that of the selected fullscreen resolution | ||
| if (fullScreen) | ||
| const_cast<SDLDisplay *>((const SDLDisplay *)getDisplay())->getWindowSize(targetWidth, targetHeight); |
There was a problem hiding this comment.
| const_cast<SDLDisplay *>((const SDLDisplay *)getDisplay())->getWindowSize(targetWidth, targetHeight); | |
| const_cast<SDLDisplay *>(static_cast<const SDLDisplay *>(getDisplay()))->getWindowSize(targetWidth, targetHeight); |
it's odd to mix C-style (bad) and C++-style (less bad) casts in a single statement.
And getWindowSize isn't a const method? Why not?
|
|
||
| bool SDLWindow::create(void) | ||
| { | ||
| int targetWidth, targetHeight; |
There was a problem hiding this comment.
From a complexity standpoint, uninitialized variables are bad. If this block were refactored so that these variables were initialized to the base dimensions, and overridden if fullScreen is true (if case below, lose the else), then you wouldn't have uninitialized variables dangling.
There was a problem hiding this comment.
You want to set these two variables and then immediately set them a second time every time this function is called for a fullscreen window?
There was a problem hiding this comment.
int targetWidth;declares an uninitialized variable.
int targetWidth = base_width;declares an initialized variable.
We aren't talking wasting CPU cycles or potential for optimization here; we are talking about removing the complexity of an uninitialized variable. You are going to do the assignment in the full screen case regardless.
| // destroy the pre-existing window if it exists | ||
| if (windowId != NULL) | ||
| // Create the SDL window if it doesn't already exist | ||
| if (!windowId) |
There was a problem hiding this comment.
I don't know if this is just a personal preference or if there is research to back this position, but I prefer the explicit test that was there before (updated for modern C++)
| if (!windowId) | |
| if (windowId != nullptr) |
There was a problem hiding this comment.
The opposite, I think, since the purpose of the comparison is now to create a new window versus destroying an existing one.
There was a problem hiding this comment.
Yes, of course. Hence the improved maintainability of the explicit logic test.
| if (!windowId) | ||
| { | ||
| printf("Could not create the window: %s\n", SDL_GetError()); | ||
| return false; |
There was a problem hiding this comment.
what happens in this case? There is no window, can the game be played? Multiple returns are bad.
I wonder if this might be one of those exceptional conditions where throwing an exception is warranted?
There was a problem hiding this comment.
// set fullscreen again so MainWindow object knows it's full screen
if (useFullscreen)
// this will also call window create
pmainWindow->setFullscreen();
else
window->create();
The return value is ignored.
It looks like he was just following the existing pattern. We do need a way to bail out from errors like this, but since that involves cleanup inside SDL (e.g., SDL_Quit()) and possibly outside of SDL as well, that seems more like a separate project entirely. SDL has functions to display an error dialog, or if SDL initialization itself failed, we can always fall back to stderr output. Yet another thing that would be simplified by going all in on SDL.
| targetWidth, | ||
| targetHeight, | ||
| flags); | ||
| // Store the gamma immediately after creating the first window |
There was a problem hiding this comment.
the first window? I thought the point was to only create one.
| else | ||
| eventStack.push_back(thisEvent); | ||
| { | ||
| //SDL_SetWindowSize(windowId, targetWidth, targetHeight); |
There was a problem hiding this comment.
why are we keeping commented out code?
This changes SDL2Window so that it only creates a window once. It initially creates a window at the base size (640x480 or what was provided by the -window argument). If the game is to be run fullscreen, it then changes to fullscreen. Window size or resolution changes and switching between fullscreen and windowed merely changes the size and properties of the existing window instead of destroying and recreating the window.
This is similar to #228 but takes it further by never recreating the window. Additionally, by creating the initial window at a lower resolution of 640x480, it works around the SDL2 multi-monitor issue in #201, assuming that there's at least a usable area of 640x480 (plus the size of the window decorations) on the primary display.
This likely breaks non-SDL2 platform code as I did adjust a couple other files. I have tested this mostly on Linux. I have not tested on macOS or with SDL1 or native Linux platform code.
I ran a brief test on Windows and changing the fullscreen resolution is broken, so I'll need to look into that some more. Specifically, when I had two 2560x1440 displays, was running fullscreen, and tried to change to 1920x1080, it started rendering on the secondary display with an old snapshot of the menu stuck on the primary display.