Three.js: GLTF animation causes unexpected placement

[danielrea]

Description of the problem

When i import the model it is correct.
https://gyazo.com/ff1c9b427be5ba44ab1a500bffecabeb

But with the animation on it flies off into the distance
https://gyazo.com/3ce34820e57708c86c004f7f307a332a

But it's working fine on Babylon with the same file and animation on:
https://gyazo.com/0791af74b0da7ca1f8ec7e39f8314096

Animation export settings:
https://gyazo.com/f8666858b9108691b10228caa6d29c6a

Blend file:
https://drive.google.com/file/d/1kWiWDLJ4rQ98mPha3kcss6cCl5y2iTg3/view?usp=sharing

GLB file:
https://drive.google.com/file/d/1JNY90Zvn4pxuBdQ_PivTMZpDJIpiOsOa/view?usp=sharing

Browser

• [x] All of them
• [ ] Chrome
• [ ] Firefox
• [ ] Internet Explorer

OS

• [x] All of them
• [ ] Windows
• [ ] macOS
• [ ] Linux
• [ ] Android
• [ ] iOS

[Mugen87]

I can reproduce the issue even outside of gltf-viewer.

[takahirox]

This model seems to include a node named root.

In Three.js Animation system, track name root has special meaning. It specifies an root object passed to AnimationMixer.

https://github.com/mrdoob/three.js/blob/r105/src/animation/PropertyBinding.js#L210-L214

I think we have three solutions.

1. Removing the special meaning from root track name. PropertyBinding handles node name root as just a node name, not root node. Cons is breaking backward compatibility.

2. PropertyBinding.sanitizeNodeName(), GLTFLoader calls, renames root to another name

https://github.com/mrdoob/three.js/blob/r105/src/animation/PropertyBinding.js#L112-L122

1. GLTFLoader uses object.uuid instead of object.name for track name to specify animation target node. Cons is animation clip can't be reused for cloned object because cloned object has different uuid. https://github.com/mrdoob/three.js/pull/16639#issuecomment-499086718

Probably 1 or 2 sounds reasonable. And I locally tried 1 and saw animation of this model seems fine.

[danielrea]

@takahirox thank you #1 worked for me.

[Mugen87]

I think we should reopen the issue since a fix in the library is needed. Other users might have the same problem as you.

[takahirox]

Yes, I think we should fix in the library side. Please reopen and let's close when we fix.

[Mugen87]

I think I vote for option 2. Renaming root to something different fits perfectly into PropertyBinding.sanitizeNodeName().

[donmccurdy]

Let's start with (1) or (2), yes. "Root" seems like a very generic name, e.g. I would not be surprised to see bones named that in a Blender rig. I have no idea how many users depend on that resolving automatically to the AnimationMixer's root node, but in retrospect it seems non-ideal. If we think that's too risky to change, (2) is very easy to do.

I have reservations about (3) as we've discussed, but we can keep that thread in https://github.com/mrdoob/three.js/pull/16639. :)

[takahirox]

Voting for 2, too. And I want to add note about reserved node name like root in animation system documents. Unless I'm missing, no documents mentions it, does it?

[Mugen87]

Unless I'm missing, no documents mentions it, does it?

I've checked the docs and found no reference.

[Usnul]

I don't like options (2) and (3).

(2) - you assume that the name is accidental. What if the name is intentional and I, as a user do, in fact want to find my thing with name "root". You are saying "sure, friend, use 'name' property, just know that if you set it to X we will forcibly rename it to Y".

(3) - This was, in fact, used in the Blender exporter a long while ago. And it caused me, personally, a bit of trouble. I had a model with duplicate names for different things. Assuming name to be an identity is not valid.

I don't want to push people in a specific direction. But both of these options are hacks.

[donmccurdy]

Assuming name to be an identity is not valid.

For better or worse, that's the way it works in both three.js and Blender, and I don't see any plausible path to changing this. The three.js animation system requires names to be unique _within the subtree used by an AnimationMixer_, unless you bind keyframe tracks to UUIDs (which has even more serious drawbacks) or do some manual binding by reference.

I understand there are non-identity ways you might want to use .name, but... on some level I think that's a request for a different feature, like object.classList or object.tags.

That said, I'd agree with doing (1) unless there's a reason to expect problems. If any loader was relying on that root behavior, it should be easy enough to change that. And if not, there doesn't seem to be much reason to preserve an undocumented magic behavior?

[donmccurdy]

@Mugen87 was there any specific reason you don't like option (1)? It feels like a more complete solution, given that it would be entirely reasonable for a user to give an object the name "root" manually in JS, and not expect these side effects.

[Mugen87]

@Mugen87 was there any specific reason you don't like option (1)?

When I remember correctly then option (1) could break backwards compatibility (see https://github.com/mrdoob/three.js/issues/16693#issuecomment-499092329). But honestly I don't know anymore how this breakage would look like 🤔.

[takahirox]

I'm curious to know if there is any historical reason of "root" name. It seems the "root" name got in https://github.com/mrdoob/three.js/pull/6934 by @bhouston

@bhouston, if you see this thread, is there any special reasons why you assign "root" name as root node?

And I prefer (1) now if the breakage looks causing no (or only rare case) problem.

[Mugen87]

I've create a PR that implements option (1) now. I was not able to detect any breakage in the official examples and with the animated assets on my local computer. However, we should document this change in the migration guide just for the case users relied on this "feature".

[bhouston]

This is completely my fault. I had to make a lot of decisions fairly quick when writing the https://github.com/mrdoob/three.js/pull/6934 PR back in 2015. This decision was a mistake. And removing "root" special handling is the correct solution.