mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
102.86k stars 35.39k forks source link

Animation jump when timeScale changes sign in the first loop when using LoopPingPong mode #19151

Open SuperPoneyBoy opened 4 years ago

SuperPoneyBoy commented 4 years ago

Hi I'm testing this lovely library and i've found a bug.

I have an animation in a pingpong way. When i click on it, the mixer.timeScale is switched between 1 and -1. In the first loop, at the first click, the animation jumps straight to the end and start going in reverse mode. The others loops are fine.

Looking at the code, it seems to be coming from there :

https://github.com/mrdoob/three.js/blob/eb8639016466f0066714f7c1c0c67a59898979e0/src/animation/AnimationAction.js#L545-L567

if ( deltaTime >= 0 ) Should be changed because if delta is negative, then it jumps to the end of animation instead of goinf in reverse mode! if ( deltaTime * timeDirection >= 0 )

Here is the full stack to change the caller : https://github.com/mrdoob/three.js/blob/eb8639016466f0066714f7c1c0c67a59898979e0/src/animation/AnimationAction.js#L330 https://github.com/mrdoob/three.js/blob/eb8639016466f0066714f7c1c0c67a59898979e0/src/animation/AnimationAction.js#L366

Adding timeDirection to https://github.com/mrdoob/three.js/blob/eb8639016466f0066714f7c1c0c67a59898979e0/src/animation/AnimationAction.js#L487

Mugen87 commented 4 years ago

I'm not sure I understand the issue. Using THREE.LoopPingPong seems to work fine:

https://jsfiddle.net/xedtoLmy/1/

Do you mind modifying the fiddle in order to demonstrate the bug?

SuperPoneyBoy commented 4 years ago

I've updated the code : https://jsfiddle.net/sjc3ao4m/ Click on the window to change the mixer.timeScale before a loop is done and after !

import * as THREE from "https://threejs.org/build/three.module.js";

let renderer, scene, camera, mixer, clock;

init();
animate();

function init() {

    clock = new THREE.Clock();

    renderer = new THREE.WebGLRenderer();
    renderer.setSize(window.innerWidth, window.innerHeight);
    renderer.setPixelRatio(window.devicePixelRatio);

    document.body.appendChild(renderer.domElement);

    scene = new THREE.Scene();

    camera = new THREE.PerspectiveCamera(40, window.innerWidth / window.innerHeight, 0.1, 10);
    camera.position.set(0, 0, 5);

    const geometry = new THREE.BoxBufferGeometry();
    const material = new THREE.MeshBasicMaterial();

    const mesh = new THREE.Mesh(geometry, material);
    scene.add(mesh);

    mixer = new THREE.AnimationMixer(mesh);
    document.addEventListener("click", async (e) => {
        //Until the first loop is not finished it will jump straigth to end or to the start of animation
        //Then it will just change direction.
        mixer.timeScale *= -1;
    });

    const keyframeTrack = new THREE.VectorKeyframeTrack('.position', [0, 3], [0, 0, 0, 1, 0, 0]);
    const clip = new THREE.AnimationClip('default', - 1, [keyframeTrack]);

    const clipAction = mixer.clipAction(clip);
    clipAction.setLoop(THREE.LoopPingPong);
    clipAction.play();

}

function animate() {

    requestAnimationFrame(animate);

    mixer.update(clock.getDelta());

    renderer.render(scene, camera);

}
Mugen87 commented 4 years ago

if ( deltaTime * timeDirection >= 0 )

I don't think this modification is correct. Consider this fiddle: https://jsfiddle.net/m4th2fsc/

AnimationMixer.timeScale is by default -1. If you apply your patch, it will move from left to right (although it should move right to left like in the fiddle).

The change does also not work if you modify AnimationAction.timeScale.

However, I can confirm that setting the time scale to a negative value in the first run of an action is not correct. This happens only for repetitive actions (using THREE.LoopRepeat or THREE.LoopPingPong).

SuperPoneyBoy commented 4 years ago

I think that in a pingpong reversed, time should start at original point and do the same animation form the same starting point. For example, in the case of timeScale =1 the pingpong does : t0 -> t1 -> t2 -> t3 -> t2 -> t1 -> t0 In the case of timeScale =-1, the reversed time should be the same because the point before t0 is t1 and then is t2 ...

In the case of a LoopRepeat, i'm totaly agree because : t0->t1->t2->t3->t0->t1->t2->t3 In the case of timeScale =-1, the point before t0 is t3 then t2 ...

According to what i said above, the proposed patch seems to work well in both cases (LoopRepeat and LoopPingPong). By the way i dont see any problem with the THREE.LoopRepeat, it works fine when changing timeScale to -1 at the first loop.

Mugen87 commented 4 years ago

I think that in a pingpong reversed, time should start at original point and do the same animation form the same starting point.

Um, this really depends on the definition. Users could rely on the current behavior and your change would break it.

The change does also not work if you modify AnimationAction.timeScale.

Can you please respond on this point? It would be also valid to modulate AnimationAction.timeScale which seems not to work so far according to your definitions.