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

Add display refresh rate game option #8813

merged 1 commit into from Mar 9, 2021

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

spnda commented Mar 8, 2021

Updated for compatibility with #8819
image

@spnda
Copy link
Contributor Author

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

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

Choose a reason for hiding this comment

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

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

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 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
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

Choose a reason for hiding this comment

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

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

@LordAro LordAro left a comment

Choose a reason for hiding this comment

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

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

@LordAro LordAro left a comment

Choose a reason for hiding this comment

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

No further issues :)

@TrueBrain
Copy link
Member

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

@TrueBrain TrueBrain merged commit 0464a50 into OpenTTD:master Mar 9, 2021
@spnda spnda deleted the videosettings branch March 9, 2021 10:10
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