-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Fix Frame Rate higher than target on high refresh rate displays (>200Hz) #5847
Conversation
🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme 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.
Sorry for the delay in reviewing, and thanks for making these changes! There were a few typos that prevented it from building, but other than those it looks like it works as expected. I'm also going to look into unit tests and get back to you on whether its feasible to leave a test or two behind to prevent any regressions in the future.
src/core/main.js
Outdated
@@ -385,9 +387,11 @@ class p5 { | |||
//mandatory update values(matrixes and stack) | |||
this.redraw(); | |||
this._frameRate = 1000.0 / (now - this._lastFrameTime); | |||
this.deltaTime = now - this._lastFrameTime; | |||
this.deltaTime = now - _this._deltaFrame; |
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 think on this line and on 392, we need to use this
(without the _
prefix)
src/core/main.js
Outdated
this._setProperty('deltaTime', this.deltaTime); | ||
this._lastFrameTime = now; | ||
this._lastFrameTime = max.Math(_this._lastFrameTime |
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 believe this should also be Math.max
instead of max.Math
src/core/main.js
Outdated
@@ -358,6 +359,7 @@ class p5 { | |||
} | |||
|
|||
this._lastFrameTime = window.performance.now(); | |||
this._deltaFrame = window.performance.now(); |
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.
How do you feel about renaming _lastFrameTime
and _deltaFrame
to _lastTargetFrameTime
and _lastRealFrameTime
, respectively? It might make it clearer that they are both timestamps instead of differences between timestamps, and also that the former is the time we expected the frame to be drawn at, while the latter is the time it was actually called at.
Update on unit tests! There were a few issues with my original code snippet (in a comment in #5684, for anyone reading without context) and the version of sinon we use in tests. After some tinkering, this seems to work (I added it inside the existing suite('drawing with target frame rates', function() {
let clock;
let prevRequestAnimationFrame;
let nextFrameCallback = () => {};
let controlledP5;
setup(function() {
clock = sinon.useFakeTimers(0);
sinon.stub(window.performance, 'now', Date.now);
// Save the real requestAnimationFrame so we can restore it later
prevRequestAnimationFrame = window.requestAnimationFrame;
// Use a fake requestAnimationFrame that just stores a ref to the callback
// so that we can call it manually
window.requestAnimationFrame = function(cb) {
nextFrameCallback = cb;
};
return new Promise(function(resolve) {
controlledP5 = new p5(function(p) {
p.setup = function() {
p.createCanvas(10, 10);
p.frameRate(60);
p.loop();
resolve(p);
};
p.draw = function() {};
});
});
});
teardown(function() {
clock.restore();
window.performance.now.restore();
window.requestAnimationFrame = prevRequestAnimationFrame;
nextFrameCallback = function() {};
controlledP5.remove();
});
test('draw() is called at the correct frame rate given a faster display', function() {
sinon.spy(controlledP5, 'draw');
clock.tick(1000 / 200); // Simulate a 200Hz refresh rate
nextFrameCallback(); // trigger the next requestAnimationFrame
assert(controlledP5.draw.notCalled, 'draw() should not be called before 1s/60');
// Advance until 5ms before the next frame should render.
// This should be within p5's threshold for rendering the frame.
clock.tick(1000 / 60 - 1000 / 200 - 5);
nextFrameCallback(); // trigger the next requestAnimationFrame
assert(controlledP5.draw.calledOnce, 'one frame should have been drawn');
// deltaTime should reflect real elapsed time
assert.equal(controlledP5.deltaTime, 1000 / 60 - 5);
// Advance enough time forward to be 1s/60 - 5ms from the last draw
clock.tick(1000 / 60 - 5);
nextFrameCallback(); // trigger the next requestAnimationFrame
// Even though this is 1s/60 - 5ms from the last draw, the last frame came
// in early, so we still shouldn't draw
assert(controlledP5.draw.calledOnce, 'draw() should not be called before 1s/60 past the last target draw time');
// Advance enough time forward to be 1s/60 from the last draw
clock.tick(5);
nextFrameCallback();
assert(controlledP5.draw.calledTwice); // Now it should draw again!
// deltaTime should reflect real elapsed time
assert.equal(controlledP5.deltaTime, 1000 / 60);
});
}); |
Resolves #5684.
Changes:
Added a new property
_deltaFrame
to act the way_lastFrameTime
used to. Changed_lastFrameTime
so that it now cannot be updated by a smaller increment than would be desired by the target frame rate.In my manual testing, this kept the frame rate at the target and did not have adverse effects on
deltaTime
or when tabbing.PR Checklist
npm run lint
passes