Replace frame limiter

[theyareonit]

Fixes #2610 by removing the possibility for long-term desync between the player's FPS cap and their actual framerate.

I'm not 100% sure about the things I mentioned in the comments about GLFW.glfwWaitEventsTimeout and Thread.sleep. I would appreciate some input from anyone who might know more on those subjects.

[douira]

I've done some tests to observe the behavior of this patch:

reference commit before this patch with targets of 80 and 150. It can reach 80, but not 150:

this patch with targets 80 an 150:

With the patch, it much more accurately holds a frame rate that it can actually produce. However, if it can't produce the frames fast enough, with the patch it will have regularly spaced spikes. I don't know if these spikes are noticeable or impact the experience, but I wanted to point this out.

[theyareonit]

One of the things this patch does is that it fully drops a frame if it can't come on time, to ensure that frames always stay perfectly in sync. This should be fine if your FPS limit is a multiple of your monitor's refresh rate, but it might make the game less smooth if you're running a strange FPS limit (for this reason, a patch that would allow the user to set their FPS limit to any value might help).

But yeah, it might be worth trying other approaches to synchronization. I also experimented with a version that essentially shifts to unlimited FPS whenever you lag to reduce dropped frames, but it just felt bad to play on IMO, probably because most of those extra frames weren't actually doing anything.

[douira]

I've added this to the upcoming minor milestone for visibility, but the concept and implementation need more work. We have reports that it regresses in terms of jitter. The original author but also everyone else is invited to test and contribute.

[redactedontop]

Why not set it as a draft then?

[douira]

Because I didn't know you could. Thanks for pointing this out so kindly

[decce6]

In the original issue the proposed solution was to use lastDrawTime = target, but in this PR the lastDrawTime field seems entirely unused, and the sleep time is instead calculated from solely the current time. Why is this change? Doesn't this cause frames to be dropped when one frame took too long?

[theyareonit]

The motivation behind this change was that, with lastDrawTime = target, the game essentially switches to unlimited FPS whenever you lag in an effort to catch up (since lastDrawTime will be far in the past). This creates problems if you have a big stutter, since the game might switch to unlimited FPS for multiple seconds, which somewhat defeats the point of using a capped framerate.

There are a few ways to address this issue. You could, for instance:

• Not let lastDrawTime get too far behind (e.g. ensure it's only 5 frames behind at most)
• Always drop frames when you get out of sync instead of trying to catch up
• Use lastDrawTime = now like the vanilla game does, but only when you get a large stutter

I chose option 2 because option 1 and option 3 both cause temporary or permanent desync from theoretically perfect frame timing, which might cause issues like having your tearline jump around the screen, or less predictability with inputs. And I also wasn't sure that switching to unlimited FPS after a minor stutter really provided a meaningful benefit in terms of smoothness, compared to capped FPS that never gets out of sync. But I might have been wrong about this, and others could do their own testing here.

edit:

I suppose option 2 is really just option 1 but with the max catchup window set to 0 frames. So maybe the number of frames behind it can get could just be an ingame setting with the default at like 2-5? Not sure.

I think on a personal level something like option 1 but with a catchup window of only 1 frame sounds like the best experience to me thinking about it now, but setting it higher by default is probably safe enough.

[decce6]

I think on a personal level something like option 1 but with a catchup window of only 1 frame sounds like the best experience to me thinking about it now, but setting it higher by default is probably safe enough.

I agree this seems the best solution.

douira said on discord adding an in-game option seems ok.

[decce6]

No sleep is performed here when sleep time is smaller than 3ms. While I'm not familiar with macOS, this at least shouldn't be necessary on Windows and Linux, both of which have 1ms timer resolution - only subtract 1ms (or something like 1.1ms to be safer) from waitTime to allow the render thread to sleep longer.

[decce6]

Using glfwWaitEventsTimeout is not ideal:

• It is not a sleep function. It may wake up whenever there is new event from the operating system. In my testing, it woke up around 20 times each frame at 60FPS (with no user input), literally taking away the potential power saving benefits of limiting the FPS
• It does not, as one might imagine, use a high resolution timer (Win32). As such, it'd be better to prefer Java's built-in sleep methods
  • Thread#sleep, however, should be avoided: it repeats waiting until it believes (as specified by System#nanoTime) the elapsed time is longer than or equal to the requested sleep time. Due to the precision of System#nanoTime. it could frequently cause additional sleeps, requiring extra scheduler periods to finish
  • As an alternative, LockSupport#parkNanos could be used

The spin wait when waitTime <1ms could be (theoretically) improved with Thread#onSpinWait.

[decce6]

Could adapt RenderSystemMixin to skip both polls when FPS limiter is active.

[umbrageodotus]

Sorry @douira , didn't mean to to be rude.

[decce6]

The framerate limiter has been rewritten in 26.1-snapshot-11 (net.minecraft.client.FramerateLimiter), making use of LockSupport.parkNanos and Thread.onSpinWait. May be worth looking at its implementation as well as whether this PR is still relevant.

[douira]

That's good to hear. If the vanilla implementation solves (nearly) all of the issues with the frame limiter we could consider backporting the concepts if we decide to do backports. We could also consider mixing into it on 26.1 if we find any remaining issues with it.

[douira]

I want to point out recent other work that uses glfwWaitEventsTimeout, parkNanos, and onSpinWait. It remains to be seen whether this PR is still relevant given Minecraft's own changes and this other mod.

[decce6]

Adding to this framerate limiter changes in 26.1:

• As previously mentioned, LockSupport.parkNanos is used for sleeping. The game calculate the discrepancies ("overshoot") of this function over multiple frames, and uses this data to decide the actual sleep time to be used
• When the sleep time is low enough (also impacted by the overshoot time), Thread.onSpinWait is used when busy waiting

The sleep logic itself seems fine enough and should not need any further modification. However, I believe the concept you illustrated in #2610 still applies. The previous unresolved review threads may be discarded if we change to target 26.1 as we shouldn't need to change the sleep logic any further.