Skip to content

SDL2: Only create the window once #238

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

SDL2: Only create the window once #238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e249f55
Refactor our SDL2 window creation so that we only create a window once.
blast007 Dec 29, 2019
f5c5e6d
Do not get the window size until the window has been created or resized.
blast007 Dec 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/bzflag/MainWindow.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,10 +277,9 @@ void MainWindow::setProjectionRadar(int x, int y, int w, int h, float radarRange

void MainWindow::resize()
{
window->getSize(trueWidth, trueHeight);
window->makeCurrent();
if (!window->create())
faulting = true;
window->getSize(trueWidth, trueHeight);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not at all clear what this change does. Some comments would be awesome

setQuadrant(quadrant);
}

Expand Down
3 changes: 0 additions & 3 deletions src/bzflag/playing.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1092,10 +1092,7 @@ static void doEvent(BzfDisplay *disply)

case BzfEvent::Resize:
if (mainWindow->getWidth() != event.resize.width || mainWindow->getHeight() != event.resize.height)
{
mainWindow->getWindow()->setSize(event.resize.width, event.resize.height);
mainWindow->getWindow()->callResizeCallbacks();
}
break;

case BzfEvent::Map:
Expand Down
186 changes: 81 additions & 105 deletions src/platform/SDL2Window.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,13 +125,7 @@ void SDLWindow::setSize(int _width, int _height)

void SDLWindow::getSize(int& width, int& height) const
{
if (fullScreen)
const_cast<SDLDisplay *>((const SDLDisplay *)getDisplay())->getWindowSize(width, height);
else
{
width = base_width;
height = base_height;
}
SDL_GL_GetDrawableSize(windowId, &width, &height);
}

void SDLWindow::setGamma(float gamma)
Expand Down Expand Up @@ -187,128 +181,110 @@ void SDLWindow::swapBuffers()
bool SDLWindow::create(void)
{
int targetWidth, targetHeight;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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.

getSize(targetWidth, targetHeight);
SDL_bool windowWasGrabbed = SDL_FALSE;
if (windowId != NULL)
windowWasGrabbed = SDL_GetWindowGrab(windowId);

// if we have an existing identical window, go no further
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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?

// Otherwise, it will be the base size, which controls the windowed size
else
{
int currentWidth, currentHeight;
SDL_GetWindowSize(windowId, &currentWidth, &currentHeight);

Uint32 priorWindowFlags = SDL_GetWindowFlags(windowId);
if (fullScreen == (priorWindowFlags & SDL_WINDOW_FULLSCREEN) &&
targetWidth == currentWidth && targetHeight == currentHeight)
return true;
targetWidth = base_width;
targetHeight = base_height;
}

// destroy the pre-existing window if it exists
if (windowId != NULL)
// Create the SDL window if it doesn't already exist
if (!windowId)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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++)

Suggested change
if (!windowId)
if (windowId != nullptr)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The opposite, I think, since the purpose of the comparison is now to create a new window versus destroying an existing one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, of course. Hence the improved maintainability of the explicit logic test.

{
if (glContext)
SDL_GL_DeleteContext(glContext);
glContext = NULL;

SDL_DestroyWindow(windowId);
}

// (re)create the window
const Uint32 flags = SDL_WINDOW_OPENGL |
(fullScreen ? SDL_WINDOW_FULLSCREEN : SDL_WINDOW_RESIZABLE) |
(windowWasGrabbed ? SDL_WINDOW_INPUT_GRABBED : 0);
const Uint32 flags = SDL_WINDOW_OPENGL | SDL_WINDOW_RESIZABLE;
windowId = SDL_CreateWindow(
title.c_str(),
SDL_WINDOWPOS_UNDEFINED,
SDL_WINDOWPOS_UNDEFINED,
base_width,
base_height,
flags);

// If the window could not be created, bail out
if (!windowId)
{
printf("Could not create the window: %s\n", SDL_GetError());
return false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

// 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.

}

windowId = SDL_CreateWindow(
title.c_str(),
SDL_WINDOWPOS_UNDEFINED,
SDL_WINDOWPOS_UNDEFINED,
targetWidth,
targetHeight,
flags);
// Store the gamma immediately after creating the first window
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the first window? I thought the point was to only create one.

if (origGamma < 0)
origGamma = getGamma();

// Store the gamma immediately after creating the first window
if (origGamma < 0)
origGamma = getGamma();
// Set the minimum window size
setMinSize(min_width, min_height);

// At least on Windows, recreating the window resets the gamma, so set it
setGamma(lastGamma);
// Create the OpenGL context and make it current
makeContext();
makeCurrent();

#ifdef _WIN32
SDL_VERSION(&info.version);
if (SDL_GetWindowWMInfo(windowId,&info))
{
if (info.subsystem == SDL_SYSWM_WINDOWS)
hwnd = info.info.win.window;
}
SDL_VERSION(&info.version);
if (SDL_GetWindowWMInfo(windowId,&info))
{
if (info.subsystem == SDL_SYSWM_WINDOWS)
hwnd = info.info.win.window;
}
#endif

if (!windowId)
{
printf("Could not set Video Mode: %s.\n", SDL_GetError());
return false;
}

if (min_width >= 0)
setMinSize(min_width, min_height);

makeContext();
makeCurrent();

if(SDL_GL_SetSwapInterval(vsync ? -1 : 0) == -1 && vsync)
// no adaptive vsync; set regular vsync
SDL_GL_SetSwapInterval(1);

// init opengl context
OpenGLGState::initContext();
// Set desired vertical-sync mode
setVerticalSync(vsync);

// workaround for SDL 2 bug on mac where toggling fullscreen will
// generate a resize event and mess up the window size/resolution
// (TODO: remove this if they ever fix it)
// bug report: https://bugzilla.libsdl.org/show_bug.cgi?id=3146
#ifdef __APPLE__
if (fullScreen)
return true;

int currentDisplayIndex = SDL_GetWindowDisplayIndex(windowId);
if (currentDisplayIndex < 0)
{
printf("Unable to get current display index: %s\n", SDL_GetError());
return true;
// init opengl context
OpenGLGState::initContext();
}

SDL_DisplayMode desktopDisplayMode;
if (SDL_GetDesktopDisplayMode(currentDisplayIndex, &desktopDisplayMode) < 0)
{
printf("Unable to get desktop display mode: %s\n", SDL_GetError());
return true;
}
// Get the current window dimensions
int currentWidth, currentHeight;
SDL_GetWindowSize(windowId, &currentWidth, &currentHeight);

std::vector<SDL_Event> eventStack;
SDL_Event thisEvent;
// Get the current window flags
Uint32 currentWindowFlags = SDL_GetWindowFlags(windowId);

// pop off all the events except a resize event
while (SDL_PollEvent(&thisEvent))
if (targetWidth != currentWidth || targetHeight != currentHeight)
{
if (thisEvent.type == SDL_WINDOWEVENT && thisEvent.window.event == SDL_WINDOWEVENT_RESIZED)
// If we're fullscreen (or switching to fullscreen), find the closest resolution and set the display mode
if (fullScreen)
{
// switching from "native" fullscreen to SDL fullscreen and then going back to
// windowed mode will generate a legitimate resize event, so add it back
if (thisEvent.window.data1 != desktopDisplayMode.w || thisEvent.window.data2 != desktopDisplayMode.h)
eventStack.push_back(thisEvent);
SDL_DisplayMode closest;
SDL_DisplayMode target;
target.w = targetWidth;
target.h = targetHeight;
target.format = 0;
target.refresh_rate = 0;
target.driverdata = nullptr;

// Attempt to find a usable resolution close to our target resolution
if (SDL_GetClosestDisplayMode(0, &target, &closest) == nullptr)
{
printf("Unable to find a usable fullscreen resolution: %s\n", SDL_GetError());
return false;
}

// Attempt to set the display mode
if (SDL_SetWindowDisplayMode(windowId, &closest) < 0)
{
printf("Unable to set display mode: %s", SDL_GetError());
return false;
}
}
// Otherwise just set the window size
else
eventStack.push_back(thisEvent);
{
//SDL_SetWindowSize(windowId, targetWidth, targetHeight);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we keeping commented out code?

}
}

// push them back on in the same order
while (eventStack.size() > 0)
// Check if we need to toggle between fullscreen and windowed
if (fullScreen != (currentWindowFlags & SDL_WINDOW_FULLSCREEN))
{
SDL_PushEvent(&eventStack[0]);

eventStack.erase(eventStack.begin());
// Adjust the fullscreen/resizable flags
SDL_SetWindowFullscreen(windowId, fullScreen?SDL_WINDOW_FULLSCREEN:0);
SDL_SetWindowResizable(windowId, fullScreen?SDL_FALSE:SDL_TRUE);
}
#endif //__APPLE__

return true;
}
Expand Down