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

Clock: Consider changing getElapsedTime behavior #30497

Open
AlaricBaraou opened this issue Feb 10, 2025 · 4 comments
Open

Clock: Consider changing getElapsedTime behavior #30497

AlaricBaraou opened this issue Feb 10, 2025 · 4 comments

Comments

@AlaricBaraou
Copy link
Contributor

AlaricBaraou commented Feb 10, 2025

Description

Over the years, I came across multiple projects that were calling .getElapsedTime() and messing up their deltas instead of reading the .elapsedTime property.

The core problem is that .getElapsedTime() does more than just "get" a value - it has the side effect of updating the internal state by calling .getDelta(). As much as the doc is relatively clear about it, users would typically expect a getter method to be pure and not modify state.

Here is a potential solutions to address this issue.

Solution

Rename the methods to better reflect their behavior ( example below )

    // Rename to make it clear this updates state, a bit of a mouth full but you get the idea
    updateAndGetElapsedTime() {  // formerly getElapsedTime()
        this.updateDelta();      // formerly getDelta()
        return this.elapsedTime;
    }

    getElapsedTime() {
        console.warn('getElapsedTime is deprecated, read the .elapsedTime property')
        return this.elapsedTime;
    }

    getDelta() {
        console.warn('getDelta is deprecated, call updateDelta instead')
    }

    // Rename to make it clear this updates state
    updateDelta() {              // formerly getDelta()
        let diff = 0;
        if (this.autoStart && !this.running) {
            this.start();
            return 0;
        }
        if (this.running) {
            const newTime = now();
            diff = (newTime - this.oldTime) / 1000;
            this.oldTime = newTime;
            this.elapsedTime += diff;
        }
        return diff;
    }

Alternatives

Maybe make the doc clearer, but I've seen this error made by advanced three.js developer so I'm not sure it would help.

Additional context

The issue / misconception being present in community project like Drei and React three fiber and Tres made me feel like it was time to talk about this,

pmndrs/react-three-fiber#3455
pmndrs/drei#2341
https://github.com/Tresjs/tres/pull/928/files

This is also misused in the official examples.
Here each call will call getDelta while a single getDelta followed by reading .elapsedTime would be more appropriate

capsule.position.y = Math.sin( clock.getElapsedTime() * 0.5 + capsule.position.x );
capsule.rotation.z = Math.sin( clock.getElapsedTime() * 0.5 ) * Math.PI * 1;

Same for these that while harmless because they are called immediately after, make unnecessary call to getDelta internally.
const delta = clock.getDelta();
const time = clock.getElapsedTime();

const delta = clock.getDelta();
const time = clock.getElapsedTime() * 10;

colorOffset.value += clock.getDelta() * colorRotationSpeed.value * timeScale.value;
const elapsedTime = clock.getElapsedTime();

Happy to eventually make a PR once the way to go is decided.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 10, 2025

Would it be an option for you to migrate to THREE.Timer instead?

Doc page: https://threejs.org/docs/index.html#examples/en/misc/Timer

It has been added as a new alternative to THREE.Clock that avoids the issues you have described in your post.

I'm personally not in favor of changing the API of THREE.Clock at this point since this will lead to a lot of confusion and migration tasks. THREE.Timer has been added as a separate module so THREE.Clock can be used as before. That said, it might make sense to deprecate THREE.Clock at some point.

@AlaricBaraou
Copy link
Contributor Author

This could be an option.

We'd have to extend it while deprecating the previous APIs to allow the usage of old methods like getElapsedTime while the users make the transition.

I guess my only issue is the mandatory _usePageVisibilityAPI. It should be possible to disable it with a parameter in the constructor to give the choice to use it or not in my opinion.
Even though I can imagine way to work around it.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 10, 2025

It should be possible to disable it with a parameter in the constructor to give the choice to use it or not in my opinion.

Making the usage optional is okay, imo. I vote to let the usage of the API enabled by default but an additional ctor parameter sounds good to me.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 10, 2025

As an additional note: In context of WebGPURenderer and TSL we use the TSL objects time and deltaTime now when we need time data in the shaders.

/**
* Represents the elapsed time in seconds.
*
* @tsl
* @type {UniformNode<float>}
*/
export const time = /*@__PURE__*/ uniform( 0 ).setGroup( renderGroup ).onRenderUpdate( ( frame ) => frame.time );
/**
* Represents the delta time in seconds.
*
* @tsl
* @type {UniformNode<float>}
*/
export const deltaTime = /*@__PURE__*/ uniform( 0 ).setGroup( renderGroup ).onRenderUpdate( ( frame ) => frame.deltaTime );

There is also the elapsed time that is passed as a parameter to the animation loop. That often makes the usage of a separate timer obsolete. That said, it's still important to have something like THREE.Timer or THREE.Clock around.

@gkjohnson gkjohnson changed the title Consider changing getElapsedTime behavior Clock: Consider changing getElapsedTime behavior Feb 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants