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.

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

Fix empty name problem in GLTFLoader #18585

Merged
merged 1 commit into from
Feb 10, 2020
Merged

Conversation

drcmda
Copy link
Contributor

@drcmda drcmda commented Feb 9, 2020

References:

#18530 (comment)

https://spectrum.chat/react-three-fiber/general/help-required-unable-to-load-a-gltf-model~d77788d6-35ff-4e5d-b3e9-23682c661699

GLTF's can be exported with empty names, or missing names. GLTFLoader seems to have problems with the former. For instance, this is the nodes array of the following model: https://sketchfab.com/3d-models/red-city-356c773c4b9a45d8b9d4aa04c60ecb27

  ...
  { mesh: 0, name: '' },
  { mesh: 1, name: '' },
  { mesh: 2, name: '' },
  { mesh: 3, name: '' }

GLTFLoader will auto generate names, like mesh_0, mesh_1 and so on, this happens in loadMesh:

mesh.name = meshDef.name || ( 'mesh_' + meshIndex );
if ( geometries.length > 1 ) mesh.name += '_' + i;

Notice how it checks the string itself. So, by now the mesh has an actual name, but a small bug wipes it out again afterwards in the LoadNode function:

if ( nodeDef.name !== undefined ) {
  node.userData.name = nodeDef.name;
  node.name = THREE.PropertyBinding.sanitizeNodeName( nodeDef.name );
}

Here it checks against undefined, so despite the def name being empty and the auto-generated name being present, it goes in there and overwrites the name with an empty string.

I have fixed all the other places that had this problem.

@drcmda drcmda requested a review from donmccurdy February 9, 2020 12:48
Copy link
Collaborator

@donmccurdy donmccurdy left a comment

Choose a reason for hiding this comment

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

Thanks! Agreed the change.

/cc @lexaknyazev do you think empty string, "name": "", should result in a validation warning? It seems similar to null to me, but not a strong preference.

@mrdoob mrdoob added this to the r114 milestone Feb 10, 2020
@mrdoob mrdoob merged commit bc8ed2a into mrdoob:dev Feb 10, 2020
@mrdoob
Copy link
Owner

mrdoob commented Feb 10, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants