Return-To-The-Roots / s25client

Return To The Roots (Settlers II(R) Clone)
http://www.rttr.info
GNU General Public License v2.0
470 stars 75 forks source link

Implement GUI scaling for HighDPI displays and accessibility (SDL2 only) #1594

Closed falbrechtskirchinger closed 11 months ago

falbrechtskirchinger commented 1 year ago

This PR implements a global scaling of the window content in the SDL2 driver (a stub has been added to the the WinAPI driver).

The window zoom can be adjusted in the options screen by selecting the desired zoom level from a combo box or by holding down Control and scrolling.

The zoom behavior in the menu screens is a little janky due to how the controls are sized. I've not investigated this any further. After looking at #1579 again, I suspect this is related to RescaleWindowProp/ScaleWindowPropUp. Maybe there's something to be done?

I don't own a HighDPI display myself and have only tested it on setups where windowSize == renderSize. Some optional improvements are under consideration for this PR and are listed in the to-do list below.

Fixes #533 Closes #1579

To-do:

falbrechtskirchinger commented 1 year ago

I've incorporated @Spikeone's feedback and added an option to decouple scaling the game world from scaling the GUI. On HighDPI setups, some scaling is applied regardless of the setting.

I want to note that I very specifically chose the term "window zoom" (inspired by VSCode) over "GUI scale" because the game world was scaled as well (I wasn't eager to figure out how not to when I started and was far less familiar with the code base than I am now). Unlike in 3D games where scaling is limited to the GUI/HUD and scaling the game world makes no sense due to the perspective projection.

Arguably, this setting could be removed, "Window Zoom" be renamed to "GUI Scale," and the game world scaled depending on DPI, as is the case with the setting turned off.

Xellzul commented 1 year ago

@falbrechtskirchinger Hey, had quick look at this PR (e64943ef6c81c0d21013403049c62f4b9ec55a80) on Windows 11 at 6880x2880 (21:9) resolution:

Everything works as expected except: Taking in-game screenshots using F12, this was the result with scaling (note the size 1720 x 720 of the screenshot was exact 1/4 of used resolution of 6880x2880 using 400% scaling): 2023-07-11_19-56-19

In-game graph was thin Screenshot 2023-07-11 200349

Few screenshots: https://imgur.com/a/VX9xn1u

falbrechtskirchinger commented 1 year ago

@Xellzul Thank you! I'm away for a few days but should be able to fix those issues over the weekend.

falbrechtskirchinger commented 1 year ago

Both issues should be fixed. As expected, they required minimal changes, though the line drawing could be optimized by calculating the line width only once at a higher level – sacrificing a degree of abstraction – or by implementing BeginDrawLine()/EndDrawLine() functions to calculate and set the width once per multiple draw calls.

iwStatistics could also benefit from a significant rewrite, as many draw positions are specified manually. (In a separate PR and at a later date, of course.)

falbrechtskirchinger commented 12 months ago
  • Should the setting be renamed to (G)UI scale and Zoom Game World be removed and always disabled?

I'm making an executive decision and going ahead with the change. "Window Zoom" becomes "GUI Scale" and the DPI scale always scales the game world view. I'm also moving from using unsigned to a new type that encapsulates all the precomputed values and the functions to transform points, obviating some of the additions to `IVideoDriver, as well as further additions or redundant calculations.

falbrechtskirchinger commented 11 months ago

I've started using C++17 features. CI is expected to fail accordingly.

Flamefire commented 11 months ago

Please use force-push more carefully. After starting to review a PR a force-push means loosing any information about the state the last conversation was about and what the new changes are. It may make sense to do a final rebase and/or cleanup once all is resolved right before merging but until then especially for those large changes I find it confusing.

Idea: Prefix a commit meant to fix an older commit with "fixup: " such that it sticks out and it is easy to combine those commits later on in a single final rebase.

falbrechtskirchinger commented 11 months ago

Please use force-push more carefully.

The last force-push was an unfortunate accident. I moved my work to a new branch and pushed the way outdated old window-zoom branch. (Renamed it now and set the tracking branch properly to avoid this in the future.)

Idea: Prefix a commit meant to fix an older commit with "fixup: " such that it sticks out and it is easy to combine those commits later on in a single final rebase.

I'm using git commit --fixup with git rebase -i anyway. I can just push my "fixup! " commits without the rebase.

I can merge master instead of rebasing for C++17 if you prefer.

falbrechtskirchinger commented 11 months ago

Disclaimer: I can't do a functional test of the changes right now (graphics driver was updated and I can't reboot at the moment).

falbrechtskirchinger commented 11 months ago

OK, the code seems to work as expected. I didn't find anything else beyond the selection/index issue.

If this iteration gets the seal of approval, remember to let me do a final rebase to clean up history before merging.

falbrechtskirchinger commented 11 months ago

Bonus points for adding a small test for the dskOptions that would have caught the index off-by-one issue you just fixed. But I know tests for those classes are often a nightmare so not required.

I'm already planning a small follow-up PR anyway (multi-monitor DPI awareness). The tests aren't related but I'd maybe include them then. I can think of more things that could be tested in dskOptions.

falbrechtskirchinger commented 11 months ago

@Flamefire The rebase is done. Once you approve the changes a force-push should happen automatically within about a minute. I've added a few more doc comments and applied one of your suggestions to another line.

Blackpearl1 commented 10 months ago

I just used the newest nightly(e37a322)

I change the GUI zoom to 200% and.... nothing happen. Restartet the game and... nothing happen.

I Use 3840x2160 (16:9)

Whats wrong?

falbrechtskirchinger commented 10 months ago

I just used the newest nightly(e37a322)

I change the GUI zoom to 200% and.... nothing happen. Restartet the game and... nothing happen.

I Use 3840x2160 (16:9)

Whats wrong?

@Blackpearl1 Changing the GUI scale in the options screen should have an immediate effect. I need more information:

The most likely scenario is that you're using the WinAPI backend. If you didn't get a warning message when leaving the options screen, I made some mistake in determining whether the selected GUI scale is supported.

Blackpearl1 commented 10 months ago

@falbrechtskirchinger

Are you using the SDL2 video backend/driver? (WinAPI is not supported.)

That was my fault. I used the win...whatever

I changed it to SDL2 and now it works like a charm. Thank you :D nice job.

The Message that it wont work with the win...whatever comes after changing to SDL2...