Skip to content

Commit

Permalink
GLTFLoader: Fixes to improve mesh name uniqueness.
Browse files Browse the repository at this point in the history
  • Loading branch information
@donmccurdy
donmccurdy committed Sep 21, 2017
1 parent 7ee7989 commit 8c3cfb5
Showing 1 changed file with 3 additions and 5 deletions.
8 changes: 3 additions & 5 deletions examples/js/loaders/GLTFLoader.js
View file
Expand Up @@ -1807,13 +1807,10 @@ THREE.GLTFLoader = ( function () {

] ).then( function ( dependencies ) {

return _each( json.meshes, function ( meshDef ) {
return _each( json.meshes, function ( meshDef, meshIndex ) {

var group = new THREE.Group();

if ( meshDef.name !== undefined ) group.name = meshDef.name;
if ( meshDef.extras ) group.userData = meshDef.extras;

This comment has been minimized.

Sign in to view

Copy link
@cnspaha

cnspaha Nov 17, 2017

Contributor

@donmccurdy
Why did you removed if ( meshDef.extras ) group.userData = meshDef.extras; ? I did need this code to get the extras...

@twittmann
here it happened...

This comment has been minimized.

Sign in to view

Copy link
@donmccurdy

donmccurdy Nov 17, 2017

Author Collaborator

The glTF format contains node > mesh > primitive[] but in three.js this becomes Group > Mesh[]. Any of the three glTF objects could have .extras, but there aren't enough three.js objects to put the .userData, so:

  • node.extras => group.userData
  • mesh.extras => ignored
  • primitive[].extras => mesh[].userData

If you are modifying the glTF file I would put extras on the node rather than the mesh. If the extras are coming from an exporter, could you open an issue with an example?

This comment has been minimized.

Sign in to view

Copy link
@donmccurdy

donmccurdy Nov 17, 2017

Author Collaborator

In the glTF Blender exporter, for example, custom properties are automatically attached to the node and not the mesh definition.

This comment has been minimized.

Sign in to view

Copy link
@cnspaha

cnspaha Nov 20, 2017

Contributor

The extras are coming from our own exporter.

But according to #11745 you said, that it was a bug of the loader to ignore the mesh.extras and there was even a PR made for it ( #11757 ).
I wouldn't mind to make another PR to "resurrect" the changed made back then...

This comment has been minimized.

Sign in to view

Copy link
@donmccurdy

donmccurdy Nov 20, 2017

Author Collaborator

@cnspaha We don't want to add extra Object3Ds to the loaded model just to store extras in them, that was the problem with the previous code. It hurts rendering performance and causes round-trip export+imports from GLTFExporter to have extra hierarchy. See: #11944.

The simplest workaround would be to have your exporter write extras onto the node and not the mesh object. But I think I'd also be OK with a PR that merges extras of the mesh onto the node object, being careful not to overwrite more than necessary. Something like:

node.userData = Object.assign( {}, node.extras, mesh.extras );

This comment has been minimized.

Sign in to view

Copy link
@cnspaha

cnspaha Nov 21, 2017

Contributor

@donmccurdy
what do you think if we add an extra callback when .extras are being set on .userData so we can immediately listen to userData and do some magic (if needed) with the userData + Object.
In my case I could just add the object I want to, to fit my logic.
In other cases you may listen to some specific ShaderMaterial variables. stored in .extas which you could then set immediately or so.

This comment has been minimized.

Sign in to view

Copy link
@donmccurdy

donmccurdy Nov 21, 2017

Author Collaborator

I do hope to make GLTFLoader more extensible. See #11682. But I'm not sure about a special callback just for .extras... maybe something like onAfterMesh(threeMesh, gltfMesh) and you can use that to read .extras or .extensions..

It may take some time to find the right way of doing that, though.. the Object.assign fix would be the quicker change.

var primitives = meshDef.primitives || [];

return scope.loadGeometries( primitives ).then( function ( geometries ) {
Expand Down Expand Up @@ -1896,7 +1893,8 @@ THREE.GLTFLoader = ( function () {

}

mesh.name = group.name + ( i > 0 ? ( '_' + i ) : '' );
mesh.name = meshDef.name || ( 'mesh_' + meshIndex );
mesh.name += i > 0 ? ( '_' + i ) : '';

if ( primitive.targets !== undefined ) {

Expand Down

0 comments on commit 8c3cfb5

Please sign in to comment.