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

TSL: Fix bitangent* if used material.flatShading #30837

Merged
merged 9 commits into from
Apr 2, 2025

Conversation

sunag
Copy link
Collaborator

@sunag sunag commented Apr 1, 2025

Related issue: #30828 (comment)

Description

Fix bitangent* if used material.flatShading.

@sunag sunag requested a review from Copilot April 1, 2025 04:00
Copilot

This comment was marked as duplicate.

Copy link

github-actions bot commented Apr 1, 2025

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 336.36
78.34
336.36
78.34
+0 B
+0 B
WebGPU 533.07
148.15
533.25
148.18
+175 B
+28 B
WebGPU Nodes 532.54
148.05
532.71
148.08
+175 B
+29 B

🌳 Bundle size after tree-shaking

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

Before After Diff
WebGL 465.37
112.21
465.37
112.21
+0 B
+0 B
WebGPU 604.68
164.12
604.83
164.15
+149 B
+30 B
WebGPU Nodes 559.67
153.56
559.82
153.58
+149 B
+26 B

@sunag sunag added this to the r176 milestone Apr 1, 2025
@sunag sunag requested a review from Copilot April 1, 2025 04:09
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes issues with bitangent computations when materials use flat shading. It updates various accessors to conditionally assign varyings based on the material’s flatShading flag.

  • Update TextureNode to pass the builder parameter to getUV
  • Update Normal to conditionally assign a varying for normalWorld based on flatShading
  • Update Bitangent accessor implementation to correctly handle flatShading and adjust type annotations

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
src/nodes/accessors/TextureNode.js Passes builder to getUV to ensure the correct context is available
src/nodes/accessors/Normal.js Refactors normalWorld creation to use conditional varying assignment based on material.flatShading
src/nodes/accessors/Bitangent.js Wraps getBitangent in Fn, adds a varyingName argument, and standardizes bitangent types
Comments suppressed due to low confidence (3)

src/nodes/accessors/TextureNode.js:331

  • Confirm that the updated call to getUV with the additional 'builder' parameter matches the expected API and is consistent with all call sites.
uvNode = builder.context.getUV( this, builder );

src/nodes/accessors/Normal.js:78

  • Ensure that omitting the varying assignment for 'normal' when flat shading is enabled is the intended behavior and aligns with the shading strategy.
if ( builder.material.flatShading !== true ) {

src/nodes/accessors/Bitangent.js:21

  • Consider using the computed 'bitangent' (from crossNormalTangent.mul(tangentGeometry.w).xyz) instead of 'crossNormalTangent' when assigning the varying to ensure the correct value is propagated.
bitangent = varying( crossNormalTangent, varyingName );

@sunag sunag marked this pull request as ready for review April 1, 2025 04:52
@WestLangley
Copy link
Collaborator

If the front side is extruded by a normal map, then the back side should be indented. With this PR, both sides are extruded when tangents are present and flat shading is true.

You can fix that in a separate PR if you wish.

@WestLangley
Copy link
Collaborator

@sunag Are you planning on fixing the problem in this PR, or in another one?

@sunag
Copy link
Collaborator Author

sunag commented Apr 1, 2025

If the front side is extruded by a normal map, then the back side should be indented.

I agree, but apparently I'm getting these results here.

gltf.scene.children[ 0 ].geometry.computeTangents();
gltf.scene.children[ 0 ].material.side = THREE.DoubleSide;
gltf.scene.children[ 0 ].material.flatShading = true;

Front side
image

Back side
image

@sunag
Copy link
Collaborator Author

sunag commented Apr 1, 2025

In fact something is wrong, it seems to be related to the camera angle, I will have to check the code.

@sunag
Copy link
Collaborator Author

sunag commented Apr 2, 2025

@WestLangley I will make this fix in another PR due to the amount of change.

@sunag sunag merged commit 2a7cf1f into mrdoob:dev Apr 2, 2025
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