Three.js: Inconsistent behavior between AnimationMixer.stopAllActions() and AnimationAction.stop()

[DsRQuicke]

Hi,

While trying to get my animations to work as I wanted, I noticed some inconsistent behavior between AnimationMixer.stopAllActions() and AnimationAction.stop().

What I was doing basically comes down to the following (assuming two AnimationActions A and B):

1. Make my model visible.
2. Fade in A
3. After some time, cross-fade to B
4. After some more time "stop all animations" and hide the model.
5. At some later point in time, start again at 1. This is where the difference comes into play

Initially I just called stopAllActions() for "stop all animations" (Version 1 in the JSFiddle). But then I had the problem at step 5, that the fading in A again, started the fade in from the last pose that the model was in at step 4. So after stopAllActions() the model somehow seems to retained its last pose.

But if I instead call stop() on all AnimationActions (instead of just calling stopAllActions()), then the models starts fading in from the default pose (Versions 2/3 in the JSFiddle). (Which in my situation was what I wanted).
After some investigation I found out that if stopAllActions() doesn't cause restoreOriginalState() to be called on the bindings. While it does when using stop().

I'm not sure what the exact expected behavior should be, but I would guess the stop() behavior.

Change between VERSION 1, 2 & 3 in this JSFiddle to see the difference in behavior.

---

Note: If you would call stopAllActions() the first time and then for later "loops" use the stop() approach, then it will keep behaving as if stopAllActions() was called. I.e. it seems that the "original state" was updated to the last pose, and no longer is the default pose. But I didn't investigate this any further though.

---

Three.js version

• [x] Dev
• [x] r116
• [ ] ...

Browser

• [x] All of them (Probably, only tested Chrome and Firefox)
• [ ] Chrome
• [ ] Firefox
• [ ] Internet Explorer

OS

• [x] All of them (Probably, only tested Windows and Android)
• [ ] Windows
• [ ] macOS
• [ ] Linux
• [ ] Android
• [ ] iOS

[Mugen87]

If you call AnimationAction.stop(), the action internally calls AnimationMixer._deactivateAction() and then resets itself. That makes me wonder why AnimationMixer.stopAllAction() isn't implemented like this:

stopAllAction: function () {

    var actions = this._actions,
        nActions = this._nActiveActions;

    for ( var i = 0; i !== nActions; ++ i ) {

        var action = actions[ i ];
        this._deactivateAction( action );
        action.reset();

    }

    return this;

},

I think we can't merge your PR as long as this discrepancy in the implementation is unclear.

[mrdoob]

/ping @bhouston

[bhouston]

This part was rewriten by @tschw.

That said I think I would just have stopAllAction call the function stopAction() for each active action. There there can be no discrepancy between the two implementations.

[Mugen87]

The code for this would be:

stopAllAction: function () {

    var actions = this._actions,
        nActions = this._nActiveActions;

    for ( var i = 0; i !== nActions; ++ i ) {

        var action = actions[ i ];
        action.stop();

    }

    return this;

},

[mrdoob]

@Mugen87 Would you like to do a PR with it? I would replace #19329.

[DsRQuicke]

@mrdoob @Mugen87 Calling the action's stop() is indeed a cleaner, more maintainable solution than the quick fix I suggested.
I'm not sure, but isn't a reverse for-loop required in this case? Because of the swapping that is happening in AnimationMixer::_takeBackAction(). So something like this (not tested):

stopAllAction: function () {

    var actions = this._actions,
        nActions = this._nActiveActions;

    for ( var i = nActions - 1; i >= 0; -- i ) {

        var action = actions[ i ];
        action.stop();

    }

    return this;

},

[Mugen87]

According to my understanding, the order in which actions are stopped does not matter. Hence I would prefer the default for loop.

[Mugen87]

I think we should use the method at least once in the examples.How about replacing the following code section with AnimationMixer.stopAllAction().

https://github.com/mrdoob/three.js/blob/05ce4be3d80c74e7fa568fd5ab6a60a6a1a5f63e/examples/webgl_animation_skinning_blending.html#L282-L286

[DsRQuicke]

@Mugen87 I'm sorry, but I do think the reverse loop is needed. See this JSFiddle

[Mugen87]

You'are right! Thanks for sharing the fiddle. Debugging makes it easier to understand :)

Since _takeBackAction() sorts the array by putting active actions at the beginning and inactive actions at the back, it's necessary to iterate from back to forth. That explains why the walk animation is still active since the same action is stopped twice.

[Mugen87]

@DsRQuicke If you like, you can write a unit test for stopAllAction() with a subsequent PR to ensure this logic will never break.

[DsRQuicke]

I'll see what I can do for the unit test. I'll have to figure out how to do them (properly (for animations)). So it might take a couple of days.
Anyway, thank you for accepting the pull request for the issue.

[mrdoob]

Thank you for fixing it!