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

BatchedMesh: Add getInstanceCount/setInstanceCount methods for instanced multi-draw #28103

Merged
merged 11 commits into from
Apr 15, 2024

Conversation

CodyJasonBennett
Copy link
Contributor

@CodyJasonBennett CodyJasonBennett commented Apr 9, 2024

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 enhancement also aims to provide guidance for the WebGLRenderer, for example with renderMultiDrawInstances.
In the future, we might witness the introduction of an InstancedBatchMesh or some improvements to the BatchMesh. The current implementation of BatchMesh inefficiently allocates excessive memory by redundantly replicating geometries instead of utilizing a Map for referencing and instancing them.

This PR implements getInstanceCount and setInstanceCount methods for BatchedMesh and initializes a _multiDrawInstances field in BatchedMesh from WebGLBackend with per-geometry instance counts. Effectively, each geometry can be instanced as though they were an InstancedBufferGeometry, yielding significant memory savings for complex scenes.

// Enables use of instanced multi-draw
for ( let i = 0; i < count; i ++ ) {

  batchedMesh.setInstanceCount( i, instanceCount );
  batchedMesh.getInstanceCount( i );

}

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 in WebGLBackend, 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.

Copy link

github-actions bot commented Apr 9, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
673.4 kB (166.9 kB) 674.4 kB (167.1 kB) +1.03 kB

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
453.1 kB (109.4 kB) 453.8 kB (109.6 kB) +733 B
@gkjohnson
Copy link
Collaborator

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.

@CodyJasonBennett
Copy link
Contributor Author

Instanced multi-draw introduced an internal field _multiDrawInstances which contains an Int32Array with instance counts for each batched geometry, effectively like merging many InstancedMesh or InstancedBufferGeometry. As mentioned by @RenaudRohlinger, one way of using this is to not merge duplicate geometries in BatchedMesh, but increment a per-geometry instance count instead. This can dramatically reduce memory usage and consequently better GPU utilization, and my experiments will include using it by default or finding a critical threshold if there is one. Per-instance sorting could work by sorting the indirection we have for batchId rather than changing the data itself like we'd have to for InstancedMesh. It's not clear to me how to do this between instances of different geometries nor if that's solvable in WebGL without base instance. It's possible an InstancedBatchMesh wouldn't have this feature or it would come at a high cost/complexity best left to user-land -- same with InstancedMesh.

@Mugen87 Mugen87 added this to the r164 milestone Apr 11, 2024
@CodyJasonBennett
Copy link
Contributor Author

A better example would make use of a complex scene structure which benefits from both batching and instancing, for CPU cost and memory usage, respectively. I'm investigating the performance ramifications of instanced multi-draw to find a good threshold to use it.

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 EXT_mesh_gpu_instancing or some way of re-using geometry. I've used https://developer.nvidia.com/orca/amazon-lumberyard-bistro prior as a modestly complex model, but it needs some work in Blender. I'll dedicate the weekend to this, but wanted to put this PR out there so others could experiment early with the internal hooks for instanced multi-draw.

@@ -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 ) {
Copy link
Collaborator

@Mugen87 Mugen87 Apr 12, 2024

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".

Copy link
Contributor Author

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;

}
Copy link
Collaborator

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.

@CodyJasonBennett CodyJasonBennett changed the title WebGLRenderer: Backport instanced multi-draw support. Apr 13, 2024
@CodyJasonBennett CodyJasonBennett changed the title BatchedMesh: Add getInstanceCount/setInstanceCount methods to support instanced multi-draw Apr 13, 2024
@Mugen87 Mugen87 merged commit 19c51aa into mrdoob:dev Apr 15, 2024
12 checks passed
@CodyJasonBennett CodyJasonBennett deleted the instanced-multidraw branch April 15, 2024 20:28
@prideout
Copy link
Contributor

prideout commented May 3, 2024

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.)

@CodyJasonBennett
Copy link
Contributor Author

This is instanced rendering though; gl_InstanceId will be populated as you expect regardless if multi-draw is supported. See https://developer.mozilla.org/en-US/docs/Web/API/WEBGL_multi_draw. You can combine this with batchId which emulates gl_DrawId when multi-draw is not supported.

@prideout
Copy link
Contributor

prideout commented May 7, 2024

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.

@CodyJasonBennett
Copy link
Contributor Author

Please see the PR description or #28103 (comment). If you have a suggestion or improvement, open an issue.

@mrdoob
Copy link
Owner

mrdoob commented May 9, 2024

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.)

@gkjohnson what do you think?

@gkjohnson
Copy link
Collaborator

After having thought about it for a bit I'm feeling like these functions should not be on BatchedMesh. The use case is still not clear to me and they're clearly confusing.

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:

  • Remove getInstanceCount / setInstanceCount methods from BatchedMesh.
  • Clearly lay out the problems that we're having and look at what solutions or improvements to BatchedMesh there might be to address them with this multidraw instanced API or others.
  • If these functions are being used or highly desired at the moment then we can add something like InstancedBatchedMesh to examples until we can figure out what an API like this should look like.
@CodyJasonBennett
Copy link
Contributor Author

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 BatchedMesh could be implemented in user-land, and I'm increasingly of the opinion that should be the approach instead, whether that be implemented in an InstancedBatchedMesh or InstancedSkinnedMesh or neither at all. The renderer or core API doesn't have to be smart, and user-land can compose great abstractions that otherwise wouldn't be shippable in three, like if I wanted to throw in https://github.com/gkjohnson/three-mesh-bvh for sorting, culling, or simplification.

@donmccurdy
Copy link
Collaborator

donmccurdy commented May 14, 2024

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 ...

@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 WEBGL_multi_draw uses it similarly. If the documentation isn't clear enough now, a good diagram should do the trick, and I'm happy to draw something up when we're ready.


@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

  1. Way off topic... but this might be one way to help with the recurring Semver-related concerns... we liberally tag new features and most files in three/addons/* as 'experimental', and aim to hold breaking changes in non-experimental APIs to a less-than-monthly cadence.

@RenaudRohlinger
Copy link
Collaborator

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 👍

@gkjohnson
Copy link
Collaborator

gkjohnson commented May 16, 2024

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 😁

(Just not sure about the sorting/transparency part that I never covered)

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!

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