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

Mac fullscreen fix #817

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

larsy1995
Copy link
Contributor

Changes windowed mode on macOS to use native macOS fullscreen instead of SDL to enable macOS menubar access while in borderless fullscreen mode.

This workaround is needed until we update to SDL3 and can use SDL_HINT_VIDEO_MAC_FULLSCREEN_MENU_VISIBILITY for this functionality.
Also fixes crashing when going from native macOS fullscreen to SDL.

Also changes the default fullscreen behaviour from exclusive to borderless windowed.
This will need a separate PR in each game to change this visually, as well as to disable the fullscreen toggle while already in a fullscreen mode to avoid unintended behaviour.

Thanks to @ReddestDream for all the help testing and coming up with the workaround with disabling the toggle while in a fullscreen mode.

Fixes the issue where windowed fullscreen cursor hiding didn't work as it should according to the presence of the imGUI menubar.
We still need a specific fix in each game to disable changing fullscreen mode while in fullscreen, but this can be removed when SDL3 gets implemented in the future.

Also renamed some macOS specific functions and variables to make sure it is clear that they are macOS only.
@ReddestDream
Copy link
Contributor

Note: This PR works best with and was designed with the macOS Scaling Fix in mind: #811

@ReddestDream
Copy link
Contributor

ReddestDream commented Feb 13, 2025

Also note: This PR was designed to address this issue with the input of @Archez: #661

// SDL for Exclusive Fullscreen. This code can and will be changed when we upgrade to SDL3 because of the
// ability to use SDL_HINT_VIDEO_MAC_FULLSCREEN_MENU_VISIBILITY to get the menubar working instead of this
// workaround
bool useNativeMacOSFullscreen = !CVarGetInteger(CVAR_SDL_WINDOWED_FULLSCREEN, 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

my biggest issue with this PR is that it is changing the meaning of CVAR_SDL_WINDOWED_FULLSCREEN

Copy link
Contributor

Choose a reason for hiding this comment

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

That is understandable. larsy1995 (Fenrir) can say more, but my understanding is that this was done to conform with what @Archez suggested on Discord: "It might make sense for the LUS fullscreen to use the OS one when "windowed fullscreen" is off, and the SDL one when on." We followed this suggestion for the implementation here since this is a solution to the issue by @Archez here #661. If it turns out that this isn't the best way, please let us know. Thanks!

Copy link

Choose a reason for hiding this comment

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

How about we rename the CVAR? To make an emphasis on the fact that it changes the behavior of the fullscreen mode, not just borderless window mode on and off?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At least that way it would be clearer on macOS and not make it confusing for other devs. A universal changeFullscreenMode CVar.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the context and use cases of our ports, is there any reason why anyone would want "exclusive fullscreen" over the native behavior for MacOS? I only originally made that suggestion as a "if both matter", but I'm not convinced at this point that both matter.

It seems all this could just be simplified on MacOS (hide the windowed fullscreen toggle/ignore the CVar).

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the context and use cases of our ports, is there any reason why anyone would want "exclusive fullscreen" over the native behavior for MacOS? I only originally made that suggestion as a "if both matter", but I'm not convinced at this point that both matter.

It seems all this could just be simplified on MacOS (hide the windowed fullscreen toggle/ignore the CVar).

yeah, i suggested the same thing (just always use native on mac) in HarbourMasters/Shipwright#5060

Copy link
Contributor

@ReddestDream ReddestDream Feb 21, 2025

Choose a reason for hiding this comment

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

I'm not convinced that both matter either. IIRC, we kept the exclusive fullscreen because it was able to use "Direct" rendering while the native only used "Composited," but I'm not really sure that matters in the end.

Copy link
Contributor

Choose a reason for hiding this comment

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

I got confirmation from larsy1995 (Fenrir) that, yes, we kept the exclusive (SDL) fullscreen because it was the only way to get Direct rendering. Again, if that matters. Also, when SDL3 is available, as noted above, there may even be further streamlining possible.

Copy link
Contributor

@ReddestDream ReddestDream Feb 21, 2025

Choose a reason for hiding this comment

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

Just as a note, other SDL apps (e.g., DOSBox-X) on macOS offer two fullscreen options kinda like what we have here. Again, it's a question of how much it matters and what makes sense for the project, clarity, consistency, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anyhow, this fix also makes the f11 button exit native fullscreen as well. I agree the code would be cleaner with just native borderless and is what I wanted from the start personally, but I just followed the specs given by Archez in that issue ReddestDream linked earlier.
I just need the go ahead and I'll make the changes to both this and the menubars asap.

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.

4 participants