mrdoob / three.js

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

Time warping bug in AnimationAction.crossFadeTo() and AnimationAction.crossFadeFrom() #11147

Open jostschmithals opened 7 years ago

jostschmithals commented 7 years ago

After crossfading with time warping the timeScale value of the start action is not reset.

To reproduce the problem try this example which is using this animation code.

If you make a crossfade from “run” to “idle” first, wait until it is finished, and then play “run” again, running will always happen in slow motion (this would even be the case if you would make a runAction.reset().play). The display shows that the "run" action's timeScale is 0.18 instead of 1.0.

The time warping of “run” should work like the green line in the following screenshot (which is symmetric to the blue “idle” line). But since the reset of the “run” action's timeScale is missing, it ends with the red line: graph timescale So you have to reset the effective timeScale to the former value manually in each place of an application where (dependent on user inputs) a crossfading with time warping might have happened before.

I can't imagine that this is the intended behaviour(?)


BTW: I noticed one other issue with crossfading, which is at least inconvenient:

Before you can make a crossfade you always have to manually set the effective weight of the end action to 1. Wouldn’t this better be the default in the crossfade methods, which use this internally?

RemusMar commented 4 years ago

@mrdoob Ricardo, the bug is still there. Example: https://necromanthus.com/Test/html5/dog_gltf.html There are 3 animations and with the mouse click you switch and cross fade between them. The first two transitions run ok. anim1 ---> anim2 anim2 ---> anim3 But the next ones don't work (the animations are stopped from the very beginning). anim3 ---> anim1 anim1 ---> anim2 ... etc

The workaround is to STOP the animation before crossFadeFrom:

anim2.stop();
anim2.crossFadeFrom( anim1, 0.5, true).play();
Mugen87 commented 4 years ago

@RemusMar Can you please edit the following fiddle in order to demonstrate the issue? As you can see, if you a combine AnimationAction.play() and AnimationAction.reset(), the output seems fine. Notice that AnimationAction.play() should only called once. The idea is to control via AnimationAction.enabled whether an action should influence a model or not. AnimationAction.reset() is intended to restart the animation clip. AnimationAction.stop() should only be called if you don't need the action anymore.

https://jsfiddle.net/qm5uh6s0/

Unfortunately, I've never understood what the OP meant with timeScale is not resetted. When performing a crossFadeFrom(), the timeScale value will be updated right in the next update step (so there won't be an invalid value).

RemusMar commented 4 years ago

Yes Michael,

With RESET and STOP we get what we want in this case. The main problem is that we shouldn't need them. crossFadeFrom should always "reset" the new animation.

Mugen87 commented 4 years ago

crossFadeFrom should always "reset" the new animation.

Um, let me think about this. The animation system's API currently assumes a certain know-how about its internals. If we start to automate certain features, we have to consider the side-effects. It requires some testing/validation to understand what happens when crossFadeFrom() or even fadeIn() perform some sort of reset of animation actions.

RemusMar commented 4 years ago

Notice that AnimationAction.play() should only called once.

That's why the current approach is not the correct one. In fact, "anim2.crossFadeFrom( anim1, 0.5, true)" is a new animation. And that's why if it doesn't play by default, you need to "play()" it.

"anim2.crossFadeFrom( anim1)" means the following: create a new animation based on the current "anim1" and the new "anim2". Anim1 amplitude goes from 1 to 0. Anim2 amplitude goes from 0 to 1.

The current issue is with the 2nd access of the animation clips. Second time the Anim2 amplitude is always ZERO. And that's wrong!

RemusMar commented 4 years ago

So the 1st workaraound is:

    anim2.stop();
    anim2.crossFadeFrom( anim1, 0.5, true).play();

And the 2nd one is:

    anim2.reset();
    anim2.play();
    anim2.crossFadeFrom( anim1, 0.5, true);

But again, the current "crossFadeFrom" implementation is not the right one.

Mugen87 commented 4 years ago

Um, calling just reset() like in the fiddle should be sufficient.

anim2.reset();
anim2.crossFadeFrom( anim1, 0.5, true);
RemusMar commented 4 years ago

calling just reset() like in the fiddle should be sufficient.

It's not because I've tried. You assume that all the animations are playing from the very beginning, but that's false. Feel free to play with the files provided here: https://necromanthus.com/Test/html5/dog_gltf.html

Mugen87 commented 4 years ago

You assume that all the animations are playing from the very beginning, but that's false.

Let's say it's not always true in any scenario (for whatever reasons). If you do it like in the official example, you activate all actions you are going to use for a character once:

https://threejs.org/examples/webgl_animation_skinning_blending

RemusMar commented 4 years ago

Michael,

Again: I don't think the "official example" and "crossFadeFrom" are properly implemented. anim2.crossFadeFrom( anim1) is a new animation based on the CURRENT RUNNING anim1 and the NEW STOPPED anim2. All you need to do is to play the new animation. just my 2 cents ...