Skip to content
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.

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

Another attempt to fix multimonitor issues with SDL backend #2010

Merged
merged 5 commits into from
Feb 9, 2025

Conversation

OlegAckbar
Copy link
Contributor

In this PR we will do the next:

  1. Let the platform decide where to put our fullscreen window instead of unconditionally putting it at (0,0)
  2. Fix window save position on multimonitor configurations.
  3. Enumerate video modes for display where our window is created instead of display index zero.

Copy link
Member

@a1batross a1batross left a comment

Choose a reason for hiding this comment

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

I'm mostly fine with these changes, but you need to format the code.

engine/platform/sdl/vid_sdl.c Outdated Show resolved Hide resolved
engine/platform/sdl/vid_sdl.c Outdated Show resolved Hide resolved
engine/platform/sdl/vid_sdl.c Show resolved Hide resolved
@@ -757,34 +778,37 @@ qboolean VID_CreateWindow( int width, int height, window_mode_t window_mode )

if( window_mode == WINDOW_MODE_WINDOWED )
{
SDL_Rect r;
SDL_Rect *display_rects = (SDL_Rect *)malloc( num_displays * sizeof( SDL_Rect ));
Copy link
Member

Choose a reason for hiding this comment

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

You forgot to free display_rects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a little confused: have I done this correctly now?

Copy link
Member

Choose a reason for hiding this comment

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

No, the memory you allocated with malloc must be deallocated using free, once you don't need it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see. Should be done now.

@OlegAckbar OlegAckbar force-pushed the master branch 2 times, most recently from 5c459a9 to b5402f9 Compare February 8, 2025 17:00
if( !display_rects )
{
Con_Printf( S_ERROR "Failed to allocate memory for display rects!\n" );
SDL_Quit();
Copy link
Member

@a1batross a1batross Feb 8, 2025

Choose a reason for hiding this comment

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

Also, it should be Sys_Error, if you want to exit.

But it would be more correct to handle the error if allocating display_rects has failed and just fall back to SDL_WINDOWPOS_UNDEFINED.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

{
if( SDL_GetDisplayBounds( i, &display_rects[i] ) != 0 )
{
Con_Printf( S_ERROR "Failed to get bounds for display %d! SDL_Error: %s\n", i, SDL_GetError());
Copy link
Member

Choose a reason for hiding this comment

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

You need to handle this error as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}
free(display_rects);
Copy link
Member

Choose a reason for hiding this comment

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

Spaces in parentheses.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. One day I'll get used to this.

@OlegAckbar
Copy link
Contributor Author

I've uploaded one more fix. Previously window position would only be saved in window mode, and display in fullscreen mode would always be dictated by the platform. Now engine will also remember last used display.

Copy link
Member

@a1batross a1batross left a comment

Choose a reason for hiding this comment

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

Yep, looks good to me! Thank you!

@a1batross a1batross merged commit eda0ac9 into FWGS:master Feb 9, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants