PropertyBinding falls back to root node for incorrect paths/nodes

[hybridherbst]

Description

I'm not sure if this is/was intended, but we've run into a number of cases where slightly wrong animation property names result in horribly broken results, because instead of ignoring incorrect paths three falls back to applying everything to the root node (often resulting in chaos).

If that's not intended, I can take a look at making a PR.

Maybe someone knows why this is used in a number of places (falling back to the root if parsing the path fails):

targetObject = PropertyBinding.findNode( this.rootNode, parsedPath.nodeName ) || this.rootNode;

Reproduction steps

1. create a track with a wrong/incorrect path to a child node that animates position
2. play a clip with that track
3. note the parent getting animated instead of the expected result (error logged, nothing happens)

Code

    mesh.name = "Mesh1";
    mesh2.name = "Mesh2";
    mesh.add(mesh2);

    // this is problematic: the name doesn't match, but instead of being ignored the track gets applied to the root
    const trackName = "Mesh_2.position";
    const track = new THREE.VectorKeyframeTrack(
				trackName,
				[0.0, 1.0, 2.0, 3.0, 4.0],
				[0,0,0,1,0,0,1,1,0,1,1,1,0,0,0],
				THREE.InterpolateLinear
    );

    mixer = new THREE.AnimationMixer( mesh );
    const tracks = [ track ];		
    const clip = new THREE.AnimationClip( "animationclip", undefined, tracks );
    const action = mixer.clipAction( clip );
    action.play();

Live example

https://jsfiddle.net/nqyf79mp/31/

Version

dev

[Mugen87]

The fallback to the root node is present since the new animation system was added via #6934. The specific commit which add || this.rootNode in the constructor was 4985ed0. So this code was added on purpose. (BTW: The fallback is also used in bind() but it is not relevant for the above code example).

I' don't know if there is a use case that breaks if the fallback is removed and an error throw/logged instead. I'm not sure it's worth to find this out since you can easily use PropertyBinding.findNode() on app level to verify your track names and act accordingly.

[hybridherbst]

I think the behaviour with || this.rootNode is always undesired. Is there a known usecase where this is needed? Could also argue the opposite: on app level, one could decide to use PropertyBinding.findNode and then assign the root node if that is really intended. I think I outlined above why I consider this a bug.

I ran into this case with files using KHR_animation_pointer which use way more complex property bindings (as seen here: #24108).

Trying to sanitize each and every animation track before playing it on each and every asset doesn't sound like something that should happen on app level to me...

[Mugen87]

What would your desired behavior? Just ignoring the binding and log a warning?

[hybridherbst]

Yep - a warning is logged later in the code already, all I changed to fix this was to remove the fallback to root -
happy to make a PR :):

• needle-tools@6519d77

It will then log something like this instead of applying the animations to the root:
index-a222539d.js:3154 THREE.PropertyBinding: Trying to update node for track: .nodes.mixamorigRightUpLeg_3.quaternion but it wasn't found.

[Mugen87]

. Is there a known usecase where this is needed?

I've searched the history again and couldn't find something.

When we log a warning, it's at least easy to relate a potential breakage to the removal of the fallback. It's also easy to add the code back if necessary. And we finally know its purpose and can document it (or work on an alternative solution).

So I guess I'm fine with a PR.

[donmccurdy]

I think there might be important cases where an empty object name (e.g. .rotation) should be resolved to the root node. If the name is present, but incorrect, then I agree it probably doesn't need to fall back to the root, and could throw or log an error instead.

[hybridherbst]

@donmccurdy I believe that case is handled explicitly already - empty nodeName (in a couple of combinations) is targeted at root: https://github.com/mrdoob/three.js/blob/dev/src/animation/PropertyBinding.js#L196-L202

[hybridherbst]

PR is up @Mugen87