GLTFLoader: Deduplicate node names.

[donmccurdy]

Fixes #15087.

/cc @takahirox

[takahirox]

Good work. Probably renaming is reasonable to resolve the animation problem.

But minor concern. Polluting user or asset-author defined data (name) might break a certain workflow. For example, if a user has a program out of Three.js processing something only for nodes which have the certain same name. Imagine one imports gltf to Three.js, does something, and then exports back to gltf. If the loader renames the program might not work correctly for exported gltf.

I think it may be kind of rare use case. It'd be ok to merge this PR now and resolve my concern later (if needed).

Potential solutions may be

1. With GLTFLoader - store original node name #16412 we store original name. GLTFExporter restores the name when exporting.

2. Console warning if renamed. At least user can notice the rename. Cons is it can be tons of warning if many nodes are renamed.

3. Add rename option to the loader. Default is false. User needs to explicitly to turn it on for rename so at least one can notice the rename.

I speculate 1 sounds good?

[takahirox]

BTW, I'd think better to optimize the algorithm.

GLTFParser.prototype.assignUniqueName = function ( object, originalName ) {

	var name = originalName;

	for ( var i = 1; this.nodeNamesUsed[ name ]; ++ i ) {

		name = originalName + '_' + i;

	}

	this.nodeNamesUsed[ name ] = true;

	object.name = name;

};

The complexity is O(n) where n is the number of nodes having the same name in a gltf.
One renaming is O(n), so O(n^2) for parsing entire gltf because renaming n times. For example, if gltf has a lot of nodes with empty name (eg: Voxel model?), user may see serious performance problem.

This is similar performance problem to the geometry cache performance problem we had and fixed. Hopefully O(1) algorithm is good.

[donmccurdy]

I’d prefer not to preserve the original name in extras, not to give a warning, and not to add a new option – this all seems too complex for a case that has never given us any reason to add complexity. Three.js and other tools depend on unique names, and if users’ workflows depend on names then they should make those names unique. If they’re trying to assign non-unique data like tags, they can use glTF extras just as easily.

The complexity is O(n) where n is the number of nodes having the same name in a gltf.

I think it’s pretty unlikely that N is ever going to be large with this case, but I’d be open to suggestions on other ways to implement this without naming conflicts.

if gltf has a lot of nodes with empty name (eg: Voxel model?), user may see serious performance problem.

We’re intentionally not de-duplicating empty names here, so _1, _2, ... would not be generated.

[takahirox]

Just to clarify, I don't block merging. Just my two cents.

I’d prefer not to preserve the original name in extras

With #16412 we already store original name to .userData.name. I meant I suggested glTF exporter (not loader) exports a node with original name if exists.

// in GLTFExporter
if (object.userData.name || object.name) gltfNode.name = object.userData.name || object.name;

Three.js and other tools depend on unique names,

Yeah unique names may be less problematic but Three.js and glTF core spec allow duplicate names.

I think better not to lose or change valid information during gltf import-export cycle not to affect user workflow. I know we can't perfectly keep information because Three.js structure doesn't perfectly 1:1 to glTF spec so as much as possible tho.

if users’ workflows depend on names then they should make those names unique.

The example I said

For example, if a user has a program out of Three.js processing something only for nodes which have a certain name.

meant there might be program processing multiple nodes which have the same certain name. I know it may be rare but I just wanted to mention that there might be a chance renaming could break user workflow.

I think it’s pretty unlikely that N is ever going to be large with this case,

We’re intentionally not de-duplicating empty names here, so _1, _2, ... would not be generated.

We don't do for empty node names but we do for empty mesh names with mesh_1, mesh_2, .... In general the number of meshes wouldn't be so large but I guess there might still be a chance. Yeah I know it may be rare so optimizing later may be ok. Adding inline comment about O(n) so far would be good.Update: I realized 'mesh_' + meshIndex is passed as original name for empty mesh name, so yes pretty unlikely N can big. But still feeling like adding inline comment just in case.
Update: We don't do for empty node names but we do for mesh names. If mesh has non-empty name and has tons of primitives user might see performance problem.

[donmccurdy]

I know it may be rare but I just wanted to mention that there might be a chance renaming could break user workflow.

I understand, but it's not just rare – it's not a workflow we've ever seen anyone use before. The glTF spec tries to be clear, but cannot disallow users from doing all possible bad things. That doesn't mean we have to preemptively support bad workflows in three.js, just because they're technically possible. Relying on non-unique names is a bad idea in any three.js-based workflow. I would much rather not add code we have to maintain for this – at least this way it handles things in an easy-to-understand way (by deduping the names) rather than a hard-to-understand way (animation is broken).

We don't do for empty node names but we do for empty mesh names with mesh_1, mesh_2,...

Hm, I see. So if the mesh had 100 primitives, they'll get the same initial name, and then that's looping over a lot of things to dedupe them. We could fix that case by including an index on each primitive's default name, so they don't conflict?

[takahirox]

I was thinking of using object.uuid instead of object.name. If we do, the loader can be a bit simpler because we can remove unique name creation function and PropertyBinding.sanitizeNodeName(). And directly pointing node with uuid seems being more fitting to glTF spec rather than specifying with name. In glTF spec, channel.target directly points to a node with node index.

But yes, as you mentioned in the previous thread, if we use uuid animation clip can't be reused for cloned object because the cloned object will have different uuid. Probably we need to add this type of animation clip cloning utility function. But maybe still user who wants to clone object and reuse animation would be bothered a bit.

function cloneAnimationClipForClonedObject(clip, clonedObject) {
  var uuid = clonedObject.uuid;
  var tracks = clip.tracks;
  var newTracks = [];
  for (var i = 0, il = tracks.length; i < il; i++) {
    var track = tracks[i];
    var newName = track.name.replace(/^[^.]*\./, uuid + '.');
    var newTrack = new track.constructor(newName, track.times, track.values, track.getInterpolation());
    newTracks.push(newTrack);
  }
  return new THREE.AnimationClip(clip.name, clip.duration, newTracks);
}

So, to be honest I'm still on the fence about renaming. But I can understand your opinion. Renaming is reasonable solution and less problem animation is very useful to users. And I know maybe I'm thinking of very rare (or unlikely happening) use case. So please go ahead with this PR.

Hm, I see. So if the mesh had 100 primitives, they'll get the same initial name, and then that's looping over a lot of things to dedupe them. We could fix that case by including an index on each primitive's default name, so they don't conflict?

As you know personally I don't really want to aggressively rename but yes I think it'd resolve.

[takahirox]

Just in case, let me clarify my opinion. I don't block this change. Yes, we should resolve animation problem in case nodes have non-unique name. Renaming sounds a reasonable solution. Just I wanted to share my two cents.

[robertlong]

Right now we're seeing a related bug in Mozilla Spoke. Someone uses an asset from Sketchfab with animations and duplicate names and it breaks.

Example:
https://sketchfab.com/3d-models/christmas-low-poly-scene-7984bd6024db4b89928ddeb625eebac0

We want to be able to export a scene composed of these assets and others using GLTFExporter. I wish we could keep the same name and avoid renaming nodes altogether, that'd probably create less confusion when using the exported asset. If there's any way to make the AnimationMixer not rely on names, or an alternate AnimationMixer that handled references to objects some other way, that'd be ideal, but maybe this solution is good enough.

[donmccurdy]

I have no better suggestion than this PR – in my opinion, we should deduplicate node names.

If there's any way to make the AnimationMixer not rely on names, or an alternate AnimationMixer that handled references to objects some other way, that'd be ideal...

AnimationMixer also supports UUIDs. If you never need to clone an animated object, or are willing to implement custom logic to remap animations to the new UUIDs after cloning, that would work fine for Spoke – just modify GLTFLoader to target animations to a UUID. Unfortunately, this is not a general-purpose solution: most users expect .clone() to work with animated objects, and the fact that it does not work naturally is (a) unavoidable as far as I can see, and (b) a more critical problem than appending suffixes to nodes with duplicated names.

[donmccurdy]

Rebased ✅

[takahirox]

Thanks for rebasing.

E2E check seems to fail on webgl_instancing_scatter example. The example uses GLTFLoader, sorry I haven't looked into the detail yet but can this change affect the example? You made the example so I guess you may know.

[donmccurdy]

Thanks! Looks like that failure might be an actual issue, I'll look into it.

[donmccurdy]

Ok, fixed. Bit of an edge case to make sure that the parent object gets first shot at the name, rather than its children.

[mrdoob]

Thanks!

[revilotio]

I was wondering if there is any specific reason to start at index i=1 when renaming nodes within the GLTFLoader, rather than i=0. The loop I am referring to is located in the assignUniqueName function posted below:

GLTFParser.prototype.assignUniqueName = function ( object, originalName ) {

	var name = originalName;

	for ( var i = 1; this.nodeNamesUsed[ name ]; ++ i ) {

		name = originalName + '_' + i;

	}

	this.nodeNamesUsed[ name ] = true;

	object.name = name;

};

I am adressing this because previous ThreeJS versions would start renaming nodes starting at index 0. As a result the GLTF-Loader caused problems in certain ThreeJS-projects of mine.
Concerning backwards compability I would therefore suggest to start the loop at index i=0, if there is no specific reason to start at index i=1.

[drcmda]

keep in mind though that backward compat is already shot. gltfloader changed two or three times over the last few releases, each handling naming a little bit different. this hit the react crowd especially since they are not traversing gltf data, they depend on unique names and declarative scenes. if we change this just to fix an index it will not do anything to backward compat since that's already gone out of the window. instead it will just break code again and with all the hard breaking changes that three had recently it's getting increasingly difficult. i suggest not touching this part so often since it's finally working. can't we just assume 1 means it's the first duplicate?

[donmccurdy]

It's pretty hard to maintain backwards compatibility on generated names, since the order of traversal depends on various things. Ideally, it would be best to ensure everything that needs a stable name contains that name in the file. However, the switch from 0- to 1-indexed names probably was not necessary, sorry about that issue! But as @drcmda says, it may just cause more churn to revert it at this point.

[acmoles]

I really appreciate all the hard work you're all putting into this... but you broke my workflow ;)

Previously it was possible to load a GLTF file with several skinned meshes and several animation clips and share those clips among them. Upon updating I notice only one mesh is able to use those animations and after rooting around the problem I believe this change is the cause. I haven't figured out why it's causing an issue yet and would appreciate a steer.

Here is where I was applying animations to each mesh, published at: https://www.acmoles.com/about/

[mrdoob]

@acmoles Oh noes... Any change you can put together a simple jsfiddle that repros the issue?

[acmoles]

@mrdoob Here's a fairly minimal fiddle: https://jsfiddle.net/acmoles/oy60a41z/132/

You can change the CDN link from latest to r115 to see what happens - animations working on all models in r115 and stop on latest
(but I think the problem is introduced from r120 - r121, I just couldn't get UNPKG to work for some reason, maybe adblocker)

Thanks in advance for checking it out!

[FlorentMasson]

Deduplicating names is a big breaking change for us, I wish there was a way to disable it.
The way described here #15087 (comment) appears to be working.
Are there any hidden side effects?

[donmccurdy]

Are there any hidden side effects?

glTF targets animations to specific objects by an internal ID, and does not care what the names of the objects are. three.js targets animation to nodes by name, and assumes that those names are unique within the subtree of the AnimationMixer. So, the hidden side effect of disabling de-duplication of names is that if you are trying to animate anything with a duplicated name, your animation might modify the wrong part of the scene.

The example by @acmoles is basically making use of that mismatch between glTF and three.js, and retargeting animations (which did target a single character in the source glTF file) to different characters, which is possible if you know that the characters have similarly-named joints.

The other workaround we've considered was to set up the animations to target object.uuid, which three.js already supports, and then you don't have to de-duplicate names. But then you have another problem — objects cannot be animated after cloning — and the workflow of @acmoles still wouldn't work in this case.

---

I don't have a good solution for this. De-duplicating names ensures that animation plays as expected for people who are just copying animation examples from the three.js tutorials, regardless of the input file. But it does cause problems for those whose workflows depend on objects having specific names, like this case. I don't know how to make both of those "just work", but here are two compromise options:

1. Revert this change, no longer de-duplicating names, and log a warning if any duplicated names are targeted by animation. Then at least the user has a way to figure out why the animation doesn't play, and @acmoles' workflow would be supported, albeit with a noisy warning. Users could either rename things in Blender (lot of work...) or we could provide an opt-in method to do the unique naming.
2. Keep this change, but provide a method to opt out of unique naming.

[FlorentMasson]

Thanks for the clarification!

I'm a bit surprised the motivation is fixing duplicate naming in three.js tutorials, wouldn't it best to update them? Ambiguous names can't be a good thing there.

We use mixamo animations which do not have duplicate names, so that's not a concern for us.
But for level design we use Sketchup which generates duplicate names (especially when you use components). We use names to locate specific nodes and we had to use full paths with names from root to leaf - and manually make sure at least the full path is unique. If the de-duplication had been there from the beginning it would certainly have been easier so I can't really speak up against it.
My only worry would be that there's no guarantee a deduplicated node will always have the same name when the node ordering changes. And that, unless I'm mistaken, the uniqueness of names is questionable when names can be empty.

Just a random thought but if duplicated names are only a problem for animation targets, maybe only these names should be deduplicated?

[drcmda]

if there's any debate to be had still, i beg you to please not break it again. in my opinion, de-dupe is quite useful and if people can opt out with a plugin that would be fine. the plugin could be officially put into jsm. it went through a serious of hard breaking changes and finally it has settled for a bit. seeing codebases and hundreds of examples, hundreds of apps our users make, all just break on every release for multiple months in row wasn't fun.

Just a random thought but if duplicated names are only a problem for animation targets, maybe only these names should be deduplicated?

in react scenes can (and should be) laid out declaratively: https://github.com/pmndrs/gltfjsx which saves us from querying and traversal. this isn't a change that would just affect 1-3 people messing with mixamo keyframes. this would send us all scrambling. hundreds of examples we made, all apps in the react space that use gltf.

[donmccurdy]

I'm a bit surprised the motivation is fixing duplicate naming in three.js tutorials, wouldn't it best to update them?

There's nothing wrong with the tutorials though — if you load a model and play its animations, users should expect that to just work. But if the model happens to have duplicate names on animated objects, it won't work in three.js without this de-duplication. The same glTF file will work in any other glTF viewer though, it's a perfectly valid file.

I guess I am leaning toward an opt-out, then. Something like:

const loader = new GLTFLoader()
  .setUniqueNames( false )
  .setKTX2Loader( ... )
  .load( ... );

[acmoles]

The opt-in would be good. Sharing animations among similar skinned meshes is quite a common workflow in game engines, so it's likely to be an expectation for users coming from that world.

[mattmattvanvoorst]

I would like to offer the consideration of maybe providingng both? A uuid and name property which are unique, or truthful to the original ID provided in whatever modelling software was used create it?

[donmccurdy]

The property object.uuid is always there in three.js, but has nothing to do with the source glTF model. With the setUniqueNames(false) option, any object.name value would not be changed from the source glTF model, and should match whatever the source file provided. We might still want to generate names for unnamed objects either way though.