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

Switch normal mapping to shader flag instead of shaderdef #8106

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

NotAFile
Copy link
Contributor

Objective

  • Make the normal mapping functionality reusable across shaders by making it a flag, just like the other textures
  • Allow using your own texture locations instead of the hardcoded pbr bindings, like with other textures.

Solution

  • The #ifdef calls are replaced by if statements (does not measurably impact performance, uniform branches are ~free on reasonably modern hardware)
  • The texture sampling is moved out of apply_normal_mapping into the shader main to match the other textures
  • All examples I could find were checked to make sure they still render identically

cc @superdump


Changelog

Changed

  • The PBR normal mapping functions can now work on custom normal map data

Migration Guide

If you have custom shaders that use apply_normal_mapping, prepare_world_normal, or #define STANDARDMATERIAL_NORMAL_MAP, they must be adjusted as follows:

Pass the material flags directly instead of only checking for double sidedness

pbr_input.world_normal = prepare_world_normal(
    in.world_normal,
    in.is_front,
    material.flags,
);

replace #ifdef STANDARDMATERIAL_NORMAL_MAP with flag checks

if (material.flags & STANDARD_MATERIAL_FLAGS_NORMAL_MAP_TEXTURE_BIT) != 0u {
    // your code here
}

When normal mapping is used, sample the texture yourself.

if (material.flags & STANDARD_MATERIAL_FLAGS_NORMAL_MAP_TEXTURE_BIT) != 0u {
    // NOTE: Do NOT normalize this value before applying normal mapping
    // http://www.mikktspace.com/
    let tangent_normal = textureSample(normal_map_texture, normal_map_sampler, in.uv).rgb;
    N = apply_normal_mapping(
        material.flags,
        pbr_input.world_normal,
        in.world_tangent,
        tangent_normal,
    );
}

If you are not using a normal map, simply replace the call to apply_normal_mapping with normalize

pbr_input.N = normalize(pbr_input.world_normal);

The original wasn't actually branchless, and the funky math just
confused the compiler into emitting *more* code.
This allows custom shaders to modify the source of the normal map while
reusing the normal mapping calculations
This better align with the other types of textures and makes it easier
to reuse the normal mapping function in other shaders.
@github-actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@james7132 james7132 added A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Mar 22, 2023
@james7132 james7132 requested a review from superdump March 22, 2023 18:31
@james7132 james7132 added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Mar 22, 2023
Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

I think this is an improvement. The current apply_normal_mapping function is odd and requires a lot of #ifdef conditionals at the function call site. Why? Well, mostly to avoid the user a normalize(pbr_input.world_normal). This reduces complexity a fair bit.

@NotAFile
Copy link
Contributor Author

Oops sorry for that nicopap. I should resolve the conflicts first.

@alice-i-cecile alice-i-cecile removed this from the 0.12 milestone Sep 29, 2023
@tychedelia tychedelia added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Aug 16, 2024
@NotAFile
Copy link
Contributor Author

I feel like this one is behind enough at this point that the conflicts are bigger than the actual work. I'd probably put this one up for adoption or turn it into an issue again to help the rendering backlog.

@JMS55 JMS55 removed their request for review September 11, 2024 05:37
@BenjaminBrienen BenjaminBrienen added S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 31, 2024
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-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Adopt-Me The original PR author has no intent to complete this work. Pick me up!
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

7 participants