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: New normal nodes approach #29137

Merged
merged 10 commits into from
Aug 14, 2024
Merged

WebGPURenderer: New normal nodes approach #29137

merged 10 commits into from
Aug 14, 2024

Conversation

sunag
Copy link
Collaborator

@sunag sunag commented Aug 14, 2024

Related issue: Closes #28939, #29121

Description

Thanks to the once deferred function we can use constants as functions that will be executed at compile time. This approach is superior to the current ones that use transformedNormalView.assign() and I think we can use it not so soon in positionLocal and positionPrevious as well.

Copy link

github-actions bot commented Aug 14, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
685.4 kB (169.7 kB) 685.4 kB (169.7 kB) +0 B

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
462 kB (111.4 kB) 462 kB (111.4 kB) +0 B

@sunag sunag marked this pull request as ready for review August 14, 2024 16:23
@sunag sunag merged commit f571082 into mrdoob:dev Aug 14, 2024
12 checks passed
@sunag sunag deleted the dev-normals branch August 14, 2024 17:01
@sunag sunag added this to the r168 milestone Aug 14, 2024
@@ -40,7 +39,6 @@ class NodeMaterial extends Material {

this.fog = true;
this.lights = false;
this.normals = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Glad to see normals is gone! 🎉

@Mugen87 Mugen87 mentioned this pull request Aug 17, 2024
0b5vr added a commit to pixiv/three-vrm that referenced this pull request Aug 19, 2024
Starting from r168, `setupNormal` returns the normal node instead of assigning inside the `NodeMaterial.setupNormal`

See: mrdoob/three.js#29137
import { Fn } from '../../shadernode/ShaderNode.js';

const getGeometryRoughness = Fn( () => {

const dxy = normalGeometry.dFdx().abs().max( normalGeometry.dFdy().abs() );
const dxy = normalView.dFdx().abs().max( normalView.dFdy().abs() );
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've noticed that a lot of code relies on normalView for lighting calculations and PBR purposes. However, we can't depend on it as-is because, as far as I understand, normalView is only assigned once using once() and is never reassigned.

This creates a limitation where the node-based approach doesn't seem to fully support normalNode. For example, in the current PR, roughness calculations would only work if there's a roughnessMap. Otherwise, it defaults to using normalView, which is derived from normalLocal rather than normalNode.

If my understanding is correct, normalNode effectively resolves to normalView, making normalNode incompatible with the current node system. Or am I missing something here, @sunag?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think you're looking for transformedNormalView. normalView is supposed to keep the geometry normals, so it was used in getGeometryRoughness, while transformedNormalView is affected by normalNode.

I was already thinking about renaming it because the term transformed is uncommon, moving normalView to geometryNormalView and transformedNormalView to normalView, this should be a standard across all nodes.

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.

WebGPURenderer: AttributeNode: Vertex attribute "normal" not found on geometry
3 participants