Skip to content

Conversation

@takahirox
Copy link
Collaborator

This PR cleans up buildNodeHierarchy() in GLTFLoader.

Changes

  • Move node hierarchy building and Skeleton initialization to .loadNode() from .loadScene(). With this change
    • .getDependency('node', nodeIndex) will return a node (Three.js object) with children underneath.
    • .loadScene() will be much simpler
  • Make Three.js object children in the same order as gltf nodeDef.children. Fixes GLTFExporter reorders children in groups. #15561

} else {

console.warn( 'THREE.GLTFLoader: Joint "%s" could not be found.', skinEntry.joints[ j ] );

Copy link
Collaborator

Choose a reason for hiding this comment

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

Any idea why we have the outer IIFE (starting with the return ( function() {... on line 3132? This is nested deeply enough that it's hard to read for me; removing the IIFE might help, or perhaps moving some of the skin-related code into a helper function here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems like from #15350

@donmccurdy
Copy link
Collaborator

.getDependency('node', nodeIndex) will return a node (Three.js object) with children underneath.

I agree that's cleaner, but I do have one concern. If the scene graph looks like this...

scene
  └──mesh1
      └── mesh2
           └── mesh3

... and each mesh has its own textures, are we going to request those textures serially? It looks like the child meshes will not begin loading their textures until the parent mesh has finished. Similar issue if each mesh referenced a different .bin file, but that's uncommon.

Maybe this is uncommon, and meshes usually are children of non-mesh nodes?

@takahirox
Copy link
Collaborator Author

Good point. Let me try to think how to resolve...

@mrdoob mrdoob added this to the r101 milestone Jan 16, 2019
@mrdoob mrdoob modified the milestones: r101, r102 Jan 31, 2019
@mrdoob mrdoob modified the milestones: r102, r103 Feb 28, 2019
@mrdoob mrdoob modified the milestones: r103, r104 Mar 27, 2019
@mrdoob mrdoob modified the milestones: r104, r105 Apr 24, 2019
@mrdoob mrdoob modified the milestones: r105, r106 May 30, 2019
@mrdoob mrdoob modified the milestones: r106, r107 Jun 26, 2019
@mrdoob mrdoob modified the milestones: r107, r108 Jul 31, 2019
@mrdoob mrdoob modified the milestones: r108, r109 Aug 27, 2019
@mrdoob mrdoob modified the milestones: r109, r110 Sep 25, 2019
@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 8, 2019

Any updates on this PR? It seems the concerns of @donmccurdy are not yet addressed, right?

@takahirox
Copy link
Collaborator Author

Yes, you're right. I've been a bit too busy so give me time. Or does this PR block any other PRs?

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 8, 2019

Not that I'm aware of. I just wanted to check the status of this PR 😇 . Sometimes, issues are already solved with different ones...

@mrdoob mrdoob modified the milestones: r110, r111 Oct 30, 2019
@mrdoob mrdoob removed this from the r111 milestone Nov 27, 2019
@mrdoob mrdoob added this to the r112 milestone Nov 27, 2019
@mrdoob mrdoob modified the milestones: r112, r113 Dec 23, 2019
@mrdoob mrdoob modified the milestones: r113, r114 Jan 29, 2020
@mrdoob
Copy link
Owner

mrdoob commented Feb 14, 2020

Closing this for now.

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.

GLTFExporter reorders children in groups.

4 participants