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.

Sign up for GitHub

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

GLTFLoader: Fix collision in unique object names #27052

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
+6 −6
Open

GLTFLoader: Fix collision in unique object names #27052

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d3b5ca9
Use uuid to prevent animation conflict
feryandi Oct 24, 2023
07e677f
Remove console log
feryandi Oct 25, 2023
4beb621
Change the unique name generation instead
feryandi Oct 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions examples/jsm/loaders/GLTFLoader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3524,17 +3524,17 @@ class GLTFParser {

const sanitizedName = PropertyBinding.sanitizeNodeName( originalName || '' );

if ( sanitizedName in this.nodeNamesUsed ) {
let uniqueName = sanitizedName;

return sanitizedName + '_' + ( ++ this.nodeNamesUsed[ sanitizedName ] );
if ( sanitizedName in this.nodeNamesUsed ) {

} else {
uniqueName = sanitizedName + '_' + ( ++ this.nodeNamesUsed[ sanitizedName ] );

this.nodeNamesUsed[ sanitizedName ] = 0;
}

return sanitizedName;
this.nodeNamesUsed[ uniqueName ] = 0;

}
return uniqueName;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh I see, thank you!

Copy link
Collaborator

@takahirox takahirox Mar 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


}

Expand Down