-
Notifications
You must be signed in to change notification settings - Fork 917
Replace frame limiter #2618
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
base: dev
Are you sure you want to change the base?
Replace frame limiter #2618
Changes from all commits
2534a8a
824c332
32fc495
c9a6654
7946060
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| package net.caffeinemc.mods.sodium.mixin.features.render.sync; | ||
|
|
||
| import com.mojang.blaze3d.systems.RenderSystem; | ||
| import org.lwjgl.glfw.GLFW; | ||
| import org.spongepowered.asm.mixin.Mixin; | ||
| import org.spongepowered.asm.mixin.Overwrite; | ||
|
|
||
| @Mixin(value = RenderSystem.class, remap = false) | ||
| public class RenderSystemMixin { | ||
| /** | ||
| * @author theyareonit | ||
| * @reason Improve frame synchronization | ||
| */ | ||
| @Overwrite | ||
| public static void limitDisplayFPS(int fps) { | ||
| double frameTime = 1.0 / fps; | ||
| double now = GLFW.glfwGetTime(); | ||
| double end = (now - (now % frameTime)) + frameTime; // subtracting (now % frameTime) corrects for desync | ||
|
|
||
| for (; now < end; now = GLFW.glfwGetTime()) { | ||
| double waitTime = (end - now) - 0.002; // -2ms to account for sleep imprecision on some operating systems | ||
| if (waitTime >= 0.001) { // cant sleep less than 1ms without platform-specific code | ||
|
Comment on lines
+21
to
+22
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| GLFW.glfwWaitEventsTimeout(waitTime); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using
The spin wait when waitTime <1ms could be (theoretically) improved with |
||
| } | ||
| } | ||
|
|
||
| GLFW.glfwPollEvents(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could adapt RenderSystemMixin to skip both polls when FPS limiter is active. |
||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the original issue the proposed solution was to use
lastDrawTime = target, but in this PR thelastDrawTimefield 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?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
lastDrawTime = nowlike the vanilla game does, but only when you get a large stutterI 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this seems the best solution.
douira said on discord adding an in-game option seems ok.