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

Timer: Add optional timestamp parameter. #27421

Merged
merged 1 commit into from
Dec 22, 2023

Conversation

vanruesc
Copy link
Contributor

Related PR: #17912

Description

This PR continues the discussion from #17912 (comment).

I've added an optional timestamp parameter to Timer.update() that allows the user to pass the same timestamp to multiple timers. This also avoids unnecessary calls to performance.now() when using requestAnimationFrame because the callback already receives a timestamp argument.

@Mugen87 previously raised some concerns regarding this parameter:

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).

We can clarify in the docs that using a fixedDelta overrides dynamic time deltas. I'm not sure if the timestamp parameter would somehow interfere with _startTime. Shouldn't this just work as if performance.now() was used?

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 22, 2023

Shouldn't this just work as if performance.now() was used?

This only works if the value of performance.now() and the value of the timestamp parameter are actually compatible to each other. Since we can't control what values users pass into update(), I was afraid of unexpected side effects.

However, the timestamp value you get from requestAnimationFrame() works in any case and I think this is the most common use case. The documentation is also clarified so I'm okay with this change.

@Mugen87 Mugen87 added this to the r160 milestone Dec 22, 2023
@mrdoob mrdoob merged commit e049f9c into mrdoob:dev Dec 22, 2023
11 checks passed
@Mugen87 Mugen87 changed the title Add optional timestamp parameter TImer: Add optional timestamp parameter. Dec 22, 2023
@Mugen87 Mugen87 changed the title TImer: Add optional timestamp parameter. Timer: Add optional timestamp parameter. Dec 22, 2023
@vanruesc vanruesc deleted the feat/timer-timestamp branch December 22, 2023 14:33
AdaRoseCannon pushed a commit to AdaRoseCannon/three.js that referenced this pull request Jan 15, 2024
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.

3 participants