Skip to content

Fix double-sided normals for negative scale meshes#24395

Open
issam3105 wants to merge 1 commit into
bevyengine:mainfrom
issam3105:Fix_double-sided_normals_for_negative-scale_meshes
Open

Fix double-sided normals for negative scale meshes#24395
issam3105 wants to merge 1 commit into
bevyengine:mainfrom
issam3105:Fix_double-sided_normals_for_negative-scale_meshes

Conversation

@issam3105
Copy link
Copy Markdown
Contributor

@issam3105 issam3105 commented May 22, 2026

Objective

Fix incorrect PBR lighting for double-sided meshes with negative-scale transforms.

#8859 fixed culling for single-sided glTF materials by detecting inverted scale in the loader. Double-sided materials still need a shader-side fix, because culling is disabled and raw front_facing is reversed by negative-scale winding.

Solution

Add a winding-corrected front-facing helper in the PBR shader path.

Testing

Tested with Khronos NegativeScaleTest

The bottom-right -1.0 column should match the expected lighting/reflection behavior for double-sided materials.

To validate non-regression for normal/tangent handling, reviewers can also test:

NormalTangentMirrorTest
NormalTangentTest

Showcase

Before
image

After
image

@issam3105 issam3105 force-pushed the Fix_double-sided_normals_for_negative-scale_meshes branch from d188e6f to d1c8ebf Compare May 22, 2026 15:31
@kfc35 kfc35 added A-Rendering Drawing game state to the screen S-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Shaders This code uses GPU shader languages labels May 23, 2026
@github-project-automation github-project-automation Bot moved this to Needs SME Triage in Rendering May 23, 2026
@kfc35 kfc35 added C-Bug An unexpected or incorrect behavior D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels May 23, 2026
Copy link
Copy Markdown
Contributor

@kfc35 kfc35 left a comment

Choose a reason for hiding this comment

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

To the more rendering savvy reviewers who would know, should this fix also be put into the array_texture.wgsl files (under assets/shaders and examples/wasm/assets/shaders)? There is code there that also prepare world normals and applies normal mappings, although presumably it’s for examples / showcasing and not used by the engine itself

Beyond that, this code looks good to me and makes sense mathematically

@kfc35 kfc35 requested review from JMS55 and mockersf May 24, 2026 14:15
#else
let mesh_flags = mesh[in.instance_index].flags;
#endif
let mesh_is_front = pbr_functions::winding_corrected_front_facing(mesh_flags, is_front);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nitpicking on the naming: s/mesh_is_front/is_logically_front

@goodartistscopy
Copy link
Copy Markdown
Contributor

To the more rendering savvy reviewers who would know, should this fix also be put into the array_texture.wgsl files (under assets/shaders and examples/wasm/assets/shaders)? There is code there that also prepare world normals and applies normal mappings, although presumably it’s for examples / showcasing and not used by the engine itself

Indeed, either replicate this or add a comment for properly handling handedness-reversing transformations ?

Copy link
Copy Markdown
Contributor

@goodartistscopy goodartistscopy left a comment

Choose a reason for hiding this comment

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

Looks sensible apart from a small naming nit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior D-Shaders This code uses GPU shader languages D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward

Projects

Status: Needs SME Triage

Development

Successfully merging this pull request may close these issues.

3 participants