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

SDL3 multi viewport fix #8098

Closed

Conversation

SuperRonan
Copy link

Fixed wrong SDL_Window's flag inheritance from the main SDL_Window's flags.
Before the fix, flags that should not apply to viewports were inherited, like the fullscreen one, which created some problems (the viewport was not placed properly, with Vulkan it could conflict with the main window's surface).
The new version only inherits the HighDPI flag (like the SDL2 impl), which fixes the issues.
(Also SDL_WindowFlags (u64) is used rather than a u32).

@ocornut
Copy link
Owner

ocornut commented Oct 24, 2024

Hello,
Thanks for the PR. I have merged this with minor changes as 943e26b .
Two observations:

  • the flag was renamed to SDL_WINDOW_HIGH_PIXEL_DENSITY in SDL3.
  • I suspect it's not used at all. I couldn't find references in the code (might have missearched). In my experience high-dpi is always enabled on Windows.

@ocornut
Copy link
Owner

ocornut commented Oct 24, 2024

To clarify, even if SDL_WINDOW_HIGH_PIXEL_DENSITY is not used, the PR is indeed a fix by omitting the other flags.
I'm going to search if SDL_WINDOW_HIGH_PIXEL_DENSITY isn't an accidental leftover in the API, if that's the case we can remove the line.

Thanks!

@ocornut ocornut closed this Oct 24, 2024
@ocornut
Copy link
Owner

ocornut commented Oct 24, 2024

AMEND I indeed had missearched: the flag has no effect on Windows anymore but it does on other platforms.

@SuperRonan
Copy link
Author

About the flags being renamed in SDL3, I used the old one to be consistent with the rest of ImGui still uses the old ones (and are remapped to the new ones with #define SDL_ENABLE_OLD_NAMES).

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.

2 participants