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

SDL2: Support highdpi #14703

Merged
merged 2 commits into from
Jun 16, 2024
Merged

SDL2: Support highdpi #14703

merged 2 commits into from
Jun 16, 2024

Conversation

grorp
Copy link
Member

@grorp grorp commented May 25, 2024

Fixes #14549

Tested on Windows. This could work on Mac too, but I don't have a device to test it there.

I used CIrrDeviceSDL from https://github.com/MoNTE48/Irrlicht as a reference, but I believe my implementation is slightly more elegant.

To do

This PR is a Ready for Review.

How to test

Set the display/screen scaling settings on Windows, verify that Minetest respects them. Verify that mouse input works correctly. Also verify that the window size is saved and restored correctly.

@grorp grorp added Windows Bugfix 🐛 PRs that fix a bug @ Engine Core What happens inside the very engine WIP and removed WIP labels May 25, 2024
@superfloh247
Copy link
Contributor

tested on Mac: everything looks and behaves normal

@grorp
Copy link
Member Author

grorp commented May 26, 2024

@superfloh247 in #14268 (comment), you wrote for USE_SDL2=TRUE:

HUD elements and texts are way larger

Does this PR fix that?

@superfloh247
Copy link
Contributor

@superfloh247 in #14268 (comment), you wrote for USE_SDL2=TRUE:

HUD elements and texts are way larger

Does this PR fix that?

you tell me

screenshot_20240526_225203

@grorp
Copy link
Member Author

grorp commented May 26, 2024

you tell me

Your old screenshots have a different resolution. With the old screenshots scaled down to 50% to be able to compare, it looks like the issue is fixed. Is it?

@superfloh247
Copy link
Contributor

without patch

screenshot_20240527_080048

@superfloh247
Copy link
Contributor

looks like the font size increases a tiny bit with this patch

@grorp
Copy link
Member Author

grorp commented May 27, 2024

looks like the font size increases a tiny bit with this patch

The font size on both screenshots looks the same to me

without patch

I assume the "with patch" screenshot is with USE_SDL2=TRUE. What about the "without patch" screenshot? Is it with USE_SDL2=FALSE?

@superfloh247
Copy link
Contributor

with PR, USE_SDL2=TRUE

screenshot_20240528_075156

with PR, USE_SDL2=FALSE

screenshot_20240528_075514

without PR, USE_SDL2=TRUE

screenshot_20240528_075835

without PR, USE_SDL2=FALSE

screenshot_20240528_080246

you're right, all texts look exactly the same

@grorp
Copy link
Member Author

grorp commented May 28, 2024

Ok, thank you for testing. The results don't make much sense for me, but at least nothing is broken.

@grorp grorp force-pushed the sdl-highdpi branch 2 times, most recently from 76b21e7 to 62ca274 Compare June 3, 2024 15:07
builtin/settingtypes.txt Outdated Show resolved Hide resolved
irr/src/CIrrDeviceSDL.cpp Show resolved Hide resolved
src/client/renderingengine.cpp Outdated Show resolved Hide resolved
@grorp grorp requested a review from sfan5 June 7, 2024 13:02
Copy link
Collaborator

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

doesn't work on my xfce/x11 hidpi system: window and everything else is too small (scale not detected)

@grorp
Copy link
Member Author

grorp commented Jun 8, 2024

Lots of additional information

I had only tested this PR on Windows so far. Now I also tested it on two Linux systems. Here are my results:

100% 200%
Fedora Linux 40 (Workstation Edition)
GNOME 46
X11
window size = 1920x1080
drawable size = 1920x1080
scale = 1; 1 ✔
SDL_GetDisplayDPI() scale = 1.05833;1.05833 ❌
window size = 1920x1080
drawable size = 1920x1080
scale = 1; 1 ❌
SDL_GetDisplayDPI() scale = 1.05833;1.05833 ❌
Fedora Linux 40 (Workstation Edition)
GNOME 46
Wayland
window size = 1920x1080
drawable size = 1920x1080
scale = 1; 1 ✔
SDL_GetDisplayDPI() scale = 1.05833;1.05833 ❌
window size = 960x540
drawable size = 1920x1080
scale = 2; 2 ✔
SDL_GetDisplayDPI() scale = 1.05833;1.05833 ❌
Ubuntu 23.10
GNOME 45.2
X11
window size = 1920x1080
drawable size = 1920x1080
scale = 1; 1 ✔
SDL_GetDisplayDPI() scale = 1.47674;1.47294 ❌
window size = 1920x1080
drawable size = 1920x1080
scale = 1; 1 ❌
SDL_GetDisplayDPI() scale = 1.47674;1.47294 ❌
Ubuntu 23.10
GNOME 45.2
Wayland
window size = 1920x1080
drawable size = 1920x1080
scale = 1; 1 ✔
SDL_GetDisplayDPI() scale = 1.49412;1.50395 ❌
window size = 1920x1080
drawable size = 1920x1080
scale = 1; 1 ❌
SDL_GetDisplayDPI() scale = 1.49412;1.50395 ❌
100% ("fractional scaling") 125% ("fractional scaling") 200% ("fractional scaling")
Fedora Linux 40 (Workstation Edition)
GNOME 46
X11
n/a on Fedora n/a on Fedora n/a on Fedora
Fedora Linux 40 (Workstation Edition)
GNOME 46
Wayland
n/a on Fedora n/a on Fedora n/a on Fedora
Ubuntu 23.10
GNOME 45.2
X11
window size = 1920x1080
drawable size = 1920x1080
scale = 1; 1 ✔
SDL_GetDisplayDPI() scale = 1.47674;1.47294 ❌
jumping between two values:

window size = 1920x1080
drawable size = 1920x1080
scale = 1; 1 ❌
SDL_GetDisplayDPI() scale = 2.36279;2.3567 ❌

window size = 3072x1728
drawable size = 3072x1728
scale = 1; 1 ❌
SDL_GetDisplayDPI() scale = 2.36279;2.3567 ❌
(downscaled by system => wtf?)
window size = 1920x1080
drawable size = 1920x1080
scale = 1; 1 ❌
SDL_GetDisplayDPI() scale = 1.47674;1.47294 ❌
Ubuntu 23.10
GNOME 45.2
Wayland
window size = 1920x1080
drawable size = 1920x1080
scale = 1; 1 ✔
SDL_GetDisplayDPI() scale = 1.49412;1.50395 ❌
window size = 1536x864
drawable size = 1536x864
scale = 1; 1 ❌
SDL_GetDisplayDPI() scale = 1.19529;1.20316 ❌
(upscaled by system => blurry)
window size = 960x540
drawable size = 960x540
scale = 1; 1 ❌
SDL_GetDisplayDPI() scale = 0.747059;0.751974 ❌
(upscaled by system => blurry)

These are the changes I used: grorp@7f1438f

There are basically two ways to get a scaling factor from SDL2:

  1. SDL_GL_GetDrawableSize() / SDL_GetWindowSize() * 96
    => later divided by 96 by Minetest

  2. SDL_GetDisplayDPI()
    => later divided by 96 by Minetest

This PR is based on the first approach, which is what the SDL2 docs recommend: https://wiki.libsdl.org/SDL2/SDL_GetDisplayDPI#remarks

Takeaways:

doesn't work on my xfce/x11 hidpi system: window and everything else is too small (scale not detected)

I don't see a way I could improve this PR to make it work better on Linux/X11. It looks like SDL2 simply doesn't support highdpi on X11 (but SDL3 does): libsdl-org/SDL#4176 / libsdl-org/SDL#7718

@grorp
Copy link
Member Author

grorp commented Jun 8, 2024

@superfloh247 it would be great if you could test this PR again. After e0e2f3c, it might actually make a difference on macos (but only if you have a retina/hidpi display).

@superfloh247
Copy link
Contributor

yes it does make a difference
-USE_SDL2=TRUE

do you need any other setting that could influence the fonts?

HUD elements and HUD fonts very small, way too small
other text eg. on signs has normal font size

screenshot_20240608_172254

screenshot_20240608_172338

@grorp
Copy link
Member Author

grorp commented Jun 8, 2024

HUD elements and HUD fonts very small, way too small

Could you test grorp@efbe0d7 and tell me what it outputs to the console after [updateSizeAndScale]?

EDIT: There was a new bug that caused a scale of 0.5 with CIrrDeviceOSX. I've added the fix for that bug to the commit linked above. This shouldn't fix your problem, but if it does, that means you're not compiling with CIrrDeviceSDL.

do you need any other setting that could influence the fonts?

Ideally, you'd test this with the default settings, i.e. an empty minetest.conf file.

Another question: Is there a resolution difference between screenshots taken with Minetest's F12 and screenshots taken with macOS' screenshot feature?

@sfan5
Copy link
Collaborator

sfan5 commented Jun 9, 2024

I don't see a way I could improve this PR to make it work better on Linux/X11. It looks like SDL2 simply doesn't support highdpi on X11 (but SDL3 does)

That's disappointing and actually means a loss of features compares to the older "Linux" device.

Copy link
Collaborator

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

code lgtm

@sfan5 sfan5 added One approval ✅ ◻️ Rebase needed The PR needs to be rebased by its author labels Jun 9, 2024
and handle DPI changes at runtime
@grorp grorp removed the Rebase needed The PR needs to be rebased by its author label Jun 11, 2024
@okias
Copy link
Contributor

okias commented Jun 12, 2024

I see more and more stuff lacking on X11 (see #14542 Avoid bug on Linux/X11), may I suggest incorporating X11 (and XWayland) deprecation into roadmap? I think there will be more and more problems, as XWayland is (and will be) maintained, but X11 is EOL for some time.

I don't want to cut X11 users, but giving them grace period to migrate (even Xfce working on the transition) is the best thing..

@sfan5
Copy link
Collaborator

sfan5 commented Jun 12, 2024

I don't think that would make much sense as X11 will be with us for at least a few years in the Linux world and there's few signs of the BSD world moving.

@superfloh247
Copy link
Contributor

with this PR and without -USE_SDL2=TRUE

screenshot_20240614_084357

with this PR and with -USE_SDL2=TRUE

screenshot_20240614_084746

latest master without -USE_SDL2=TRUE

screenshot_20240614_085044

latest master with -USE_SDL2=TRUE

screenshot_20240614_085329

the screenshots taken in minetest look exactly the same as the game looks on screen

latest master with -USE_SDL2=TRUE, screenshot taken with OS's snipping tool

Bildschirmfoto 2024-06-14 um 08 54 31

@superfloh247
Copy link
Contributor

build.log

@superfloh247
Copy link
Contributor

screenshot_20240614_160558

@superfloh247
Copy link
Contributor

Irrlicht Device: SDL
fonts langer than usual, but very smooth

key binding broken
l-shift does not work to fly down / sneak
l-shift does not work to start commands with / in game mode, only when I open the text console with t, then l-shift works

@grorp
Copy link
Member Author

grorp commented Jun 15, 2024

Seems to work on macOS. Thank you.

The default on macOS is still CIrrDeviceOSX, which doesn't support highdpi. It looks like NSHighResolutionCapable was explicitly set to false by #10970 to fix #10654.

To avoid unexpected behavior changes/regressions with CIrrDeviceOSX, I've reverted the NSHighResolutionCapable change until we switch macOS to CIrrDeviceSDL by default.

@grorp grorp merged commit a9cca5e into luanti-org:master Jun 16, 2024
13 checks passed
@grorp grorp deleted the sdl-highdpi branch June 16, 2024 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix 🐛 PRs that fix a bug @ Engine Core What happens inside the very engine One approval ✅ ◻️ Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SDL: Support high DPI
4 participants