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: Use world space normal approach #29312

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

sunag
Copy link
Collaborator

@sunag sunag commented Sep 4, 2024

Related issue: #29299 (comment)

Description

@WestLangley the approach here rendered as expected in your example and E2E testing, and maintains the performance related in the previous PR.

Copy link

github-actions bot commented Sep 4, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 685.1 kB
169.6 kB
685.1 kB
169.6 kB
+0 B
+0 B
WebGPU 822.9 kB
220.9 kB
823 kB
220.9 kB
+140 B
-3 B
WebGPU Nodes 822.5 kB
220.8 kB
822.6 kB
220.8 kB
+559 B
+95 B

🌳 Bundle size after tree-shaking

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

Before After Diff
WebGL 461.9 kB
111.4 kB
461.9 kB
111.4 kB
+0 B
+0 B
WebGPU 522 kB
140.7 kB
522.1 kB
140.8 kB
+112 B
+26 B
WebGPU Nodes 478.7 kB
130.6 kB
478.8 kB
130.6 kB
-43.23 kB
+40 B

@sunag sunag added this to the r169 milestone Sep 4, 2024
@sunag sunag marked this pull request as ready for review September 4, 2024 01:13

const transformedNormal = modelNormalMatrix.mul( normal );

return cameraViewMatrix.transformDirection( transformedNormal );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason you are using transformDirection() here?

That method is designed to be applied to vectors such as the tangent or bi-tangent, not the normal. We use the normalMatrix when transforming normal vectors.

Using transformDirection() will be correct only if the camera has uniform scale (the typical use case), but what is the point of doing it this way?

What is it about this approach that is making the code faster?

Copy link
Collaborator Author

@sunag sunag Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mesh.normalMatrix is a view space matrix, this should be called normalViewMatrix as I updated in TSL.

modelNormalMatrix.mul( normal ) will convert the normals to world space, and transformDirection() to view space. I could use mul().normalize() instead of transformDirection() but I don't see much advantage in that.

What is it about this approach that is making the code faster?

This doesn't use CPU-computed viewMatrices, so we don't need to update GPU buffers every frame since the global matrix doesn't always change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modelNormalMatrix is not the same as mesh.normalMatrix.

I used matrix3.getNormalMatrix( object.matrixWorld ) instead of matrix3.getNormalMatrix( object.modelViewMatrix ), this is a normal world space matrix.

mesh.normalMatrix is a view space matrix and should be have the suffix NormalViewMatrix in TSL context, it is not used in this code, you can access it using highPrecisionModelNormalViewMatrix, with the approach mentioned in #29299 (comment) updated.

I believe the following explanation would be the same as the one mentioned above.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The view matrix is assumed to be orthogonal and have uniform scale. Plus, we normalize the result after applying the matrix transform.

Consequently, normal vectors can be transformed with the same matrix as direction vectors.

Thus, it is OK to use transformDirection(), instead of the normal matrix, to transform the normal from world space to view space.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tldr; This looks OK to me.

@sunag sunag merged commit 71058e9 into mrdoob:dev Sep 4, 2024
12 checks passed
@sunag sunag deleted the dev-compute-normal-fix branch September 4, 2024 04:27
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.

2 participants