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

AnimationAction: action.enabled doesn’t change action._effectiveWeight #10912

Closed
jostschmithals opened this issue Mar 1, 2017 · 3 comments
Closed

Comments

@jostschmithals
Copy link
Contributor

There seems to be a logical inconsistency in AnimationMixer/AnimationAction:

From examining the comments and the code structure it becomes clear, that the action.paused property and the action.enabled property should work analogous, one concerning timescale/warping and the other concerning weight/fading.

Setting action.paused to true changes the value of action._effectiveTimeScale temporarily to 0 and leaves action.timeScale untouched (important for being able to restore this value when unpausing).

This is correct.

Setting action.enabled to false should actually do the same for the weight: leaving action.weight untouched, but changing action._effectiveWeight to 0.

But this doesn’t work.

Setting action.enabled to false has no effect on action._effectiveWeight. This is reflected for example in the dat-gui of webgl_animation_skinning_morph.html (for reproducing please deselect “enabled” in the “Clip’Action’” section and watch the “eff weight” slider).

The reason:

On the one hand action._updateWeight is implemented correctly (analogous to action._updateTimeScale), so that action._effectiveWeight actually should be set to 0 on setting enabled to false. - But: action._updateWeight will never be executed in this (action.enabled = false) case, since it is only called by action._update, and this is only called by mixer._update, if action.enabled is true.

Nevertheless the knight model in the example behaves correctly at the first glance (as if the effective weight would be set to 0 on disabling). But this happens kind of accidentally, due to the fact that the further processing of the weight (the correct property binding) is interrupted in this case. I think that is not how things should work under the hood?

How to fix:

Probably simply remove in mixer._update the condition: if (action.enabled) ? – Seems to work without negative side effects.


By the way:
Two mistakes in the comments should be fixed, too:

  1. In the constructor of AnimationAction:
    “this.enabled = true; // true -> zero effective weight” (must be “1” instead of “zero”)
  2. In setEffectiveTimeScale():
    “// set the weight stopping any scheduled warping” (must be “set the time scale …”)
@mrdoob
Copy link
Owner

mrdoob commented Mar 6, 2017

/ping @tschw @takahirox

@takahirox
Copy link
Collaborator

It seems like a bug because
_updateWeight() takes care of enabled

if ( this.enabled ) {
weight = this.weight;
var interpolant = this._weightInterpolant;
if ( interpolant !== null ) {
var interpolantValue = interpolant.evaluate( time )[ 0 ];
weight *= interpolantValue;
if ( time > interpolant.parameterPositions[ 1 ] ) {
this.stopFading();
if ( interpolantValue === 0 ) {
// faded out, disable
this.enabled = false;
}
}
}
}

but it won't be called if enabled is false.

I'll try to make PR to fix it...

@jostschmithals
Copy link
Contributor Author

Just to support your opinion (assuming that this really is a bug):

Line 146 states: … .enabled = false yields an effective weight of zero …

// although .enabled = false yields an effective weight of zero, this

And lines 152/153, read together, are implying the same:

// note: same logic as when updated at runtime
this._effectiveWeight = this.enabled ? weight : 0;

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

No branches or pull requests

3 participants