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.

Sign up for GitHub

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

Jump to bottom

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
+173 −28

Conversation

spnda
Copy link
Contributor

@spnda spnda commented Mar 6, 2021 •
edited
Loading

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 •
edited
Loading

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

TrueBrain
TrueBrain reviewed Mar 6, 2021
View reviewed changes
@spnda
Copy link
Contributor Author

spnda commented Mar 8, 2021 •
edited
Loading

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 •
edited
Loading

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

TrueBrain
TrueBrain reviewed Mar 8, 2021
View reviewed changes
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 :)

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

TrueBrain
TrueBrain reviewed Mar 8, 2021
View reviewed changes
@TrueBrain TrueBrain added this to the 1.11.0 milestone Mar 8, 2021
LordAro
LordAro requested changes Mar 8, 2021
View reviewed changes
LordAro
LordAro requested changes Mar 8, 2021
View reviewed changes
TrueBrain
TrueBrain reviewed Mar 8, 2021
View reviewed changes
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 :(

Sorry, something went wrong.

LordAro
LordAro reviewed Mar 8, 2021
View reviewed changes
TrueBrain
TrueBrain reviewed Mar 8, 2021
View reviewed changes
LordAro
LordAro reviewed Mar 8, 2021
View reviewed changes
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 :)

michicc
michicc reviewed Mar 8, 2021
View reviewed changes
LordAro
LordAro approved these changes Mar 9, 2021
View reviewed changes
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
@spnda spnda mentioned this pull request Mar 10, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michicc michicc

@TrueBrain TrueBrain

@LordAro LordAro

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.11.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@spnda @TrueBrain @michicc @LordAro