-
-
Notifications
You must be signed in to change notification settings - Fork 35.5k
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
BatchedMesh: Add getInstanceCount/setInstanceCount methods for instanced multi-draw #28103
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
It looks like this PR is mostly setting up hooks in WebGLRenderer to be used laster but can you explain how instanced multidraw can / should be used with BatchedMesh or some other object class in the future and what the benefits are? When I was looking into the API it wasn't clear to me how it should be used - especially if we want to be able to do something like per-instance sorting. |
Instanced multi-draw introduced an internal field |
I realize the modified examples don't visualize this well nor have a memory counter, so I'm working on an example which utilizes the new glTF |
src/renderers/WebGLRenderer.js
Outdated
@@ -856,7 +856,16 @@ class WebGLRenderer { | |||
|
|||
if ( object.isBatchedMesh ) { | |||
|
|||
renderer.renderMultiDraw( object._multiDrawStarts, object._multiDrawCounts, object._multiDrawCount ); | |||
// TODO: implement this field in BatchedMesh or InstancedBatchedMesh | |||
if ( object._multiDrawInstances !== undefined ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are planning to make this public somehow so app code is not required to define a private field, right?
How about exposing it directly as BatchedMesh.multiDrawInstances
with null
as the default? In this way, the feature isn't so "hidden away".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept this field private to align with the other multi-draw properties. Defaulting to null
is good, but what about either:
Aliasing to a public property not mentioning multi-draw:
get instanceCounts() {
return this._multiDrawInstances;
}
set instanceCounts( instanceCounts ) {
this._multiDrawInstances = instanceCounts;
}
and/or implementing setInstancesAt
in BatchedMesh
(or later InstancedBatchedMesh
)?
getInstancesAt( id ) {
if ( this._multiDrawInstances === null ) return null;
return this._multiDrawInstances[ id ];
}
setInstancesAt( id, instanceCount ) {
if ( this._multiDrawInstances === null ) {
this._multiDrawInstances = new Int32Array( this._maxGeometryCount ).fill( 1 );
}
this._multiDrawInstances[ id ] = instanceCount;
return id;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setInstancesAt()
looks easier to use since the user would not have to deal with Int32Array
.
I wonder about the naming choice for these methods since they affect multi-draw (i.e., glMultiDrawElements) but they are not related to what I tend to call "instancing" (i.e., glDrawElementsInstanced). (I came across this because I'm using a thick-lines implementation that uses instancing but does not use multi-draw.) |
This is instanced rendering though; |
I think BatchedMesh unconditionally aggregates the index buffers of all the geometries that you feed into it. For my use case of thick lines (instanced thin rectangles), the index buffers of the geometries fed into BatchedMesh should be shared, not aggregated. |
Please see the PR description or #28103 (comment). If you have a suggestion or improvement, open an issue. |
@gkjohnson what do you think? |
After having thought about it for a bit I'm feeling like these functions should not be on From what I understand these can basically be thought of "instances of the whole batched mesh" ie rendering the batched mesh multiple times which I'm still not seeing the full utility of. If you have a batched mesh with multiple objects (a character, hat, clothes, etc - say up to 20 individual meshes) then the same thing can be rendered with 20 copies of InstancedMesh with synchronized transforms. This feels like a really niche case with not a huge savings (20 draw calls) unless there's a case where you'd use this with thousands of geometries. I'd still really like to see an example of what this is solving so it can be better understood - ie a "before" that is unusable and an "after" that is fixed by the addition of instanced batches. It would help me understand the value. Here's what I propose:
|
This is particularly useful for complex models or scenes, for instance, in CAD, where I may have many different screws or small parts that are still horribly CPU and memory-bound with either instancing or non-instanced multi-draw. I don't know how to explain this any other way since it seems I'm continually misunderstood. I wish I could do more to demonstrate here, but since working on this issue, I've lost an eye, suffered partial paralysis from medical malpractice, although I can't call it that due to unreasonable Texas protections, and had to give up my SIGGRAPH demo this year of my last four years of research, so I've been particularly distracted and limited with my time here. Seeing my last two PRs backport critical capabilities for the web platform but also be reverted either due to blatant lobbying from Meta or API complications is rather demotivating, and I think I'll refrain from PRs without an RFC, whether a fix or feature. Instead, I'd like to personally sponsor $2,000 for figuring this out at the API level in three.js, specifically within the renderer, so we don't leave WebGL in the dust around general issues that can only be expressed with WebGL+nodes and not today's OOP render mode abstractions (like how do I do instanced skinning in three?). That's problematic to me; there shouldn't be asymmetry concerning three core API. These aren't new use-cases either, but people turn to engines like Wonderland for them. I've talked with @donmccurdy about my own experiments that strike a balance with low-level renderer API so |
@gkjohnson I believe this is incorrect, any virtual “geometry” in the BatchedMesh can be — individually — instanced. The potential use cases include everything InstancedMesh can do and more, even if some pieces of the API are still missing. The API design questions here are complex, but the potential impact is very high if we can make this convenient... Let's try to work out an API proposal / RFC in a separate document? Feels like a document-shaped discussion. I've been surprised by the speed of uptake on the BatchedMesh API, maybe this is a good time to think about how it fits with other abstractions over upcoming releases. Admittedly it may take some time, but I don't see a rush to revert anything now. Perhaps tag these methods experimental,1 but I think the risk+cost of having them around in the meantime is low. I do feel "instances" is the most appropriate name, and documentation for @CodyJasonBennett that's an awful lot going on, and very sorry to hear it. I hope you're recovering well, and would encourage you to do whatever feels valuable to you right now. Ideas on what these APIs could look like — say, 1–2 years from now — could be very helpful to three.js. I think your experiments with meshlets would be a really useful case study in that direction too. But also, that pressure doesn't need to fall on you if you want to do other things, and please take your time in any case. Footnotes |
I'm already using the features from this PR in my projects, so seeing it reverted would indeed be unfortunate. @gkjohnson As we previously discussed in Tokyo last year, I've already implemented a working version of batchmesh that automatically gets instances based on shared geometry. It's essentially just an extra data texture of uint IDs and an extra Map() in the code to reference the batch shared IDs. (Just not sure about the sorting/transparency part that I never covered) After my renderbundle work I can give it a try @CodyJasonBennett 👍 |
I talked Cody a bit separately but to clarify I just intended to suggest an alternative to adding this to BatchedMesh since it's not as clear or simple to use as the rest of class. Removing documentation is a fine solution, as well. Rereading my former comment I think I could have phrased it different. I understand people people feel strongly about this, though, so I won't push it. But if this has gotten people interested in pushing instancing support forward in BatchedMesh forward a bit more, seems like a good outcome 😁
I don't see it being possible with this function since all instances will be rendered sequentially no matter what. Though maybe I'm wrong. I've had some ideas on how to get this working I was going to give a try in the coming months but I'll see if I can get something working this week to see if it works! |
Related issue: #27170, #27937
Description
Fixes and backports #27950 to core so we can iterate on instanced multi-draw as an optimization to
BatchedMesh
and/or a public API as described prior, which is not tracked in triage other than general instancing improvements:This PR implements
getInstanceCount
andsetInstanceCount
methods forBatchedMesh
and initializes a_multiDrawInstances
field inBatchedMesh
fromWebGLBackend
with per-geometry instance counts. Effectively, each geometry can be instanced as though they were anInstancedBufferGeometry
, yielding significant memory savings for complex scenes.I've added an "instances" slider in both batching examples you can find below, where a non-zero count will use instanced multi-draw:
Note
Multi-draw is not supported by WebGPU (only its indirect counterparts), although I only see special handling for
BatchedMesh
inWebGLBackend
, so "instances" has no effect under the WebGPU backend.I've since reverted changes to those examples in d694d76, and working on a better example with use of a complex scene structure to benefit from both batching and instancing, for CPU cost and memory usage, respectively. A memory counter should be tracked to display memory usage. It is not clear whether sorting can be implemented for instancing in WebGL without severe performance ramifications as we can't interleave instance data nor configure a base instance despite additional indirection (see #28103 (comment)). This is comparably trivial in WebGPU where we aren't exclusively beholden to vertex state with the use of storage memory.