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

*_PREPASS Shader Def Cleanup #10136

Merged
merged 6 commits into from
Oct 17, 2023
Merged

*_PREPASS Shader Def Cleanup #10136

merged 6 commits into from
Oct 17, 2023

Conversation

coreh
Copy link
Contributor

@coreh coreh commented Oct 16, 2023

Objective

  • This PR aims to make the various *_PREPASS shader defs we have (NORMAL_PREPASS, DEPTH_PREPASS, MOTION_VECTORS_PREPASS AND DEFERRED_PREPASS) easier to use and understand:
    • So that their meaning is now consistent across all contexts; (“prepass X is enabled for the current view”)
    • So that they're also consistently set across all contexts.
  • It also aims to enable us to (with a follow up PR) to conditionally gate the BindGroupEntry and BindGroupLayoutEntry items associated with these prepasses, saving us up to 4 texture slots in WebGL (currently globally limited to 16 per shader, regardless of bind groups)

Solution

  • We now consistently set these from PrepassPipeline, the MeshPipeline and the DeferredLightingPipeline, we also set their MeshPipelineKeys;
  • We introduce PREPASS_PIPELINE, MESH_PIPELINE and DEFERRED_LIGHTING_PIPELINE that can be used to detect where the code is running, without overloading the meanings of the prepass shader defs;
  • We also gate the WGSL functions in bevy_pbr::prepass_utils with #ifdefs for their respective shader defs, so that shader code can provide a fallback whenever they're not available.

Comparison

Before

Shader Def PrepassPipeline MeshPipeline DeferredLightingPipeline
NORMAL_PREPASS Yes No No
DEPTH_PREPASS Yes No No
MOTION_VECTORS_PREPASS Yes No No
DEFERRED_PREPASS Yes No No
View Key PrepassPipeline MeshPipeline DeferredLightingPipeline
NORMAL_PREPASS Yes Yes No
DEPTH_PREPASS Yes No No
MOTION_VECTORS_PREPASS Yes No No
DEFERRED_PREPASS Yes Yes* No

* Accidentally was being set twice, once with only deferred_prepass.is_some() as a condition,
and once with deferred_p repass.is_some() && !forward as a condition.

After

Shader Def PrepassPipeline MeshPipeline DeferredLightingPipeline
NORMAL_PREPASS Yes Yes Yes
DEPTH_PREPASS Yes Yes Yes
MOTION_VECTORS_PREPASS Yes Yes Yes
DEFERRED_PREPASS Yes Yes Unconditionally
PREPASS_PIPELINE Unconditionally No No
MESH_PIPELINE No Unconditionally No
DEFERRED_LIGHTING_PIPELINE No No Unconditionally
View Key PrepassPipeline MeshPipeline DeferredLightingPipeline
NORMAL_PREPASS Yes Yes Yes
DEPTH_PREPASS Yes Yes Yes
MOTION_VECTORS_PREPASS Yes Yes Yes
DEFERRED_PREPASS Yes Yes Unconditionally

Changelog

  • Cleaned up WGSL *_PREPASS shader defs so they're now consistently used everywhere;
  • Introduced PREPASS_PIPELINE, MESH_PIPELINE and DEFERRED_LIGHTING_PIPELINE WGSL shader defs for conditionally compiling logic based the current pipeline;
  • WGSL functions from bevy_pbr::prepass_utils are now guarded with #ifdef based on the currently enabled prepasses;

Migration Guide

  • When using functions from bevy_pbr::prepass_utils (prepass_depth(), prepass_normal(), prepass_motion_vector()) in contexts where these prepasses might be disabled, you should now wrap your calls with the appropriate #ifdef guards, (#ifdef DEPTH_PREPASS, #ifdef NORMAL_PREPASS, #ifdef MOTION_VECTOR_PREPASS) providing fallback logic where applicable.

@coreh coreh mentioned this pull request Oct 16, 2023
20 tasks
@@ -2,7 +2,7 @@

#import bevy_pbr::mesh_view_bindings as view_bindings

#ifndef DEPTH_PREPASS
#ifdef DEPTH_PREPASS
Copy link
Contributor

Choose a reason for hiding this comment

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

If this works I'm pretty sure that means we can just remove the ifdef entirely no?

The reason this was an ifndef is because the prepass_utils ended up being called during the prepass for reasons I don't remember and this broke everything.

I tried it locally and removing all them seems to still work as expected

Copy link
Member

Choose a reason for hiding this comment

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

Leaving these in might make this safer to use? It would prevent people from using these functions when a given prepass isn't enabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

and will be required if making the bindings conditional (to free texture slots).

@IceSentry
Copy link
Contributor

IceSentry commented Oct 16, 2023

The migration guide is wrong, you inverted the _PREPASS suffix for the shader defs. The function names are correct.

I'm also not sure about saying people should wrap their function calls. In most cases they probably don't need to if it already worked correctly for them.

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Oct 16, 2023
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

These changes make a lot of sense to me. I'm also not getting failures here for the potential deferred pbr.wgsl read/write binding issue.

Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

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

LGTM. I agree with @IceSentry's comments, but they are also not blocking issues in my opinion. It seems @robtfm and @cart agree, given their approvals.

@superdump
Copy link
Contributor

@IceSentry confirmed on Discord that they're fine with merging this. pushes the button

@superdump superdump added this pull request to the merge queue Oct 16, 2023
@cart cart removed this pull request from the merge queue due to a manual request Oct 16, 2023
Co-authored-by: IceSentry <IceSentry@users.noreply.github.com>
@alice-i-cecile alice-i-cecile 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 Oct 16, 2023
@alice-i-cecile alice-i-cecile added this to the 0.12 milestone Oct 16, 2023
@cart
Copy link
Member

cart commented Oct 17, 2023

Just add the Has change suggested by @IceSentry, applied the spelling nit, and fixed the migration guide!

@cart cart enabled auto-merge October 17, 2023 00:04
@coreh
Copy link
Contributor Author

coreh commented Oct 17, 2023

Just add the Has change suggested by @IceSentry, applied the spelling nit, and fixed the migration guide!

Thanks!

When using functions from bevy_pbr::prepass_utils (prepass_depth(), prepass_normal(), prepass_motion_vector()) in contexts where these prepasses might be disabled, you should now wrap your calls with the appropriate #ifdef guards, (#ifdef DEPTH_PREPASS, #ifdef NORMAL_PREPASS, #ifdef MOTION_VECTOR_PREPASS) providing fallback logic where applicable.

Added the phrase above to clarify it's not mandatory, but recommended in case your shader code might run without the prepass.

@cart cart added this pull request to the merge queue Oct 17, 2023
Merged via the queue into bevyengine:main with commit 5733d24 Oct 17, 2023
21 checks passed
ameknite pushed a commit to ameknite/bevy that referenced this pull request Nov 6, 2023
# Objective

- This PR aims to make the various `*_PREPASS` shader defs we have
(`NORMAL_PREPASS`, `DEPTH_PREPASS`, `MOTION_VECTORS_PREPASS` AND
`DEFERRED_PREPASS`) easier to use and understand:
- So that their meaning is now consistent across all contexts; (“prepass
X is enabled for the current view”)
  - So that they're also consistently set across all contexts.
- It also aims to enable us to (with a follow up PR) to conditionally
gate the `BindGroupEntry` and `BindGroupLayoutEntry` items associated
with these prepasses, saving us up to 4 texture slots in WebGL
(currently globally limited to 16 per shader, regardless of bind groups)

## Solution

- We now consistently set these from `PrepassPipeline`, the
`MeshPipeline` and the `DeferredLightingPipeline`, we also set their
`MeshPipelineKey`s;
- We introduce `PREPASS_PIPELINE`, `MESH_PIPELINE` and
`DEFERRED_LIGHTING_PIPELINE` that can be used to detect where the code
is running, without overloading the meanings of the prepass shader defs;
- We also gate the WGSL functions in `bevy_pbr::prepass_utils` with
`#ifdef`s for their respective shader defs, so that shader code can
provide a fallback whenever they're not available.
- This allows us to conditionally include the bindings for these prepass
textures (My next PR, which will hopefully unblock bevyengine#8015)
- @robtfm mentioned [these were being used to prevent accessing the same
binding as read/write in the
prepass](https://discord.com/channels/691052431525675048/743663924229963868/1163270458393759814),
however even after reversing the `#ifndef`s I had no issues running the
code, so perhaps the compiler is already smart enough even without tree
shaking to know they're not being used, thanks to `#ifdef
PREPASS_PIPELINE`?

## Comparison

### Before

| Shader Def | `PrepassPipeline` | `MeshPipeline` |
`DeferredLightingPipeline` |
| ------------------------ | ----------------- | -------------- |
-------------------------- |
| `NORMAL_PREPASS` | Yes | No | No |
| `DEPTH_PREPASS` | Yes | No | No |
| `MOTION_VECTORS_PREPASS` | Yes | No | No |
| `DEFERRED_PREPASS` | Yes | No | No |

| View Key | `PrepassPipeline` | `MeshPipeline` |
`DeferredLightingPipeline` |
| ------------------------ | ----------------- | -------------- |
-------------------------- |
| `NORMAL_PREPASS` | Yes | Yes | No |
| `DEPTH_PREPASS` | Yes | No | No |
| `MOTION_VECTORS_PREPASS` | Yes | No | No |
| `DEFERRED_PREPASS` | Yes | Yes\* | No |

\* Accidentally was being set twice, once with only
`deferred_prepass.is_some()` as a condition,
and once with `deferred_p repass.is_some() && !forward` as a condition.

### After

| Shader Def | `PrepassPipeline` | `MeshPipeline` |
`DeferredLightingPipeline` |
| ---------------------------- | ----------------- | --------------- |
-------------------------- |
| `NORMAL_PREPASS` | Yes | Yes | Yes |
| `DEPTH_PREPASS` | Yes | Yes | Yes |
| `MOTION_VECTORS_PREPASS` | Yes | Yes | Yes |
| `DEFERRED_PREPASS` | Yes | Yes | Unconditionally |
| `PREPASS_PIPELINE` | Unconditionally | No | No |
| `MESH_PIPELINE` | No | Unconditionally | No |
| `DEFERRED_LIGHTING_PIPELINE` | No | No | Unconditionally |

| View Key | `PrepassPipeline` | `MeshPipeline` |
`DeferredLightingPipeline` |
| ------------------------ | ----------------- | -------------- |
-------------------------- |
| `NORMAL_PREPASS` | Yes | Yes | Yes |
| `DEPTH_PREPASS` | Yes | Yes | Yes |
| `MOTION_VECTORS_PREPASS` | Yes | Yes | Yes |
| `DEFERRED_PREPASS` | Yes | Yes | Unconditionally |

---

## Changelog

- Cleaned up WGSL `*_PREPASS` shader defs so they're now consistently
used everywhere;
- Introduced `PREPASS_PIPELINE`, `MESH_PIPELINE` and
`DEFERRED_LIGHTING_PIPELINE` WGSL shader defs for conditionally
compiling logic based the current pipeline;
- WGSL functions from `bevy_pbr::prepass_utils` are now guarded with
`#ifdef` based on the currently enabled prepasses;

## Migration Guide

- When using functions from `bevy_pbr::prepass_utils`
(`prepass_depth()`, `prepass_normal()`, `prepass_motion_vector()`) in
contexts where these prepasses might be disabled, you should now wrap
your calls with the appropriate `#ifdef` guards, (`#ifdef
DEPTH_PREPASS`, `#ifdef NORMAL_PREPASS`, `#ifdef MOTION_VECTOR_PREPASS`)
providing fallback logic where applicable.

---------

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
Co-authored-by: IceSentry <IceSentry@users.noreply.github.com>
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

- This PR aims to make the various `*_PREPASS` shader defs we have
(`NORMAL_PREPASS`, `DEPTH_PREPASS`, `MOTION_VECTORS_PREPASS` AND
`DEFERRED_PREPASS`) easier to use and understand:
- So that their meaning is now consistent across all contexts; (“prepass
X is enabled for the current view”)
  - So that they're also consistently set across all contexts.
- It also aims to enable us to (with a follow up PR) to conditionally
gate the `BindGroupEntry` and `BindGroupLayoutEntry` items associated
with these prepasses, saving us up to 4 texture slots in WebGL
(currently globally limited to 16 per shader, regardless of bind groups)

## Solution

- We now consistently set these from `PrepassPipeline`, the
`MeshPipeline` and the `DeferredLightingPipeline`, we also set their
`MeshPipelineKey`s;
- We introduce `PREPASS_PIPELINE`, `MESH_PIPELINE` and
`DEFERRED_LIGHTING_PIPELINE` that can be used to detect where the code
is running, without overloading the meanings of the prepass shader defs;
- We also gate the WGSL functions in `bevy_pbr::prepass_utils` with
`#ifdef`s for their respective shader defs, so that shader code can
provide a fallback whenever they're not available.
- This allows us to conditionally include the bindings for these prepass
textures (My next PR, which will hopefully unblock bevyengine#8015)
- @robtfm mentioned [these were being used to prevent accessing the same
binding as read/write in the
prepass](https://discord.com/channels/691052431525675048/743663924229963868/1163270458393759814),
however even after reversing the `#ifndef`s I had no issues running the
code, so perhaps the compiler is already smart enough even without tree
shaking to know they're not being used, thanks to `#ifdef
PREPASS_PIPELINE`?

## Comparison

### Before

| Shader Def | `PrepassPipeline` | `MeshPipeline` |
`DeferredLightingPipeline` |
| ------------------------ | ----------------- | -------------- |
-------------------------- |
| `NORMAL_PREPASS` | Yes | No | No |
| `DEPTH_PREPASS` | Yes | No | No |
| `MOTION_VECTORS_PREPASS` | Yes | No | No |
| `DEFERRED_PREPASS` | Yes | No | No |

| View Key | `PrepassPipeline` | `MeshPipeline` |
`DeferredLightingPipeline` |
| ------------------------ | ----------------- | -------------- |
-------------------------- |
| `NORMAL_PREPASS` | Yes | Yes | No |
| `DEPTH_PREPASS` | Yes | No | No |
| `MOTION_VECTORS_PREPASS` | Yes | No | No |
| `DEFERRED_PREPASS` | Yes | Yes\* | No |

\* Accidentally was being set twice, once with only
`deferred_prepass.is_some()` as a condition,
and once with `deferred_p repass.is_some() && !forward` as a condition.

### After

| Shader Def | `PrepassPipeline` | `MeshPipeline` |
`DeferredLightingPipeline` |
| ---------------------------- | ----------------- | --------------- |
-------------------------- |
| `NORMAL_PREPASS` | Yes | Yes | Yes |
| `DEPTH_PREPASS` | Yes | Yes | Yes |
| `MOTION_VECTORS_PREPASS` | Yes | Yes | Yes |
| `DEFERRED_PREPASS` | Yes | Yes | Unconditionally |
| `PREPASS_PIPELINE` | Unconditionally | No | No |
| `MESH_PIPELINE` | No | Unconditionally | No |
| `DEFERRED_LIGHTING_PIPELINE` | No | No | Unconditionally |

| View Key | `PrepassPipeline` | `MeshPipeline` |
`DeferredLightingPipeline` |
| ------------------------ | ----------------- | -------------- |
-------------------------- |
| `NORMAL_PREPASS` | Yes | Yes | Yes |
| `DEPTH_PREPASS` | Yes | Yes | Yes |
| `MOTION_VECTORS_PREPASS` | Yes | Yes | Yes |
| `DEFERRED_PREPASS` | Yes | Yes | Unconditionally |

---

## Changelog

- Cleaned up WGSL `*_PREPASS` shader defs so they're now consistently
used everywhere;
- Introduced `PREPASS_PIPELINE`, `MESH_PIPELINE` and
`DEFERRED_LIGHTING_PIPELINE` WGSL shader defs for conditionally
compiling logic based the current pipeline;
- WGSL functions from `bevy_pbr::prepass_utils` are now guarded with
`#ifdef` based on the currently enabled prepasses;

## Migration Guide

- When using functions from `bevy_pbr::prepass_utils`
(`prepass_depth()`, `prepass_normal()`, `prepass_motion_vector()`) in
contexts where these prepasses might be disabled, you should now wrap
your calls with the appropriate `#ifdef` guards, (`#ifdef
DEPTH_PREPASS`, `#ifdef NORMAL_PREPASS`, `#ifdef MOTION_VECTOR_PREPASS`)
providing fallback logic where applicable.

---------

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
Co-authored-by: IceSentry <IceSentry@users.noreply.github.com>
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-Code-Quality A section of code that is hard to understand or change M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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.

6 participants