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

GLTFLoader: Add EXT_mesh_gpu_instancing built-in plugin #24528

Merged
merged 7 commits into from
Oct 13, 2022

Conversation

takahirox
Copy link
Collaborator

@takahirox takahirox commented Aug 22, 2022

Related issue: #21937
From: #24518 (comment)

Description

This PR adds glTF EXT_mesh_gpu_instancing support to GLTFLoader.

I have already made an external EXT_mesh_gpu_instancing plugin. As in #21937 Three.js devs seem to think it's good that GLTFLoader has built-in EXT_mesh_gpu_instancing plugin. So I made this PR based on my plugin.

Demo: https://raw.githack.com/takahirox/three.js/cf5bf1de939d9df3bb26f4923208b17a1723125c/examples/webgl_loader_gltf_instancing.html

image

Additional context

@Mugen87 I use assets from @shantzis1962 in the example. Please add @shantzis1962 to the change log if this PR lands.

@mrdoob mrdoob requested a review from donmccurdy August 22, 2022 22:39
@mrdoob mrdoob added this to the r144 milestone Aug 22, 2022
@mrdoob mrdoob modified the milestones: r144, r145 Aug 31, 2022
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.

Thank you, excited to get instancing merged! :)

examples/jsm/loaders/GLTFLoader.js Show resolved Hide resolved
examples/jsm/loaders/GLTFLoader.js Show resolved Hide resolved
examples/jsm/loaders/GLTFLoader.js Outdated Show resolved Hide resolved
examples/jsm/loaders/GLTFLoader.js Outdated Show resolved Hide resolved
Currenlty Three.js does not seem to support Points or Lines +
Instancing so GLTFLoader GPU instancing extension handler
ignores the extension if primitive draw mode is not triangle.
@takahirox takahirox force-pushed the GLTFLoaderGPUInstancing branch from cf5bf1d to b3810a1 Compare September 6, 2022 17:38
takahirox and others added 2 commits September 6, 2022 10:40
Co-authored-by: Don McCurdy <[email protected]>
Co-authored-by: Don McCurdy <[email protected]>
examples/jsm/loaders/GLTFLoader.js Outdated Show resolved Hide resolved
examples/jsm/loaders/GLTFLoader.js Outdated Show resolved Hide resolved
examples/jsm/loaders/GLTFLoader.js Show resolved Hide resolved

}

if ( nodeObject.isGroup ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

No change needed in this PR, but I have been wondering if we should eventually stop auto-flattening the node > mesh > primitive hierarchy, as we do now when there's only one primitive. It does make a number of cases more complex or confusing when the glTF mesh and primitives are sometimes a single THREE.Mesh, or sometimes a THREE.Group > THREE.Mesh[].

Copy link
Collaborator Author

@takahirox takahirox Sep 14, 2022

Choose a reason for hiding this comment

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

Thanks for the suggestion. I think it's worth to consider to simplify the parser. But the matrices update of a scene is still one of the performance bottlenecks. So we may move the flatterning optimization to the end of the parse, or in the plugin using afterRoot hook. Anyways, I feel like opening another issue for it to dicsuss in the details.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or perhaps a SceneUtils function separate from GLTFLoader, but happy to discuss in another issue. 😇

@mrdoob mrdoob modified the milestones: r145, r146 Sep 29, 2022
@Mugen87 Mugen87 merged commit 0432e2f into mrdoob:dev Oct 13, 2022
@Mugen87 Mugen87 mentioned this pull request Oct 13, 2022
@takahirox takahirox deleted the GLTFLoaderGPUInstancing branch October 13, 2022 19:27
@ashconnell
Copy link
Contributor

@takahirox just to clarify, instancing is now handled automatically with GLTFLoader and your external plugin is no longer needed?

@takahirox
Copy link
Collaborator Author

@ashconnell Yes, you are right.

@jrjdavidson
Copy link
Contributor

Again just clarifying, but does the GLTF file require pre-processing in order to take advantage of the instancing? I think I read that somewhere else but can't quite remember...

@donmccurdy
Copy link
Collaborator

Yes, the glTF file must contain the EXT_mesh_gpu_instancing extension. Blender does not write files that way out of the box. There are a number of ways to set that up, through React Three Fiber's gltfjsx, gltf-transform, @takahirox's work in https://github.com/takahirox/glTF-Blender-IO-EXT-mesh-gpu-instancing, etc.

@jrjdavidson
Copy link
Contributor

Gotcha. I'll look into that blender add-on. Is it worth adding a comment that says exactly what you just did in the example ? that's the first place I looked to try and understand how to use this.

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.

6 participants