three.js
three.js copied to clipboard
mrdoob
Added THREE.Timer.
Related #17679
This is a new alternative to THREE.Clock with a different API design and behavior. It should make it easier to implement three.js apps FPS independent which becomes more and more important with faster refresh rates.
Background: Some time ago, I've realized on a 144 Hz gaming laptop that (at least in Chrome and Firefox) WebGL apps run with 144 FPS. Some three.js examples now look weird because they assume fix update steps (around 60 FPS). Since certain laptops already offer 240 Hz screens, I think we should reconsider how a time class works. The main differences of THREE.Timer compared to THREE.Clock are:
-
THREE.Timerhas an update method that updates its internal state. - That makes it possible to call
.getDeltaTime()and.getElapsedTime()multiple times per simulation step without getting different values. - The class uses the Page Visibility API to avoid large time delta values when the app is inactive (e.g. tab switched or browser hidden).
- It's possible to configure a fixed time delta and a time scale value (similar to Unity's
Timeinterface). - No public properties, no ctor parameter. The public API only consists of methods.
THREE.Timer is located in examples/jsm/misc/ for now. I personally think THREE.Timer (or a similar class) should replace THREE.Clock at some point since the class has some conceptual design flaws. The example webgl_morphtargets_sphere already uses THREE.Timer in order to replace the existing FPS workaround.
The method names are quite descriptive now. If e.g. getDelta() is preferred over getDeltaTime(), I'm happy to shorten the names.
I like it! Just a question, what is the expected use of now() if fixedTime is enabled? maybe is safer to return elapsedTime in that case? as it could be inconsistent to use now() together with the elapsedTime provided by the fixed steps.
Just a question, what is the expected use of now() if fixedTime is enabled?
Um, it's probably better to make now() private in order to avoid this confusion. The method should just return a time stamp which is used to compute the delta time when no fixed deltas are used.
Timer.now() should not be affected by timeScale. If elapsedTime is used in now(), that would be the case.
If getDelta() is preferred over getDeltaTime(), I'm happy to shorten the names.
I think getDelta() is fine - in the context of a timer, it's very clear that we are talking about time deltas.
It's actually useful for testing and debugging if a timer can produce deterministic values.
I think getDelta() is fine - in the context of a timer, it's very clear that we are talking about time deltas.
Okay, I've shorten the method names and variables.
Nice.
Is there a reason you've chosen to add a separate "update()" call?
I.e. why not update the timer when "getDelta()" is called like the current Clock?
I.e. why not update the timer when "getDelta()" is called like the current Clock?
Because many users complained about the fact that multiple calls of getDelta() within a single frame produces wrong results. getElapsedTime() has similar issues, see #5696.
To avoid these design problems, the state of the timer is now explicitly updated by calling update(). Yes, it is an additional method call but it avoids confusing behaviors of the current Clock class.
Should also the Timer class have the serialization/deserialization methods? They would be nice to have.
So that it would be possible to "save" and then "restore" the state of the timer (except ._previousTime and ._currentTime).
Sserialization/deserialization is only added to core classes which are part of the scene graph. Clock and Timer are not added to the scene.
@LeviPesin TBH, I don't think it's a good idea to post at PRs asking for when are they getting merged or if merge conflicts can be resolved. PRs are paused/stalled for various reasons. Sometimes their general approach isn't right, sometimes collaborators are not happy with certain parts or have a different solutions in mind. However, not every collaborator has the time or opportunity to explain their concerns in detail because other tasks are more important.
Looking at the past, many stalled PR were merged when the team was confident enough with the respective design decision.
we started to add it to three-stdlib, i noticed that the class has constructor side effects, for reference: https://github.com/mrdoob/three.js/issues/20575
i've added a connect method that pairs with dispose https://github.com/pmndrs/three-stdlib/blob/main/src/misc/Timer.ts that would also make the object pausable/re-usable.
I've also been using a modified Timer class in @pmndrs/postprocessing which could serve as feedback for the API design.
This version uses getter properties instead of methods and has an autoReset setting that controls usage of the page visibility API. The update method also has an optional timestamp parameter which can be obtained from the requestAnimationFrame callback to avoid unnecessary calls to performance.now:
requestAnimationFrame(function render(timestamp: number): void {
requestAnimationFrame(render);
timer.update(timestamp);
});
The update method also has an optional timestamp parameter which can be obtained from the requestAnimationFrame callback to avoid unnecessary calls to performance.now:
That is a good idea! I'll add it to the PR.
I think there may be a bug in _now() function.
let timer = new Timer();
// Let it run to for example 10 seconds.
timer = null;
// Wait 10 seconds.
timer = new Timer();
timer.getElapsed(); // returns 0 (as expected)
timer.update();
timer.getElapsed(); // returns 20, so there is a sudden gap of 20 seconds (unexpected)
Shouldn't it count from 0 after the second initialization?
For reproduction: https://jsfiddle.net/3kejsft2/
Indeed! I guess I have designed Timer without the assumption somebody wants to creation multiple instances of it.
Can you please verify if this version of Timer fixes your issue? https://jsfiddle.net/uje7xw2r/
The idea is to save the start time value of the timer and then just subtract it from the timestamp.
Thank you!
It seems to work now.
(There's also a minor inconsistency that if ( document.hidden === false ) this.reset(); is lacking curly braces but all other if statements have them (one-liners and multiple-liners).)
would you mind elaborate that how the Timer solves this "design flaw" of Clock https://github.com/mrdoob/three.js/pull/26806#issuecomment-1727706775? any help is appreciated.
I've seen projects in the past copying this code or use an adapted version of it so I think it's better to provide an alternative to THREE.Clock in three.js.
The update method also has an optional timestamp parameter which can be obtained from the requestAnimationFrame callback to avoid unnecessary calls to performance.now:
That is a good idea! I'll add it to the PR.
Just curious: why was this removed in d443665? :thinking:
I personally think THREE.Timer (or a similar class) should replace THREE.Clock at some point
How about adding it to core now, with documentation? Then, deprecate THREE.Clock eventually.
If you don't want to do that now, how about adding the above comprehensive description to the header of Timer.js?
Just curious: why was this removed in https://github.com/mrdoob/three.js/commit/d443665a59479f728f7de41ac58a9dd4254a4c91? 🤔
I was unsure about this feature since the parameter has no effect when a fixed time delta is used. Besides, I couldn't decide how the parameter should play together with _startTime (which was added to support multiple timers).
That said, not including a timestamp parameter in this PR does not mean we shouldn't support it. I just had the feeling this needs more discussion. A separate PR adding timestamp would be fine!
If you don't want to do that now, how about adding the above comprehensive description to the header of Timer.js?
That sounds good to me!
If you don't want to do that now, how about adding the above comprehensive description to the header of Timer.js?
That sounds good to me!
Done! 46b6bb0ab34427b36bc25c4747db2bcdb88905bf
For r161, I'll try to add a documentation page.
Hmmm... I'm not sure about the API for fixedTime. I'll open a new PR for it... 🤔