-
-
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 addon #25059
BatchedMesh addon #25059
Conversation
b82895c
to
539f9f6
Compare
FYI: If we want to support color per geometry, we may add another data texture for color. Branch: dev...takahirox:three.js:BatchedMeshColorAddon It may be good if it allows arbitary material (uniform) parameter per geometry, but I couldn't come up with good API. Maybe node based material system fits to it? It may be future work. |
examples/jsm/objects/BatchedMesh.js
Outdated
`; | ||
|
||
// @TODO: SkinnedMesh support? | ||
// @TODO: Move into the core. Can be optimized more with WEBGL_multi_draw. |
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.
Can be optimized more with WEBGL_multi_draw.
To me this is a big question ... doesn't WEBGL_multi_draw constrain the number of objects per batch quite a bit? Which could still be the better choice, but probably means a larger number of smaller batches, ideally co-located for culling, and more complex batch management...
I suspect it'll take us some time to figure that out, so I'm inclined to do our experimentation in addons and not rush this into core right 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.
Yeah, I don't think we should be in rush to move it to the core and use WEBGL_multi_draw
, or even I'm not sure we really need to do them now. To clarify it, I added "future work" to the comment.
examples/jsm/objects/BatchedMesh.js
Outdated
class BatchedMesh extends Mesh { | ||
|
||
constructor( material, maxGeometryCount = DEFAULT_MAX_GEOMETRY_COUNT, | ||
maxVertexCount = DEFAULT_MAX_VERTEX_COUNT, maxIndexCount = DEFAULT_MAX_INDEX_COUNT ) { |
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 think maxGeometryCount and maxVertexCount are probably two parameters that users really need to provide an intentional value for. We don't give a default count for InstancedMesh either. I'd be fine with material or maxIndexCount maybe having default values though. How about:
constructor( maxGeometryCount, maxVertexCount, maxIndexCount = maxVertexCount * 2, material = new MeshBasicMaterial() );
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.
Sounds good to me. Updated the constructor parameters. Default material is defined in Mesh
(super) constructor, so I don't define it here.
this._initMatricesTexture(); | ||
this._initShader(); | ||
|
||
} |
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.
just an idea — it might be nice to have some getters...
.geometryCount
.vertexCount
.indexCount
... so users can see how much space is left in the batch. The second two values would presumably change after removing a geometry and then calling .optimize()
.
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.
Sounds good to me. Added the getters.
And I renamed ._currentGeometry/Vertex/IndexCount
to ._geometry/vertex/indexCount
. I don't really remember why I named them _current
.
examples/jsm/objects/BatchedMesh.js
Outdated
|
||
} | ||
|
||
discardGeometry( geometryId ) { |
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.
nit: I think removeGeometry
or deleteGeometry
would be more consistent with other three.js APIs.
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.
OK, updated. Thanks.
} | ||
|
||
// @TODO: Rename to better name, like deflag or pack? | ||
optimize() { |
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 like .optimize()
. Or .rebuild()
would also work.
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.
OK, removed the TODO comment.
examples/jsm/objects/BatchedMesh.js
Outdated
|
||
} | ||
|
||
setVisibilityAt( geometryId, visiblity ) { |
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'm undecided on this ... would .setVisibleAt
be better? Just to match .visible
.
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.
OK, updated. Thanks.
examples/jsm/objects/BatchedMesh.js
Outdated
|
||
if ( geometryId >= this._alives.length || this._alives[ geometryId ] === false ) { | ||
|
||
// @TODO: Warning? |
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.
In general — I think it's OK (and possibly better) for these methods to hit runtime errors naturally on invalid input; we don't typically sanitize it, and I silently doing nothing may cause developers some headaches.
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.
OK, removed the TODOs. Thanks.
92cbe81
to
b73233f
Compare
Similar to #25078 there might be a chance that |
b73233f
to
cd1224c
Compare
Just wanted to point out I tested this with multiple I imagine texts require a lot more computing since I can't get good enough fps when rendering many instances. Without BatchedMesh, pure ThreeJS +
With BatchedMesh, TextGeometry from addon (no troika):
Not related to this PR, but I can't seem to mix BatchedMesh with texts from |
Just looping back to this PR - I've been trying (somewhat intensively 😅) to integrate BatchedMesh with @takahirox, would you know if it's possible to integrate BachedMesh with |
@william-mimura-thisdot i think the underlying question here would be, does troika support rendering many text instances from the same geometry and shader material? If you cannot merge troika text with |
This is very useful, thank you. I believe performance could be optimized a bit more by setting the I too would love to see support for |
Let me put this PR to r152 milestone as a reminder because many users are interested in, and no change is required in the core so it doesn't add complexity. |
Thanks for your addon, can I use raycaster or other solutions to select geomertyid? |
Unfortunately, I think raycasting raycasts the entire geometry, not per geometry instance, in the current PR. But I think it would be possible to implement. Please feel free to make a follow up PR if this PR lands. |
@Mugen87 @mrdoob I think this would be good to get in sooner rather than later. Geometry batching can be a big benefit to performance for complex applications and it would be great to take a step towards hashing out the API and using the I've had multiple clients with draw call performance issues with dynamic geometry that I would have liked to point at something like this but so far I can only say that this feature may be available at some point in the future. |
Sorry for the delay on this one! Reviewing now... |
Improve code style.
Formatting.
Clean up.
I have applied some minor refactoring in the example to make it more similar to the ones using |
Related issue: #22376
Description
This PR adds
BatchedMesh
toaddons
(not to the core).BatchedMesh
allows to render many dynamic and different shape objects referring to the same material with a single draw call. Please read #22376 (comment) for basic concept in more details.The idea is originally from @donmccurdy
Demo: https://raw.githack.com/takahirox/three.js/BatchedMeshAddon/examples/index.html#webgl_mesh_batch
API
Benefits
Good rendering performance by reducing the number of draw calls.
On my Windows 10 + Chrome, the demo above runs at 60fps with 8192 dynamic and different shape objects. The fps counter drops to like 40fps if I switch to regular meshes.
Implementation
material.onBeforeCompile()
hook to customize the vertex shader for handling local matricesTODOs
There are lot of TODOs left. You will find them in the code. And I'm not sure if tested well enough.
If the basic concept and API looks fine, I hope this PR can be merged and other contributors will help for testing and implementation.
Whether to get it in the core
BatchedMesh
API may greatly fit toWEBGL_multi_draw
. WithBatchedMesh
can be further optimized withWEBGL_multi_draw
. But it requires some changes in the core.As discussed in #22376, it would be good to start with
addons
withoutWEBGL_multi_draw
because we are not sure if it is a good moment to add change in the core now.After adding
BatchedMesh
intoaddons
, we may think of getting it into the core if all the followings are satisfied.BatchedMesh
API and demand for more stable supportWEBGL_multi_draw
and confirm it provides us noticable performance improvementAlso see #22376 (comment)