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
Copy link
Member Author

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 January 14, 2021 21:09
src/video/win32_v.cpp Outdated Show resolved Hide resolved
@michicc michicc force-pushed the pr/autozoom_setting branch 2 times, most recently from a4dd7f8 to bb75c00 Compare January 14, 2021 21:40
@michicc
Copy link
Member Author

michicc commented Jan 14, 2021

Improved Win32 code a bit.

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

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

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

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
@TrueBrain TrueBrain added candidate: yes This Pull Request is a candidate for being merged size: small This Pull Request is small, and should be relative easy to process labels Feb 13, 2021
@michicc
Copy link
Member Author

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

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 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 pr/autozoom_setting branch 3 times, most recently from 034c181 to a9b2199 Compare February 13, 2021 20:03
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.
Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

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

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
@michicc michicc deleted the pr/autozoom_setting branch February 14, 2021 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: yes This Pull Request is a candidate for being merged size: small This Pull Request is small, and should be relative easy to process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants