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

Add display refresh rate game option #8813

Merged
merged 1 commit into from Mar 9, 2021
Merged

Conversation

spnda
Copy link
Contributor

@spnda spnda commented Mar 6, 2021

Motivation / Problem

With the recent addition of the new OpenGL-based video drivers and detached render tick, I deemed it a good option to also include a option for display refresh rate. As personally I don't want to fiddle with refresh_rate inside the openttd.cfg file, like most other players, I thought this might be a good solution.

Description

This adds a new option called Display refresh rate next to Resolution in the Game Options window. Players can selected from various different refresh rates and change their refresh rate.

Limitations

Custom framerate configuration could be added in the future, but I think this is a very nieche use case.
Otherwise, only the drivers would be limiting us in any way, as some might not precisely give information about screens.

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')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Mar 6, 2021

The fact that there is no option yet in the GUIs, was a deliberate choice on my side. Currently the impact of increasing above 60fps is pretty noticeable in semi-large games. I am a bit afraid that this will give complaints about stuff that the user caused himself (after all, people will go: BIGGER NUMBERS ARE BETTER). I would like first to make sure the impact of increasing the refresh-rate isn't that bad anymore.

In my head, I had the following checklist to do first:

  1. get the gameloop-in-a-thread PR accepted (so increasing refresh-rate has less impact on the overall performance of the game)
  2. warn users (not sure how yet) when they are trying something like 240 Hz on a 4k resolution; the drawing will start to suppress game-ticks. Alternatively, we could make sure game-tick always gets priority (if not fast-forwarding)
  3. move mouse outside the game-state-lock, so we can always update the mouse, even if we had nothing else new to draw
  4. auto-detect refresh-rate, and warn users if they use a setting above their displays (warn, not disallow)
  5. (optionally) move more stuff outside the game-state-lock so draw-ticks can run more next to the game-tick.

So, my personal preference would be that we do not add this for 1.11, just so we have a bit more time to prepare for this, and kinda avoid getting reports about stuff users broke themselves. This would however be a solid start for 1.12 to continue this path we are on.
If we do want this for 1.11, I would suggest the >60fps are marked clearly with a statement it could impact performance and should be used with care.

No clue how others stand in this, hence me writing down my reasoning :D

PR wise: awesome work, tnx :D

src/settings_gui.cpp Outdated Show resolved Hide resolved
src/settings_gui.cpp Outdated Show resolved Hide resolved
@spnda
Copy link
Contributor Author

@spnda spnda commented Mar 8, 2021

This new version gets the refresh rate of all monitors and makes them available in the dropdown menu. (Only added for win32, SDL2).
image
(My second monitor is 72Hz)
Note: With the SDL implementation my second monitor is shown as 60Hz by SDL2's SDL_GetDisplayMode, eventhough its 72Hz and Win32 does report correctly. Possibly a SDL2 bug.

Also added a warning that refresh rates above 60Hz might impact performance and might differ from the actual result.
image

@spnda spnda force-pushed the videosettings branch 4 times, most recently from e7b16ac to 6b60c3b Compare Mar 8, 2021
@spnda
Copy link
Contributor Author

@spnda spnda commented Mar 8, 2021

Updated for compatibility with #8819
image

@spnda spnda force-pushed the videosettings branch 4 times, most recently from 4dde02b to 91fb689 Compare Mar 8, 2021
@spnda
Copy link
Contributor Author

@spnda spnda commented Mar 8, 2021

Reorganized the graphics options in the game options. Also moves the "Hardware Acceleration" to the refresh rate section.
image

@spnda spnda force-pushed the videosettings branch 2 times, most recently from 3653bec to df305f0 Compare Mar 8, 2021
@spnda
Copy link
Contributor Author

@spnda spnda commented Mar 8, 2021

New layout for the graphics section. Possibly a table could be useful?
image

Copy link
Member

@TrueBrain TrueBrain left a comment

I really like how you addressed my concerns here. I am okay with this, even for 1.11, because of the way you implemented it.

Code-wise, a few things, but most are coding style. I do need a C++ person to look at it, at I am not sure this is the proper way to do some parts .. but after you fixed my comments, we will get someone to look at that :)

src/lang/english.txt 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/settings_gui.cpp Outdated Show resolved Hide resolved
src/settings_gui.cpp Outdated Show resolved Hide resolved
src/video/sdl2_v.cpp Outdated Show resolved Hide resolved
src/settings_gui.cpp Outdated Show resolved Hide resolved
@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Mar 8, 2021

I have a lovely bug: 0Hz is in the list :D Selecting it crashes the game :P Might want to filter those in detection :) (triggered on SDL, via WSL2)

@spnda
Copy link
Contributor Author

@spnda spnda commented Mar 8, 2021

Oh no, that was one of the bugs I encountered when testing on SDL2 but it only sometimes happened. Will do.

src/settings_gui.cpp Outdated Show resolved Hide resolved
@TrueBrain TrueBrain added this to the 1.11.0 milestone Mar 8, 2021
src/video/win32_v.cpp Outdated Show resolved Hide resolved
src/video/win32_v.cpp Outdated Show resolved Hide resolved
src/video/win32_v.cpp Outdated Show resolved Hide resolved
src/video/video_driver.hpp Outdated Show resolved Hide resolved
src/video/sdl2_v.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/settings_gui.cpp Outdated Show resolved Hide resolved
src/settings_gui.cpp Outdated Show resolved Hide resolved
@spnda spnda force-pushed the videosettings branch 2 times, most recently from 0845dca to 558b329 Compare Mar 8, 2021
src/lang/english.txt 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/settings_gui.cpp Outdated Show resolved Hide resolved
src/settings_gui.cpp Outdated Show resolved Hide resolved
Copy link
Member

@TrueBrain TrueBrain left a comment

Sorry, a few more. Should have spotted them the first time :(

src/video/win32_v.cpp Outdated Show resolved Hide resolved
src/video/win32_v.cpp Outdated Show resolved Hide resolved
src/video/win32_v.cpp Show resolved Hide resolved
src/video/win32_v.cpp Show resolved Hide resolved
src/video/sdl2_v.cpp Outdated Show resolved Hide resolved
src/settings_gui.cpp Outdated Show resolved Hide resolved
src/settings_gui.cpp Outdated Show resolved Hide resolved
@spnda spnda force-pushed the videosettings branch 2 times, most recently from ab68e2c to 680736e Compare Mar 8, 2021
src/lang/english.txt 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
@spnda spnda force-pushed the videosettings branch 2 times, most recently from ab5a3bc to 9c82ecd Compare Mar 8, 2021
Copy link
Member

@LordAro LordAro left a comment

getting nitpickier :)

src/lang/english.txt Outdated Show resolved Hide resolved
src/settings_gui.cpp Show resolved Hide resolved
src/settings_gui.cpp Outdated Show resolved Hide resolved
src/settings_gui.cpp Outdated Show resolved Hide resolved
LordAro
LordAro approved these changes Mar 9, 2021
Copy link
Member

@LordAro LordAro left a comment

No further issues :)

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Mar 9, 2021

Again, tnx for addressing my concerns @spnda , that is really appreciated!

@TrueBrain TrueBrain merged commit 0464a50 into OpenTTD:master Mar 9, 2021
12 checks passed
@spnda spnda deleted the videosettings branch Mar 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants