Skip to content
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

Added THREE.Timer. #17912

Merged
merged 18 commits into from
Dec 16, 2023
Merged

Added THREE.Timer. #17912

merged 18 commits into from
Dec 16, 2023

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Nov 12, 2019

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.Timer has 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 Time interface).
  • 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.

@fernandojsg
Copy link
Collaborator

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.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Nov 13, 2019

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.

@Mugen87 Mugen87 added the Design label Nov 13, 2019
@fernandojsg
Copy link
Collaborator

So in that case I agree on making now() private

examples/js/misc/Timer.js Outdated Show resolved Hide resolved
@looeee
Copy link
Collaborator

looeee commented Nov 13, 2019

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.

@looeee
Copy link
Collaborator

looeee commented Nov 13, 2019

In what situations would you want to use fixedDeltaTime?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Nov 13, 2019

It's actually useful for testing and debugging if a timer can produce deterministic values.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Nov 13, 2019

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.

@CreaticDD
Copy link
Contributor

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?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 10, 2020

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.

@mrdoob mrdoob modified the milestones: rXXX, r117 May 8, 2020
@mrdoob mrdoob modified the milestones: r117, r118 May 27, 2020
@mrdoob mrdoob modified the milestones: r118, r119 Jun 24, 2020
@mrdoob mrdoob modified the milestones: r119, r120 Jul 29, 2020
@mrdoob mrdoob added this to the r156 milestone Jul 27, 2023
@mrdoob mrdoob modified the milestones: r156, r157 Aug 31, 2023
@ycw
Copy link
Contributor

ycw commented Sep 21, 2023

would you mind elaborate that how the Timer solves this "design flaw" of Clock #26806 (comment)? any help is appreciated.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 21, 2023

Updating the timer per frame before querying data is the intended usage.

@mrdoob mrdoob modified the milestones: r158, r159 Oct 27, 2023
@mrdoob mrdoob modified the milestones: r159, r160 Nov 30, 2023
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 16, 2023

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.

@Mugen87 Mugen87 merged commit c71a2d8 into mrdoob:dev Dec 16, 2023
18 checks passed
@Mugen87 Mugen87 removed the Design label Dec 16, 2023
@vanruesc
Copy link
Contributor

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? 🤔

@WestLangley
Copy link
Collaborator

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?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 16, 2023

Just curious: why was this removed in d443665? 🤔

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!

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 17, 2023

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! 46b6bb0

For r161, I'll try to add a documentation page.

@mrdoob
Copy link
Owner

mrdoob commented Dec 21, 2023

Sorry for the huge delay dealing with this... 🙏

@mrdoob
Copy link
Owner

mrdoob commented Dec 21, 2023

Hmmm... I'm not sure about the API for fixedTime.
I'll open a new PR for it... 🤔

@mrdoob mrdoob mentioned this pull request Dec 22, 2023
AdaRoseCannon pushed a commit to AdaRoseCannon/three.js that referenced this pull request Jan 15, 2024
* Added THREE.Timer.

* Timer: Make now() private.

* Timer: Fix typo.

* Timer: Shorten method and variable names.

* Timer: Remove js and add d.ts file.

* Timer: Transform to class.

* Update webgl_morphtargets_sphere.html

* Delete Timer.d.ts

* Update webgl_morphtargets_sphere.html

* Timer: Add optional timestamp parameter to update().

* Timer: Fix multiple instance usage.

* Timer: Clean up.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.