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

WebGPURenderer: instance mesh use binding group instead of attribute #28726

Merged
merged 7 commits into from
Jul 19, 2024

Conversation

z4122
Copy link
Contributor

@z4122 z4122 commented Jun 23, 2024

Description

Make instance objects use binding group instead of attribute

For instancing, it is preferable to use binding group buffers rather than attributes. I know in the initial versions of Three.js, uniforms had size limitations, leading to the use of attributes as a workaround.

However, WebGPU recommends using binding groups to pass values to the GPU. This approach eliminates the need for additional computations.

Before
66283c67560e543b4a306a4e3d047c2

After
1e02c1151aa3f497833e3a1bba3e305

In WebGL2, we can use UBOs (Uniform Buffer Objects) to achieve the same functionality. However, UBOs have a size limit of 64kB in spec. To accommodate this limitation, I restrict the instance count, allowing the application to run under different conditions with varying counts. (In fact, I found that the UBO max size is far more than 64kb in my device.)

Before
a1635e780460f6ab7f30e5d6f3d939f

After
b70efb398690f12ae77e05a1ca53814

@z4122 z4122 marked this pull request as draft June 23, 2024 16:01
@z4122 z4122 changed the title Optimize/optimize instance mesh WebGPURenderer: instance mesh use binding group instead of attribute Jun 23, 2024
@z4122 z4122 marked this pull request as ready for review June 24, 2024 14:38
@z4122
Copy link
Contributor Author

z4122 commented Jun 24, 2024

@sunag @RenaudRohlinger Please review this PR

@z4122 z4122 marked this pull request as draft June 25, 2024 15:08
@sunag
Copy link
Collaborator

sunag commented Jun 25, 2024

Initially we used binding buffer, but the size was very limited, I think having both options is great.

@sunag sunag added this to the r166 milestone Jun 25, 2024
@sunag
Copy link
Collaborator

sunag commented Jun 25, 2024

Can I merge this PR or you think on doing some other revision?

@mrdoob mrdoob modified the milestones: r166, r167 Jun 28, 2024
@z4122
Copy link
Contributor Author

z4122 commented Jun 29, 2024

Can I merge this PR or you think on doing some other revision?

Please review again.

I found that WebGPU also limits uniform buffer max size https://www.w3.org/TR/webgpu/#dom-supported-limits-maxuniformbufferbindingsize. And the data in the uniform buffer must be aligned to 16 bytes. Since InstanceColor is a vec3 and occupies 12 bytes, I removed the optimization for InstanceColor.

If I change instanceColor to vec4 in InstanceMesh.js, it will affect a lot of code. Therefore, I want to run a benchmark to determine if change instanceColor is worthwhile in another PR.

@z4122 z4122 marked this pull request as ready for review June 29, 2024 16:50
Copy link

github-actions bot commented Jul 7, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
683.5 kB (169.2 kB) 683.5 kB (169.2 kB) +0 B

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
460.6 kB (111.1 kB) 460.6 kB (111.1 kB) +0 B

@z4122
Copy link
Contributor Author

z4122 commented Jul 7, 2024

@sunag I have solved code confit, please review again. Thanks!

@sunag sunag merged commit d8621e8 into mrdoob:dev Jul 19, 2024
12 checks passed
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.

3 participants