bevy icon indicating copy to clipboard operation
bevy copied to clipboard
verified publisher icon bevyengine

bevy_pbr: Fix incorrect and unnecessary normal-mapping code

Open superdump opened this issue 3 years ago • 8 comments

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

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

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:

    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,
    );

superdump avatar Aug 22 '22 15:08 superdump

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.

superdump avatar Aug 22 '22 16:08 superdump

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."

rparrett avatar Aug 22 '22 16:08 rparrett

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 avatar Aug 22 '22 16:08 superdump

@rparrett there you go. :)

superdump avatar Aug 22 '22 18:08 superdump

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.

rparrett avatar Aug 22 '22 18:08 rparrett

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.

djeedai avatar Aug 31 '22 17:08 djeedai

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 avatar Sep 01 '22 21:09 superdump

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 avatar Sep 15 '22 22:09 superdump

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 avatar Nov 03 '22 16:11 superdump

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

superdump avatar Nov 03 '22 16:11 superdump

Makes sense to me!

cart avatar Nov 03 '22 20:11 cart

bors r+

cart avatar Nov 03 '22 20:11 cart

Pull request successfully merged into main.

Build succeeded:

bors[bot] avatar Nov 03 '22 20:11 bors[bot]