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

Feature: Automatic UI and font zoom levels. #8537

Merged
merged 6 commits into from Feb 14, 2021

Conversation

@michicc
Copy link
Member

@michicc michicc commented Jan 8, 2021

Motivation / Problem

On HiDPI screens, the default GUI and font zoom might be too small to read.

Description

An additional (and default) "auto" value for the zoom settings is added, that will get the current DPI value/scaling from the video driver and choose a zoom value accordingly.

For Win32 and OSX, the set thresholds are 2X zoom at a DPI scale > 150% and 4X zoom at a scale > 300%. Current OSX version will only report 100% or 200% for the scale factor, but the implementation still checks for 300% to allow for future OSX updates.

Limitations

I've marked this as a draft, as the OSX zoom level suggestion as implemented in this PR right now only makes sense together with #8519. It is it's own commit, so it could easily be stripped off for now.

It is also missing a SDL implementation, at least if HiDPI is a thing with SDL.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')
@michicc michicc force-pushed the michicc:pr/autozoom_setting branch from adb76da to 39d8294 Jan 14, 2021
@michicc
Copy link
Member Author

@michicc michicc commented Jan 14, 2021

Stripped OSX for now, will tack it on #8519 later.

@michicc michicc marked this pull request as ready for review Jan 14, 2021
src/video/win32_v.cpp Outdated Show resolved Hide resolved
@michicc michicc force-pushed the michicc:pr/autozoom_setting branch 2 times, most recently from a4dd7f8 to bb75c00 Jan 14, 2021
@michicc
Copy link
Member Author

@michicc michicc commented Jan 14, 2021

Improved Win32 code a bit.

@michicc michicc force-pushed the michicc:pr/autozoom_setting branch 2 times, most recently from 2f3ab7a to df7245c Jan 14, 2021
@michicc michicc force-pushed the michicc:pr/autozoom_setting branch from df7245c to a7199b4 Feb 12, 2021
Copy link
Member

@TrueBrain TrueBrain left a comment

In general, I think we should do something like this.

But, there is a but: we only have this for Windows implemented now. The commit message "Feature: .." suggests it is implemented for all. On all video drivers you can see the "auto" value. Yet, on most, it doesn't really do anything.

Not sure how to deal with this. Possibly simply change the feature into making it clear it is only for a select subset of video-drivers, but .. dunno :)

src/video/win32_v.cpp Outdated Show resolved Hide resolved
src/video/win32_v.cpp Show resolved Hide resolved
src/zoom_type.h Outdated Show resolved Hide resolved
src/settings_gui.cpp Outdated Show resolved Hide resolved
src/settings_gui.cpp Outdated Show resolved Hide resolved
src/settings_gui.cpp Outdated Show resolved Hide resolved
src/lang/english.txt Outdated Show resolved Hide resolved
@michicc
Copy link
Member Author

@michicc michicc commented Feb 13, 2021

General comment: Per previous IRC chat, apparently SDL (at least as currently used by us) has no concept of a HiDPI window, which means nothing to scale. On OSX, we currently force OS scaling, automatic zoom would only be a thing together with #8519.

Specific comments will be dealt with later.

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Feb 13, 2021

General comment: Per previous IRC chat, apparently SDL (at least as currently used by us) has no concept of a HiDPI window, which means nothing to scale. On OSX, we currently force OS scaling, automatic zoom would only be a thing together with #8519.

For a moment I was thinking: maybe we shouldn't show the "auto" option for SDL, for example. But I realised ... nobody is going to care if there is an "auto" option in there, that doesn't do anything for their driver. They simply will not notice. So that is just extra work for no added benefit. So yeah, forget about all that, let's just make this a generic "feature" on which we should extend.

For example, SDL does have HiDPI support just fine, but we don't use it. There is a bit of work to do for us to support it, but not much. So we might as well. There are functions like SDL_GetDisplayDPI which are implemented for Windows, X11, and of course mac OS. I might give a crack at this next week .. as why not!

src/video/win32_v.cpp Outdated Show resolved Hide resolved
@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Feb 13, 2021

For example, SDL does have HiDPI support just fine, but we don't use it. There is a bit of work to do for us to support it, but not much. So we might as well. There are functions like SDL_GetDisplayDPI which are implemented for Windows, X11, and of course mac OS. I might give a crack at this next week .. as why not!

So I was thinking this would be simple, SDL is giving us a function, so how hard could it be? Well, for Windows, it is fine. But we of course mostly target Linux with SDL ... euh .. yeah ...

Someone worked out nicely how SDL behaves here, which you can read here:
mosra/magnum@ae31c3c

And if we look in the SDL source, we found on X11 SDL_GetDisplayDPI gets its values from here:
https://github.com/libsdl-org/SDL/blob/main/src/video/x11/SDL_x11modes.c#L473
https://github.com/libsdl-org/SDL/blob/main/src/video/x11/SDL_x11modes.c#L756

Long story short: SDL_GetDisplayDPI will never return a useful result on Linux. A good solution is to use Xft.dpi, which is exactly what we would like (and the same as in the Win32 driver), but it requires some magic with dl:
mosra/magnum@ae31c3c#diff-953f069b28302b3aa5079d9fe56f4e646c93e52a09891554d4150f3dd2ebd0c4R64

This would be similar to how Win32 driver now solves it, so that might be okay, but do we want to go that route, I wonder?

@michicc michicc force-pushed the michicc:pr/autozoom_setting branch 3 times, most recently from 034c181 to a9b2199 Feb 13, 2021
michicc added 6 commits Jan 8, 2021
Use the same Windows XP target as for 64-bit. Current MSVC version will
not produce a binary that works on anything earlier anyway.
The zoom level suggestion is based on the DPI scaling set in Windows.
We use 150% scaling as the threshold for 2X zoom and 300% scaling
as the threshold for 4X zoom.
@michicc michicc force-pushed the michicc:pr/autozoom_setting branch from a9b2199 to ed4238b Feb 14, 2021
Copy link
Member

@TrueBrain TrueBrain left a comment

I will see what I can do for SDL this week; so we can either hold off on this PR till that is done, or just PR it later. I think the latter works just fine.

@michicc michicc merged commit 8d780e0 into OpenTTD:master Feb 14, 2021
10 checks passed
10 checks passed
Emscripten
Details
Commit checker
Details
Check preview needs update Check preview needs update
Details
Linux (clang, clang++)
Details
Linux (gcc, g++)
Details
Mac OS (x64, x86_64)
Details
Windows (windows-latest, x86)
Details
Windows (windows-latest, x64)
Details
Windows (windows-2016, x86) Windows (windows-2016, x86)
Details
Windows (windows-2016, x64) Windows (windows-2016, x64)
Details
@michicc michicc deleted the michicc:pr/autozoom_setting branch Feb 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants