GLTFLoader: Fix collision in unique object names

[feryandi]

Related issue: #27051

Description

Use uuid instead of name to avoid name conflict. Due to name conflict, animation could run on the wrong mesh.

Edit:
Instead of using uuid, I re-arrange the unique name generation to handle conflicted names.

[donmccurdy]

The reason we don't use UUID by default is because users may clone the scene, and will still want their animations to work with the cloned scene. GLTFLoader aims to assign unique names to every object, to avoid that issue.

Based on #27051, perhaps there's a case where generating unique names is not working, but I think we'll need to investigate why that might happen...

[feryandi]

The reason we don't use UUID by default is because users may clone the scene, and will still want their animations to work with the cloned scene. GLTFLoader aims to assign unique names to every object, to avoid that issue.

Based on #27051, perhaps there's a case where generating unique names is not working, but I think we'll need to investigate why that might happen...

Thank you for the context and feedback. I changed the PR to change the createUniqueName function to avoid conflict. This may change name of some object, so I'm still not sure whether this is ok or not. I would love to get some feedbacks. Thanks!

[donmccurdy]

I feel like I am missing something obvious sorry, but I can't see what this does differently than the previous code — what was the problem and solution here?

[feryandi]

Thank you for the question.

The goal of this change is to ensure the uniqueness of node name. The previous implementation could return non-unique node name on a certain case. The issue is the array this.nodeNamesUsed doesn't memorize all of the node values because of early return on line 3529.

Here's an example, given two nodes:

• First node with name "triangle" contains two primitives, each without a name.
• Second node with name "triangle_1" contains one primitive without a name.

The loader will then process each primitive as follows:
In the old implementation:

[1] sanitizedName = "triangle"
    sanitizedName in this.nodeNamesUsed = False
    this.nodeNamesUsed["triangle"] = 0
    > return "triangle" 

[2] sanitizedName = "triangle"
    sanitizedName in this.nodeNamesUsed = True
    this.nodeNamesUsed["triangle"] = 1
    > return "triangle_1"

[3] sanitizedName = "triangle_1"
    sanitizedName in this.nodeNamesUsed = False
    this.nodeNamesUsed["triangle_1"] = 0
    > return "triangle_1"

This is causing issue, because there are two nodes named "triangle_1"

In the new implementation, here is the process:

[1] sanitizedName = uniqueName = "triangle"
    sanitizedName in this.nodeNamesUsed = False
    this.nodeNamesUsed["triangle"] = 0
    > return "triangle" 

[2] sanitizedName = uniqueName = "triangle"
    sanitizedName in this.nodeNamesUsed = True
    uniqueName = "triangle_1"
    this.nodeNamesUsed["triangle"] = 1
    this.nodeNamesUsed["triangle_1"] = 0
    > return "triangle_1"

[3] sanitizedName = uniqueName = "triangle_1"
    sanitizedName in this.nodeNamesUsed = True
    uniqueName = "triangle_1_1"
    this.nodeNamesUsed["triangle_1"] = 1
    this.nodeNamesUsed["triangle_1_1"] = 0
    > return "triangle_1_1"

This function will always create unique name for each node.

Hope this answer your question.
Let me know if you have any other concern.

Thank you.

[donmccurdy]

Ahh I see, thank you!

[takahirox]

Yeah, looks like ensuring the unique names, tho I haven't thought deeply yet.

In the example above, it would be good to respect the original names more ideally the result at [3] would be kept "triangle_1" and the result at [2] would be named something differently? If we want to do that, should we first look through all the original names? Anyways, we can improve later if needed. Thanks, good work.

[donmccurdy]

I assume it is fairly rare that a glTF model already has items named both gizmo and gizmo_1 before THREE.GLTFLoader appends a suffix to ensure unique names, but it's nonetheless both a bug fix (for the issue mentioned by OP) and a breaking change for code dependent on the current suffixes. Users of gltfjsx would need to regenerate their JSX templates, if their models are affected...

@takahirox @CodyJasonBennett – does this seem like a reasonable fix, and is there more we could be doing to avoid breaking JSX templates? I'm wondering if we could add a step in gltfjsx that warns if nodes or meshes do not have unique names in the source glTF file. Or even renames objects in the glTF file, there and then, so GLTFLoader does not have to. It's quite hard for us to guarantee that a bug fix or refactor in GLTFLoader will never change the order in which unique suffixes are generated. We can, at least, prevent future JSX templates from depending on runtime-deduplicated names.

[donmccurdy]

Raising the question on the gltfjsx repository:

• Prepass to de-duplicate object names at JSX generation time pmndrs/gltfjsx#231

[CodyJasonBennett]

I'm just taking a look now, but it seems a similar question was raised in R3F for this exact problem. pmndrs/react-three-fiber#2498

[takahirox]

I assume it is fairly rare that a glTF model already has items named both gizmo and gizmo_1 before THREE.GLTFLoader appends a suffix to ensure unique names,

I think this assumption is a bit too optimistic. I've seen other platforms that do not allow duplicate names to automatically add suffixes like "_1" to names (I don't really remember whether "gizmo, gizmo_1" or "gizmo_1, gizmo_2" anyways). This can cause problems when glTFs exported from such platforms are loaded with Three.js, which is an issue we would like to avoid.

[donmccurdy]

I think this assumption is a bit too optimistic...

Yes, perhaps. I suppose we have to assume some number of users will have objects named differently after this change, and we don't really know how many. My feeling is that we should fix the naming collision all the same. I hope it is understood that relying on the runtime-generated suffix, whatever it might be, is not guaranteed to be a stable ID across version updates.

[donmccurdy]

@takahirox from your comment I can't tell, are you OK with this change? I believe the goal is to fix #27051.

[takahirox]

@donmccurdy As commented at #27052 (comment) we may improve later, but I think good to go with the change to resolve the duplicated names problem.