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

[OSX] Render screen in full native resolution on HiDPI displays. #8519

Merged
merged 3 commits into from Feb 14, 2021

Conversation

@michicc
Copy link
Member

@michicc michicc commented Jan 7, 2021

Motivation / Problem

On HiDPI displays, OTTD renders in virtual resolution, giving a permanent x2 scaling effect. This makes it impossible to e.g. get crisp text on HiDPI displays. OSX is the only platform that does this.

Description

Always use native resolution for rendering. x2 scaling can be achieved by using font/UI scaling and in-game zoom.

Limitations

Rendering in full native resolution can affect performance, there are more pixels after all. This PR adds an OSX only (non-GUI) setting to restore the old behaviour for cases where OTTD struggles to render in native resolution.

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')
@andythenorth
Copy link
Contributor

@andythenorth andythenorth commented Jan 7, 2021

First observation is zoom 😸
8519-zoom!

@LordAro
Copy link
Member

@LordAro LordAro commented Jan 8, 2021

have the climate icons always been that small?

@michicc
Copy link
Member Author

@michicc michicc commented Jan 8, 2021

@LordAro There's a bug with main menu GUI scaling, when you reduce the scaling factor form the settings menu. I've noticed it, but haven't done any digging yet.

@michicc michicc force-pushed the michicc:pr/osx_hidpi branch from 5665fdf to 0cbf1fb Jan 8, 2021
@nielsmh
Copy link
Contributor

@nielsmh nielsmh commented Feb 6, 2021

Is there a chance this could affect/be part of the cause for #7644 and #8300 ?

@michicc
Copy link
Member Author

@michicc michicc commented Feb 6, 2021

Unlikely. Test reports on IRC agreed on: slower, as expected. The slowdown did vary individually, of course.

@michicc michicc force-pushed the michicc:pr/osx_hidpi branch from 0cbf1fb to 90408d9 Feb 12, 2021
Copy link
Member

@TrueBrain TrueBrain left a comment

Codewise this looks fine by me; I understood people reported worse performance with this PR. Should we disable it by default for now? Or how do we want to deal with it .. as to me, it sounds like the right thing to do, to do support it. But we also have to overcome the performance reports somehow :)

@michicc
Copy link
Member Author

@michicc michicc commented Feb 13, 2021

OpenTTD has to generate four times the amount of pixels, so yes, a performance impact is entirely expected. The question (which has to be answered by users) is simply if the performance impact is worth it.

The added setting (settings are discoverable, driver params are not) is exactly because of that. I'm entirely open to defaulting to the current scaling behaviour.

@orudge
Copy link
Contributor

@orudge orudge commented Feb 13, 2021

I've tested it, and while there is a performance impact, I imagine it would be mostly noticeable if people are fast-forwarding. Depends on the complexity of the game of course. Quality looks good, especially with the native font rendering merged in too.

I'd be happy for it to go in, but would think it might be best to disable it by default.

@michicc
Copy link
Member Author

@michicc michicc commented Feb 13, 2021

Disabled by default is probably equal to nonexistent for most people. Precedent would be that we display in native resolution on Win32 as well.

The setting really is only for the cases where the performance impact is unacceptable IMHO.

@michicc michicc force-pushed the michicc:pr/osx_hidpi branch from 90408d9 to 10f442f Feb 13, 2021
@orudge
Copy link
Contributor

@orudge orudge commented Feb 14, 2021

Disabled by default is probably equal to nonexistent for most people. Precedent would be that we display in native resolution on Win32 as well.

I hadn’t spotted your other PR with support for autoscaling the UI elements at the time. With that included, then rendering in HiDPI by default seems good to me.

Copy link
Member

@TrueBrain TrueBrain left a comment

You are right @michicc ; making it false by default is pointless and we might as well thrown away the PR.

Mostly, this comes out of angst for more people complaining macOS is slow; but we have to deal with that sooner or later anyway, and hiding for that is not a solution either :D So I was wrong, and it was a stupid suggestion of mine :P Let's do this :D

@michicc
Copy link
Member Author

@michicc michicc commented Feb 14, 2021

Will update the auto-zoom PR after merging this.

@michicc michicc merged commit e5c3253 into OpenTTD:master Feb 14, 2021
10 checks passed
10 checks passed
@github-actions
Emscripten
Details
@github-actions
Commit checker
Details
@github-actions
Check preview needs update Check preview needs update
Details
@github-actions
Linux (clang, clang++)
Details
@github-actions
Linux (gcc, g++)
Details
@github-actions
Mac OS (x64, x86_64)
Details
@github-actions
Windows (windows-latest, x86)
Details
@github-actions
Windows (windows-latest, x64)
Details
@github-actions
Windows (windows-2016, x86) Windows (windows-2016, x86)
Details
@github-actions
Windows (windows-2016, x64) Windows (windows-2016, x64)
Details
@michicc michicc deleted the michicc:pr/osx_hidpi 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

6 participants