Page MenuHomeWildfire Games

Target FPS in menus and running games
ClosedPublic

Authored by elexis on Jan 30 2017, 4:07 AM.

Details

Summary

Use case:
Since the renderer tries to display as many frames as possible, it will always induce 100% CPU workload on that core without benefit.
Not a direct concern of 0A.D., but it can also overheat hardware if throttling was disabled (as experienced by MojoRising420).

Implementation:
rP16927 / #2882 addressed this issue for main menus. But it is insufficient:

  1. It doesn't apply to running games.
  2. It doesn't allow the user to chose a target FPS.
  3. It renders about 20% too few FPS when picking a higher framerate
  4. It doesn't actually limit the FPS, because it only considers the time consumed by the most recently rendered frame, while the framerate fluctuates.

The inaccuracy in 3. derives from the frequency filter framerates (SmoothedFrequency, StableFrequency, AverageFrequency).
Instead, measuring the milliseconds since the last call gives a correct result.

An actual FPS limiter might be useful as well, as there can be screen tearing with Vsync disabled and input lag with Vsync enabled.
Never seen any of either though, likely due to the window manager doing Vsync already.

Test Plan

To test the inaccuracy described in 3., do not apply the patch, but change the 50 to 100 in main.cpp.
Replace realTimeSinceLastFrame by 1.0 / g_frequencyFilter->StableFrequency() to see that this won't help.
Add a new function AverageFrequency to frequency_filter.cpp / .h to see that this doesn't improve either.
Apply the patch and enjoy a proper rate.
Notice that removing the wait > 0 check is fatal, as negative values converted to u32 will become a huge number, effectively freezing the app.
Observe that with the patch applied, the target FPS is reached with much greater precision, even if exceeding often.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Jan 30 2017, 4:07 AM
elexis updated the Trac tickets for this revision.Jan 30 2017, 4:20 AM
Vulcan added a subscriber: Vulcan.Jan 30 2017, 4:51 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/271/ for more details.

echotangoecho added a subscriber: echotangoecho.EditedFeb 2 2017, 12:55 PM

Some observations:

  • We should be unable to set negative values for FPS (and probably also unable to set very low values).
  • The FPS limiting should probably be disabled if VSync is enabled, otherwise it might interfere, and the options should also be greyed out in this case.
  • This could be able to use #2593 / #4039 when that is in.
elexis added a comment.Feb 5 2017, 3:07 AM

We should be unable to set negative values for FPS (and probably also unable to set very low values).

We are able to type non-numerical strings, negative numbers and low numbers, since #4448 is not implemented.
But the code will just default to if a non-numerical value or number below 30 is given.
This must be the case as the user can also pass broken config values by editing the config file.

Furthermore typing 0 means disabling the option. If we force a value > 30 FPS, then we need to add another boolean option, which is also why a slider wouldn't be ideal, at least with the current option page.

The FPS limiting should probably be disabled if VSync is enabled, otherwise it might interfere, and the options should also be greyed out in this case.

How would it interfere? Why is that an issue?

echotangoecho edited edge metadata.Feb 11 2017, 9:22 PM

In case VSync is used, as far as I'm aware VSync itself determines the frame rate (and, to add onto that, in case VSync is enabled there is no reason to limit the framerate).

"Adaptive FPS" might be a better wording than "Target FPS"

It actually limit FPS, because we just waiting until time will be divided by FDT since last frame began. So the only except case: when fdt > limited_fdt.

Also if it needs to use smooth fps, you always could use a queue or something like this.

binaries/data/mods/public/gui/options/options.json
216 ↗(On Diff #389)

Isn't it too hard for newbie players to understand this option?

source/main.cpp
193 ↗(On Diff #389)

Why it so high, I could set menu in [24, 25] fps. I suggest to use 20-24 as lower bound.

196 ↗(On Diff #389)

We could use chrono, if it needs a higher precision.

vladislavbelov requested changes to this revision.Apr 2 2017, 9:08 PM
This revision now requires changes to proceed.Apr 2 2017, 9:08 PM
elexis marked 3 inline comments as done.May 1 2017, 6:00 AM

It actually limit FPS, because we just waiting until time will be divided by FDT since last frame began. So the only except case: when fdt > limited_fdt.

That iteration of the patch definitely didn't limit and had some bumps if the FPS changed ingame.
With the microsecond precision the difference isn't noticeable, so we can just call it a limiter, even if it isn't in all cases.

I've been waiting a long time to have a justification to post pornographic material (= reasoning why the ingame one is set to 60fps):
https://vimeo.com/197728547

  • This could be able to use #2593 / #4039 when that is in.

Done. Much saner, even if it can't display the range and currently selected value IMO

binaries/data/mods/public/gui/options/options.json
216 ↗(On Diff #389)

Yes, contains unneeded info

source/main.cpp
193 ↗(On Diff #389)

Set the lower bound to 20 for the engine

196 ↗(On Diff #389)

Came across that one when searching for something microsecond precise.
It seemed exotic, but since it does the job (much more stable FPS than with milliseconds) and since it's supported, lets go for it!

elexis updated this revision to Diff 1574.May 1 2017, 6:15 AM
elexis edited edge metadata.
elexis marked 3 inline comments as done.

Use chrono for microsecond precision and the new slider GUI object, rephrase strings.

Vulcan added a comment.May 1 2017, 8:31 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/950/ for more details.

elexis updated this revision to Diff 1593.May 2 2017, 3:32 AM

Last version uploaded didn't work because of two failures:

  • g_LastFrameTime must be set to the actual current time, not the time before the delay.
  • That 10^6 should have been 10^3.

Fix another failure found by Vladislav:

  • Don't use floats when we want double.

Also:

  • Remove Min/Max from the tooltip, as that will become appended generically.
Vulcan added a comment.May 2 2017, 4:18 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/961/ for more details.

elexis added a comment.May 2 2017, 1:02 PM

About the microsecond precision: If we have 200 fps, then there will be 5 millisecond delay between consecutive frames. The greatest error will be 0,5, so 10%, so I'd say 10% more precision sounds good

elexis marked 2 inline comments as done.May 2 2017, 4:48 PM
elexis added inline comments.
source/main.cpp
196 ↗(On Diff #1593)

we could also use int for fpsLimit and wait.
But using double for fpsLimit should avoid the int -> double conversion when dividing 1000.0 by this number.

elexis updated this revision to Diff 1595.May 2 2017, 4:52 PM

Use .0 for comparison to avoid another type conversion.

Vulcan added a comment.May 2 2017, 6:15 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/963/ for more details.

elexis updated this revision to Diff 1614.May 3 2017, 4:29 PM

25 FPS minimum in both main.cpp and options.js
Disable if VSync is enabled, as proposed by Vladislav and echotangoecho

Vulcan added a comment.May 3 2017, 8:21 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/977/ for more details.

elexis updated this revision to Diff 1619.May 4 2017, 12:46 AM

As reported by Vladislav:
Use same phrasing for slightly different options.json strings
20 instead of 25 min
Move the LimitFPS() call to the bottom of that function instead of doing it in the middle of the Frame()

Also:
Didn't I already claim to use doxygen?

elexis updated this revision to Diff 1620.May 4 2017, 1:01 AM

Use static for g_LastFrameTime and remove the g_, because it's not really going to be used elsewhere, as pointed out by Vladislav.

vladislavbelov accepted this revision.May 4 2017, 1:35 AM

I've tested it, it works for me, it has a small error enough: +/- 1 FPS.

This revision is now accepted and ready to land.May 4 2017, 1:35 AM
This revision was automatically updated to reflect the committed changes.
elexis added a comment.May 4 2017, 1:49 AM

Thanks for all the elaborate reviews and cpp explanations for dummies!

Vulcan added a comment.May 4 2017, 1:56 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/982/ for more details.

Vulcan added a comment.May 4 2017, 3:30 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/983/ for more details.