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

Bring back borderless mode for macOS and Linux #6264

Merged
merged 6 commits into from
Dec 2, 2024

Conversation

Susko3
Copy link
Member

@Susko3 Susko3 commented Apr 22, 2024

Noticed when looking into #6262.

The SDL3 migration readme is cryptic about borderless being available:

SDL_WINDOW_FULLSCREEN_DESKTOP has been removed, and you can call SDL_GetWindowFullscreenMode() to see whether an exclusive fullscreen mode will be used or the borderless fullscreen desktop mode will be used when the window is fullscreen.

Not only can you check if the used fullscreen mode will be borderless, you can also set it by passing NULL to SDL_SetWindowFullscreenMode!

@Susko3
Copy link
Member Author

Susko3 commented Apr 22, 2024

Fullscreen on macOS somewhat doesn't take full control on SDL3 anymore, but cmd-tab has a horrible delay as opposed to borderless.

@Joehuu please check if the borderless mode works as expected in this pull.

@Joehuu
Copy link
Member

Joehuu commented Apr 22, 2024

Borderless itself seems to behave/work as expected (before SDL3) but windowed size gets maximised after toggling between the modes. Didn't happen in latest master.

Screen.Recording.2024-04-22.at.12.50.50.PM.mov

@peppy
Copy link
Member

peppy commented Apr 22, 2024

I'm also somewhat hesitant to add back borderless mode on macOS when it goes against how basically any other app works.

@bdach
Copy link
Collaborator

bdach commented May 19, 2024

If borderless isn't coming back then we should remove this game-side disclaimer.

Comment on lines 862 to 872
public static WindowState ToWindowState(this SDL_WindowFlags windowFlags, bool isFullscreenBorderless)
{
// for windows
if (windowFlags.HasFlagFast(SDL_WindowFlags.SDL_WINDOW_BORDERLESS))
return WindowState.FullscreenBorderless;

if (windowFlags.HasFlagFast(SDL_WindowFlags.SDL_WINDOW_MINIMIZED))
return WindowState.Minimised;

if (windowFlags.HasFlagFast(SDL_WindowFlags.SDL_WINDOW_FULLSCREEN))
return WindowState.Fullscreen;
return isFullscreenBorderless ? WindowState.FullscreenBorderless : WindowState.Fullscreen;
Copy link
Member

Choose a reason for hiding this comment

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

Regarding #6264 (comment), it appears that once the window is in borderless mode, it does not have the SDL_WINDOW_FULLSCREEN flag set:

CleanShot 2024-05-20 at 15 19 01

Therefore, the WindowState is kept at normal even after the window has switched to borderless, and sizeWindowed becomes corrupted as a result, causing the window to become maximised when returning from borderless.

Copy link
Member Author

Choose a reason for hiding this comment

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

@frenzibyte could you re-test this?

It's possible that this is because the window has not yet entered the fullscreen borderless mode due to animations still happening. Are the SDL window flags correct after the SDL_EVENT_WINDOW_ENTER_FULLSCREEN event arrives?

Copy link
Member

Choose a reason for hiding this comment

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

I can reproduce again, and the statement above seems correct. The following path is triggered before the SDL_EVENT_WINDOW_ENTER_FULLSCREEN event, causing the size bindable to incorrectly store the borderless size:

case SDL_EventType.SDL_EVENT_WINDOW_RESIZED:
// polling via SDL_PollEvent blocks on resizes (https://stackoverflow.com/a/50858339)
if (!updatingWindowStateAndSize)
fetchWindowSize();
break;

I recall there was something about SDL_EVENT_WINDOW_PIXEL_SIZE_CHANGED being more reliable than SDL_EVENT_WINDOW_RESIZED to get the window size while the window is in its final state, I'm not sure how correct that is right now.

All I can confirm is that SDL_EVENT_WINDOW_PIXEL_SIZE_CHANGED gets called after SDL_EVENT_WINDOW_ENTER_FULLSCREEN, and we have only been handling SDL_EVENT_WINDOW_RESIZED so the game doesn't look weird when resizing it. Perhaps we can just not store the window size in the "resized" event.

Copy link
Member Author

Choose a reason for hiding this comment

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

The following path is triggered [...]

When this is triggered, what is the value of SDL_GetWindowFlags(SDLWindowHandle)? Framework has cached a stale state, but what does SDL say?

Also please still check this:

Are the SDL window flags correct after the SDL_EVENT_WINDOW_ENTER_FULLSCREEN event arrives?

Copy link
Member

Choose a reason for hiding this comment

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

It does have SDL_WINDOW_FULLSCREEN when checking the flags at SDL_EVENT_WINDOW_RESIZED, while WindowState is indeed stale.

The next event triggered after resize is SDL_EVENT_WINDOW_ENTER_FULLSCREEN, which is what updates the WindowState to FullscreenBorderless because of calling updateAndFetchWindowSpecifics() I suppose, and yes the SDL window flags remains correct thereafter.

WindowState can sometimes be stale as `storeWindowSizeToConfig()`
may be called from `HandleEventFromWatch()`.
Comment on lines 789 to 791
private unsafe void storeWindowSizeToConfig()
{
if (WindowState != WindowState.Normal)
if (WindowState != WindowState.Normal || SDL_GetWindowFlags(SDLWindowHandle).HasFlagFast(SDL_WindowFlags.SDL_WINDOW_FULLSCREEN))
Copy link
Member

Choose a reason for hiding this comment

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

Reading this line makes me unsure whether WindowState can be correct anywhere, regardless of whether there is a comment explaining why this is being done. Thoughts on just not calling this method from the SDL_EVENT_WINDOW_RESIZED path?

Given that we know that path is not safe to read any states of the window, and is only meant to update the size of the window without doing anything else, I think it makes more sense that we just change that path to not do any storing of window size.

Copy link
Member Author

Choose a reason for hiding this comment

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

WindowState is probably invalid here as this is called from the event watch. Makes sense to avoid storing the size then.

Give it a test on TestSceneWindowed:

diff --git a/osu.Framework/Platform/SDL3/SDL3Window.cs b/osu.Framework/Platform/SDL3/SDL3Window.cs
index bb03b6093..33e509828 100644
--- a/osu.Framework/Platform/SDL3/SDL3Window.cs
+++ b/osu.Framework/Platform/SDL3/SDL3Window.cs
@@ -316,7 +316,7 @@ protected void HandleEventFromWatch(SDL_Event evt)
                 case SDL_EventType.SDL_EVENT_WINDOW_RESIZED:
                     // polling via SDL_PollEvent blocks on resizes (https://stackoverflow.com/a/50858339)
                     if (!updatingWindowStateAndSize)
-                        fetchWindowSize();
+                        fetchWindowSize(storeToConfig: false);
 
                     break;
             }
diff --git a/osu.Framework/Platform/SDL3/SDL3Window_Windowing.cs b/osu.Framework/Platform/SDL3/SDL3Window_Windowing.cs
index 8007a4a71..33f5a3512 100644
--- a/osu.Framework/Platform/SDL3/SDL3Window_Windowing.cs
+++ b/osu.Framework/Platform/SDL3/SDL3Window_Windowing.cs
@@ -445,7 +445,7 @@ private unsafe Rectangle windowDisplayBounds
         /// Updates <see cref="Size"/> and <see cref="Scale"/> according to SDL state.
         /// </summary>
         /// <returns>Whether the window size has been changed after updating.</returns>
-        private unsafe void fetchWindowSize()
+        private unsafe void fetchWindowSize(bool storeToConfig = true)
         {
             int w, h;
             SDL_GetWindowSize(SDLWindowHandle, &w, &h);
@@ -460,7 +460,8 @@ private unsafe void fetchWindowSize()
             Scale = (float)drawableW / w;
             Size = new Size(w, h);
 
-            storeWindowSizeToConfig();
+            if (storeToConfig)
+                storeWindowSizeToConfig();
         }
 
         #region SDL Event Handling
@@ -788,7 +789,7 @@ private void storeWindowPositionToConfig()
 
         private unsafe void storeWindowSizeToConfig()
         {
-            if (WindowState != WindowState.Normal || SDL_GetWindowFlags(SDLWindowHandle).HasFlagFast(SDL_WindowFlags.SDL_WINDOW_FULLSCREEN))
+            if (WindowState != WindowState.Normal)
                 return;
 
             storingSizeToConfig = true;

Copy link
Member

Choose a reason for hiding this comment

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

Looks to work as expected.

@pull-request-size pull-request-size bot added size/M and removed size/S labels Nov 25, 2024
Copy link
Member

@frenzibyte frenzibyte left a comment

Choose a reason for hiding this comment

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

Leaving a tentative approval to say this is pretty much good from macOS's side, and will at least bring back a less-broken fullscreen mode. Leaving it to others as to whether this can get in or not.

@frenzibyte frenzibyte requested a review from a team November 25, 2024 22:24
@peppy
Copy link
Member

peppy commented Dec 2, 2024

Gonna merge this to keep things moving.

@peppy peppy merged commit 5743c6e into ppy:master Dec 2, 2024
13 of 14 checks passed
@Susko3 Susko3 deleted the bring-back-borderless branch December 2, 2024 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants