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: Fix renderer.info.render when using multidraw. #29532

Merged
merged 2 commits into from
Oct 1, 2024

Conversation

agargaro
Copy link
Contributor

@agargaro agargaro commented Sep 29, 2024

Related issue: #29531

Description

info.update() increases the draw calls by 1 for each call, and here it is called more than once.
In addition, the number of triangles is not calculated correctly.

Edit: moved to draft to do more checking

@agargaro agargaro changed the title Fix multidraw render info Fix renderer.info.render data if use BatchedMesh._multiDraw Sep 29, 2024
Copy link

github-actions bot commented Sep 29, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 688.13
170.5
688.08
170.59
-50 B
+89 B
WebGPU 806.87
217.32
806.84
217.32
-25 B
-2 B
WebGPU Nodes 806.37
217.19
806.35
217.18
-25 B
-1 B

🌳 Bundle size after tree-shaking

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

Before After Diff
WebGL 462.85
111.79
462.8
111.78
-50 B
-6 B
WebGPU 535.67
144.58
535.64
144.58
-25 B
-1 B
WebGPU Nodes 491.75
134.31
491.73
134.31
-25 B
-1 B

@agargaro agargaro marked this pull request as draft September 29, 2024 22:07
@Mugen87 Mugen87 added this to the r170 milestone Sep 30, 2024
@agargaro agargaro marked this pull request as ready for review September 30, 2024 16:32
@agargaro
Copy link
Contributor Author

@CodyJasonBennett @Mugen87
Is the fix okay?

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 30, 2024

It looks good to me. This should also be checked by @gkjohnson 🙌 .

@gkjohnson
Copy link
Collaborator

The WebGLBufferRenderer implementation should be updated to match WebGLIndexedBufferRenderer (the current PR modifies the WebGPURenderer WebGL fallback version only):

for ( let i = 0; i < primcount.length; i ++ ) {
info.update( elementCount, mode, primcount[ i ] );
}

And I'm less familiar with the WebGPURenderer but it looks like the primitives aren't correctly counted in that case, either:

for ( let i = 0; i < drawCount; i ++ ) {
const count = drawInstances ? drawInstances[ i ] : 1;
const firstInstance = count > 1 ? 0 : i;
passEncoderGPU.drawIndexed( counts[ i ], count, starts[ i ] / bytesPerElement, 0, firstInstance );
}

@gkjohnson
Copy link
Collaborator

gkjohnson commented Sep 30, 2024

And I'm less familiar with the WebGPURenderer but it looks like the primitives aren't correctly counted in that case, either:

Looking at this a bit more it looks like WebGPURenderer has a fairly limited implementation of BatchedMesh (it's issuing new draw calls for each sub geometry and doesn't support non-indexed geometry) so this may just be an artifact of the still in-progress WebGPU implementation. It looks like InstancedMesh is also not supported, yet. Though I may be misunderstanding something.

@agargaro
Copy link
Contributor Author

agargaro commented Sep 30, 2024

The WebGLBufferRenderer implementation should be updated to match WebGLIndexedBufferRenderer (the current PR modifies the WebGPURenderer WebGL fallback version only):

for ( let i = 0; i < primcount.length; i ++ ) {
info.update( elementCount, mode, primcount[ i ] );
}

fixed, thanks

And I'm less familiar with the WebGPURenderer but it looks like the primitives aren't correctly counted in that case, either:

for ( let i = 0; i < drawCount; i ++ ) {
const count = drawInstances ? drawInstances[ i ] : 1;
const firstInstance = count > 1 ? 0 : i;
passEncoderGPU.drawIndexed( counts[ i ], count, starts[ i ] / bytesPerElement, 0, firstInstance );
}

is this relative to #29488? info.update() is not called at all

@gkjohnson
Copy link
Collaborator

is this relative to #29488? info.update() is not called at all

Seems likely - but again support for BatchedMesh in the WebGPU backend seems incomplete at the moment. If you can test it then it seems worth adding either way.

@RenaudRohlinger
Copy link
Collaborator

RenaudRohlinger commented Oct 1, 2024

Support for BatchedMesh in the WebGPU backend will be finalized with the completion of the indirect draw API (WIP: #29372). We'll also need to determine how to handle the statistics for indirect draw calls in renderer.info. Once we identify an appropriate way to represent these stats, we can make decisions on how to calculate the infos of the BatchedMesh in WebGPU I guess.

Also related discussion: #29197 (comment)

@Mugen87 Mugen87 merged commit cf28ac5 into mrdoob:dev Oct 1, 2024
12 checks passed
@Mugen87 Mugen87 changed the title Fix renderer.info.render data if use BatchedMesh._multiDraw BatchedMesh: Fix renderer.info.render when using multidraw. Oct 1, 2024
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.

4 participants