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

[Merged by Bors] - bevy_pbr: Fix incorrect and unnecessary normal-mapping code #5766

Closed

Conversation

superdump
Copy link
Contributor

@superdump superdump commented Aug 22, 2022

Objective

  • Fixes Self-shadowing on back side of meshes with "double sided lighting" enabled #4019
  • Fix lighting of double-sided materials when using a negative scale
  • The FlightHelmet.gltf model's hose uses a double-sided material. Loading the model with a uniform scale of -1.0, and comparing against Blender, it was identified that negating the world-space tangent, bitangent, and interpolated normal produces incorrect lighting. Discussion with Morten Mikkelsen clarified that this is both incorrect and unnecessary.

Solution

  • Remove the code that negates the T, B, and N vectors (the interpolated world-space tangent, calculated world-space bitangent, and interpolated world-space normal) when seeing the back face of a double-sided material with negative scale.
  • Negate the world normal for a double-sided back face only when not using normal mapping

Before, on main, flipping T, B, and N

Screenshot 2022-08-22 at 15 11 53

After, on this PR

Screenshot 2022-08-22 at 15 12 11

Double-sided material without normal maps

Kapture.2022-08-22.at.19.17.53.mp4

Changelog

  • Fixed: Lighting of normal-mapped, double-sided materials applied to models with negative scale
  • Fixed: Lighting and shadowing of back faces with no normal-mapping and a double-sided material

Migration Guide

prepare_normal from the bevy_pbr::pbr_functions shader import has been reworked.

Before:

    pbr_input.world_normal = in.world_normal;

    pbr_input.N = prepare_normal(
        pbr_input.material.flags,
        in.world_normal,
#ifdef VERTEX_TANGENTS
#ifdef STANDARDMATERIAL_NORMAL_MAP
        in.world_tangent,
#endif
#endif
        in.uv,
        in.is_front,
    );

After:

    pbr_input.world_normal = prepare_world_normal(
        in.world_normal,
        (material.flags & STANDARD_MATERIAL_FLAGS_DOUBLE_SIDED_BIT) != 0u,
        in.is_front,
    );

    pbr_input.N = apply_normal_mapping(
        pbr_input.material.flags,
        pbr_input.world_normal,
#ifdef VERTEX_TANGENTS
#ifdef STANDARDMATERIAL_NORMAL_MAP
        in.world_tangent,
#endif
#endif
        in.uv,
    );

The FlightHelmet.gltf model's hose uses a double-sided material. Loading the
model with a uniform scale of -1.0, and comparing against Blender, it was
identified that negating the world-space tangent, bitangent, and interpolated
normal produces incorrect lighting. Discussion with Morten Mikkelsen clarified
that this is both incorrect and unnecessary.
@superdump superdump added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Aug 22, 2022
@superdump
Copy link
Contributor Author

I did test this PR with positive/negative uniform scales, and also non-uniform scales with all/some positive components, and all negative components. At least empirically, this looks correct. And unless I misunderstood, it is technically correct too.

@rparrett
Copy link
Contributor

This fixes #4019 as its title is written (no more self-shadowing), but the color of the quad is still off / not the same as when it is facing "forward."

@superdump
Copy link
Contributor Author

superdump commented Aug 22, 2022

This fixes #4019 as its title is written (no more self-shadowing), but the color of the quad is still off / not the same as when it is facing "forward."

Not really. That example does not use normal-mapping. I'll take a look at it anyway. Scratch that. I see that my changes would affect non-normal-mapped cases too. Hum.

@superdump
Copy link
Contributor Author

@rparrett there you go. :)

@rparrett
Copy link
Contributor

Looking good. I can't really help with the rest, but I noticed that the behavior changed with this PR and figured I'd mention it.

@superdump superdump changed the title bevy_pbr: Remove incorrect and unnecessary normal-mapping code bevy_pbr: Fix incorrect and unnecessary normal-mapping code Aug 22, 2022
@djeedai
Copy link
Contributor

djeedai commented Aug 31, 2022

So, I've got limited knowledge on that area but knowing that normals are transformed by the inverse transpose and knowing that a uniform -1 scale matrix is its own inverse transpose, i would expect the normals to need to always be flipped. What smells wrong too is the "but only when not using normal mapping"; why would the use of the normal influence how it's calculated? The change seems to produce the correct visual result but it feels like it's for the wrong reasons and there are 2 bugs compensating each other.

@superdump
Copy link
Contributor Author

So, I've got limited knowledge on that area but knowing that normals are transformed by the inverse transpose and knowing that a uniform -1 scale matrix is its own inverse transpose, i would expect the normals to need to always be flipped. What smells wrong too is the "but only when not using normal mapping"; why would the use of the normal influence how it's calculated? The change seems to produce the correct visual result but it feels like it's for the wrong reasons and there are 2 bugs compensating each other.

I would agree, except that normal mapping takes into account the orientation of the texture coordinates and the impact of the negative scaling on the orthogonal basis formed by the tangent, bitangent, and normal. When normal mapping is not used, the vertex normals stand for themselves. That said, I don’t understand exactly why this should be as it is yet, just that changes were necessary, and came from Mikkelsen themself about double-sided and normal mapping. And those changes broke the non-normal mapping case which doesn’t use any of the things that result in signs being flipped. I’ll take another closer look when I have the head space to do so and try to derive the two cases to identify why.

@superdump
Copy link
Contributor Author

I haven’t had time to derive why these are not needed in the normal mapping case but are in the non-normal mapping case. However, the normal mapping case has been stated by Mikkelsen, and the non-normal mapping case is simple to understand why it needs flipping so I’m taking this out of draft.

@superdump superdump marked this pull request as ready for review September 15, 2022 22:09
@superdump
Copy link
Contributor Author

I have retested the problem cases (using a vertically-oriented Quad in 3d_shapes, rotating it about Y, and making its material double-sided and disabling culling causes shadow-mapping issues like in #4019 on main, but is fine on this PR; using a Vec3::splat(-1.0) scale on the flight helmet model in load_gltf looks bad on main but good on this PR), and regular cases (plain load_gltf and lighting).

@superdump superdump added this to the Bevy 0.9 milestone Nov 3, 2022
@superdump
Copy link
Contributor Author

@cart I've added this to the 0.9 milestone so it doesn't go unaddressed for another release. :)

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Weak approval here; the code quality, docs, justification and tests you've shown all check out to me.

I'm not personally equipped to verify this math directly ATM though.

@cart
Copy link
Member

cart commented Nov 3, 2022

Makes sense to me!

@cart
Copy link
Member

cart commented Nov 3, 2022

bors r+

bors bot pushed a commit that referenced this pull request Nov 3, 2022
# Objective

- Fixes #4019 
- Fix lighting of double-sided materials when using a negative scale
- The FlightHelmet.gltf model's hose uses a double-sided material. Loading the model with a uniform scale of -1.0, and comparing against Blender, it was identified that negating the world-space tangent, bitangent, and interpolated normal produces incorrect lighting. Discussion with Morten Mikkelsen clarified that this is both incorrect and unnecessary.

## Solution

- Remove the code that negates the T, B, and N vectors (the interpolated world-space tangent, calculated world-space bitangent, and interpolated world-space normal) when seeing the back face of a double-sided material with negative scale.
- Negate the world normal for a double-sided back face only when not using normal mapping

### Before, on `main`, flipping T, B, and N

<img width="932" alt="Screenshot 2022-08-22 at 15 11 53" src="https://user-images.githubusercontent.com/302146/185965366-f776ff2c-cfa1-46d1-9c84-fdcb399c273c.png">

### After, on this PR

<img width="932" alt="Screenshot 2022-08-22 at 15 12 11" src="https://user-images.githubusercontent.com/302146/185965420-8be493e2-3b1a-4188-bd13-fd6b17a76fe7.png">

### Double-sided material without normal maps

https://user-images.githubusercontent.com/302146/185988113-44a384e7-0b55-4946-9b99-20f8c803ab7e.mp4

---

## Changelog

- Fixed: Lighting of normal-mapped, double-sided materials applied to models with negative scale
- Fixed: Lighting and shadowing of back faces with no normal-mapping and a double-sided material

## Migration Guide

`prepare_normal` from the `bevy_pbr::pbr_functions` shader import has been reworked.

Before:
```rust
    pbr_input.world_normal = in.world_normal;

    pbr_input.N = prepare_normal(
        pbr_input.material.flags,
        in.world_normal,
#ifdef VERTEX_TANGENTS
#ifdef STANDARDMATERIAL_NORMAL_MAP
        in.world_tangent,
#endif
#endif
        in.uv,
        in.is_front,
    );
```

After:
```rust
    pbr_input.world_normal = prepare_world_normal(
        in.world_normal,
        (material.flags & STANDARD_MATERIAL_FLAGS_DOUBLE_SIDED_BIT) != 0u,
        in.is_front,
    );

    pbr_input.N = apply_normal_mapping(
        pbr_input.material.flags,
        pbr_input.world_normal,
#ifdef VERTEX_TANGENTS
#ifdef STANDARDMATERIAL_NORMAL_MAP
        in.world_tangent,
#endif
#endif
        in.uv,
    );
```
@bors bors bot changed the title bevy_pbr: Fix incorrect and unnecessary normal-mapping code [Merged by Bors] - bevy_pbr: Fix incorrect and unnecessary normal-mapping code Nov 3, 2022
@bors bors bot closed this Nov 3, 2022
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…ne#5766)

# Objective

- Fixes bevyengine#4019 
- Fix lighting of double-sided materials when using a negative scale
- The FlightHelmet.gltf model's hose uses a double-sided material. Loading the model with a uniform scale of -1.0, and comparing against Blender, it was identified that negating the world-space tangent, bitangent, and interpolated normal produces incorrect lighting. Discussion with Morten Mikkelsen clarified that this is both incorrect and unnecessary.

## Solution

- Remove the code that negates the T, B, and N vectors (the interpolated world-space tangent, calculated world-space bitangent, and interpolated world-space normal) when seeing the back face of a double-sided material with negative scale.
- Negate the world normal for a double-sided back face only when not using normal mapping

### Before, on `main`, flipping T, B, and N

<img width="932" alt="Screenshot 2022-08-22 at 15 11 53" src="https://user-images.githubusercontent.com/302146/185965366-f776ff2c-cfa1-46d1-9c84-fdcb399c273c.png">

### After, on this PR

<img width="932" alt="Screenshot 2022-08-22 at 15 12 11" src="https://user-images.githubusercontent.com/302146/185965420-8be493e2-3b1a-4188-bd13-fd6b17a76fe7.png">

### Double-sided material without normal maps

https://user-images.githubusercontent.com/302146/185988113-44a384e7-0b55-4946-9b99-20f8c803ab7e.mp4

---

## Changelog

- Fixed: Lighting of normal-mapped, double-sided materials applied to models with negative scale
- Fixed: Lighting and shadowing of back faces with no normal-mapping and a double-sided material

## Migration Guide

`prepare_normal` from the `bevy_pbr::pbr_functions` shader import has been reworked.

Before:
```rust
    pbr_input.world_normal = in.world_normal;

    pbr_input.N = prepare_normal(
        pbr_input.material.flags,
        in.world_normal,
#ifdef VERTEX_TANGENTS
#ifdef STANDARDMATERIAL_NORMAL_MAP
        in.world_tangent,
#endif
#endif
        in.uv,
        in.is_front,
    );
```

After:
```rust
    pbr_input.world_normal = prepare_world_normal(
        in.world_normal,
        (material.flags & STANDARD_MATERIAL_FLAGS_DOUBLE_SIDED_BIT) != 0u,
        in.is_front,
    );

    pbr_input.N = apply_normal_mapping(
        pbr_input.material.flags,
        pbr_input.world_normal,
#ifdef VERTEX_TANGENTS
#ifdef STANDARDMATERIAL_NORMAL_MAP
        in.world_tangent,
#endif
#endif
        in.uv,
    );
```
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 M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Self-shadowing on back side of meshes with "double sided lighting" enabled
6 participants