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

Gate diffuse and specular transmission behind shader defs #11627

Merged
merged 5 commits into from
Feb 2, 2024

Conversation

coreh
Copy link
Contributor

@coreh coreh commented Jan 31, 2024

Objective

Solution

  • When implementing specular and diffuse transmission, I inadvertently introduced a performance regression. On high-end hardware it is barely noticeable, but for lower-end hardware it can be pretty brutal. If I understand it correctly, this is likely due to use of masking by the GPU to implement control flow, which means that you still pay the price for the branches you don't take;
  • To avoid that, this PR introduces new shader defs (controlled via StandardMaterialKey) that conditionally include the transmission logic, that way the shader code for both types of transmission isn't even sent to the GPU if you're not using them;
  • This PR also renames STANDARDMATERIAL_NORMAL_MAP to STANDARD_MATERIAL_NORMAL_MAP for consistency with the naming convention used elsewhere in the codebase. (Drive-by fix)

Changelog

  • Added new shader defs, set when using transmission in the StandardMaterial:
    • STANDARD_MATERIAL_SPECULAR_TRANSMISSION;
    • STANDARD_MATERIAL_DIFFUSE_TRANSMISSION;
    • STANDARD_MATERIAL_SPECULAR_OR_DIFFUSE_TRANSMISSION.
  • Fixed performance regression caused by the introduction of transmission, by gating transmission shader logic behind the newly introduced shader defs;
  • Renamed STANDARDMATERIAL_NORMAL_MAP to STANDARD_MATERIAL_NORMAL_MAP for consistency;

Migration Guide

  • If you were using #ifdef STANDARDMATERIAL_NORMAL_MAP on your shader code, make sure to update the name to STANDARD_MATERIAL_NORMAL_MAP; (with an underscore between STANDARD and MATERIAL)

@coreh coreh changed the title Gate Diffuse and Specular transmission behind shader defs Gate diffuse and specular transmission behind shader defs Jan 31, 2024
@@ -43,7 +43,7 @@ fn fragment(
double_sided,
is_front,
#ifdef VERTEX_TANGENTS
#ifdef STANDARDMATERIAL_NORMAL_MAP
#ifdef STANDARD_MATERIAL_NORMAL_MAP
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a small drive-by fix for consistency with the instances of STANDARD_MATERIAL in the codebase.

Comment on lines 216 to 217
#ifdef STANDARD_MATERIAL_DIFFUSE_TRANSMISSION
if diffuse_transmission > 0.0 {
Copy link
Member

Choose a reason for hiding this comment

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

the shader def being present should mean it's > 0.0, could we remove the if from the shader? (same for the others)

Copy link
Contributor Author

@coreh coreh Feb 1, 2024

Choose a reason for hiding this comment

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

Hmm, we could, but with the use of diffuse_transmission_texture and specular_transmission_texture it's possible for the per-fragment value of diffuse_transmission and specular_transmission to be 0, even when the shader def is present. In those cases without the if we'd do a bunch of computations or texture reads that would be immediately discarded.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure how much that really matters. I suspect the overhead of the if statement would be worse than any potential extra work on average. It's not a big deal either way, but it's probably worth removing still.

Copy link
Member

@mockersf mockersf left a comment

Choose a reason for hiding this comment

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

would be nice to get confirmation from someone with the issue on android

@alice-i-cecile alice-i-cecile added this to the 0.13 milestone Jan 31, 2024
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Performance A change motivated by improving speed, memory usage or compile times labels Jan 31, 2024
Comment on lines 216 to 217
#ifdef STANDARD_MATERIAL_DIFFUSE_TRANSMISSION
if diffuse_transmission > 0.0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure how much that really matters. I suspect the overhead of the if statement would be worse than any potential extra work on average. It's not a big deal either way, but it's probably worth removing still.

@coreh
Copy link
Contributor Author

coreh commented Feb 2, 2024

I'm not sure how much that really matters. I suspect the overhead of the if statement would be worse than any potential extra work on average. It's not a big deal either way, but it's probably worth removing still.

Okay, after thinking about it for a bit, I ended up being convinced that it will be a rare enough situation (having a texture to control the transmission levels, and having the majority of it not be transmissive) that's probably not worth it to give the much more common scenario a small penalty every time just for this unlikely benefit. Removed the if statements.

Since that affects indentation, it might make sense to review this with "hide whitespace changes" on, otherwise it looks like a much bigger change than it really is.

@JMS55 JMS55 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 2, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 2, 2024
Merged via the queue into bevyengine:main with commit 91c467e Feb 2, 2024
24 checks passed
tjamaan pushed a commit to tjamaan/bevy that referenced this pull request Feb 6, 2024
…#11627)

# Objective

- Address bevyengine#10338

## Solution

- When implementing specular and diffuse transmission, I inadvertently
introduced a performance regression. On high-end hardware it is barely
noticeable, but **for lower-end hardware it can be pretty brutal**. If I
understand it correctly, this is likely due to use of masking by the GPU
to implement control flow, which means that you still pay the price for
the branches you don't take;
- To avoid that, this PR introduces new shader defs (controlled via
`StandardMaterialKey`) that conditionally include the transmission
logic, that way the shader code for both types of transmission isn't
even sent to the GPU if you're not using them;
- This PR also renames ~~`STANDARDMATERIAL_NORMAL_MAP`~~ to
`STANDARD_MATERIAL_NORMAL_MAP` for consistency with the naming
convention used elsewhere in the codebase. (Drive-by fix)

---

## Changelog

- Added new shader defs, set when using transmission in the
`StandardMaterial`:
  - `STANDARD_MATERIAL_SPECULAR_TRANSMISSION`;
  - `STANDARD_MATERIAL_DIFFUSE_TRANSMISSION`;
  - `STANDARD_MATERIAL_SPECULAR_OR_DIFFUSE_TRANSMISSION`.
- Fixed performance regression caused by the introduction of
transmission, by gating transmission shader logic behind the newly
introduced shader defs;
- Renamed ~~`STANDARDMATERIAL_NORMAL_MAP`~~ to
`STANDARD_MATERIAL_NORMAL_MAP` for consistency;

## Migration Guide

- If you were using `#ifdef STANDARDMATERIAL_NORMAL_MAP` on your shader
code, make sure to update the name to `STANDARD_MATERIAL_NORMAL_MAP`;
(with an underscore between `STANDARD` and `MATERIAL`)
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-Performance A change motivated by improving speed, memory usage or compile times S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants