pbr shader cleanup #10105

robtfm · 2023-10-13T00:50:37Z

Objective

cleanup some pbr shader code. improve shader stage io consistency and make pbr.wgsl (probably many people's first foray into bevy shader code) a little more human-readable. also fix a couple of small issues with deferred rendering.

Solution

mesh_vertex_output:

rename to forward_io (to align with prepass_io)
rename MeshVertexOutput to VertexOutput (to align with prepass_io)
move Vertex from mesh.wgsl into here (to align with prepass_io)

prepass_io:

remove FragmentInput, use VertexOutput directly (to align with forward_io)
rename VertexOutput::clip_position to position (to align with forward_io)

pbr.wgsl:

restructure so we don't need #ifdefs on the actual entrypoint, use VertexOutput and FragmentOutput in all cases and use #ifdefs to import the right struct definitions.
rearrange to make the flow clearer
move alpha_discard up from pbr_functions::pbr to avoid needing to call it on some branches and not others
add a bunch of comments

deferred_lighting:

move ssao into the !unlit block to reflect forward behaviour correctly
fix compile error with deferred + premultiply_alpha

Migration Guide

in custom material shaders:

pbr_functions::pbr no longer calls to pbr_functions::alpha_discard. if you were using the pbr function in a custom shader with alpha mask mode you now also need to call alpha_discard manually
rename imports of bevy_pbr::mesh_vertex_output to bevy_pbr::forward_io
rename instances of MeshVertexOutput to VertexOutput

in custom material prepass shaders:

rename instances of VertexOutput::clip_position to VertexOutput::position

crates/bevy_pbr/src/deferred/deferred_lighting.wgsl

examples/3d/deferred_rendering.rs

DGriffin91 · 2023-10-13T02:01:16Z

crates/bevy_pbr/src/render/pbr.wgsl

    }

+    // apply alpha discard
+    // -------------------
+    // note even though this is based on the unlit color, it must be done after all texture samples for uniform control flow


I don't think this is really just for unlit as it also allows lit alpha mask materials to discard. ~~I'm not sure why it wasn't previously used in both conditions, but I think that was probably a mistake.~~ Ahh, I see now that it discards at the top of fn pbr() for lit materials.

it discards at the top of fn pbr() for lit materials

it used to, yes

The discard should really be done as soon as possible otherwise there is unnecessary fragment shader work being done. We should instead calculate the uv gradient in uniform control flow and pass it into later sampling functions.

Yes. it is the same place that it was though (plus or minus a couple of assignments). I just want to get this done so I can update the other 0.12 prs though

superdump

I still need to review the changes to pbr.wgsl in more detail in a second pass. It does look much cleaner if it is all correct and consolidating vertex output and fragment input is a good thing for sure.

crates/bevy_pbr/src/render/forward_io.wgsl

crates/bevy_pbr/src/render/pbr.wgsl

IceSentry

LGTM

I tested a bunch of examples and they all seemed to work as expected and I like the changes overall

# Objective #10105 changed the ssao input color from the material base color to white. i can't actually see a difference in the example but there should be one in some cases. ## Solution change it back.

# Objective - The refactoring in #10105 missed including the frag_coord and normal in pbr_input. ## Solution - Add them back

# Objective cleanup some pbr shader code. improve shader stage io consistency and make pbr.wgsl (probably many people's first foray into bevy shader code) a little more human-readable. also fix a couple of small issues with deferred rendering. ## Solution mesh_vertex_output: - rename to forward_io (to align with prepass_io) - rename `MeshVertexOutput` to `VertexOutput` (to align with prepass_io) - move `Vertex` from mesh.wgsl into here (to align with prepass_io) prepass_io: - remove `FragmentInput`, use `VertexOutput` directly (to align with forward_io) - rename `VertexOutput::clip_position` to `position` (to align with forward_io) pbr.wgsl: - restructure so we don't need `#ifdefs` on the actual entrypoint, use VertexOutput and FragmentOutput in all cases and use #ifdefs to import the right struct definitions. - rearrange to make the flow clearer - move alpha_discard up from `pbr_functions::pbr` to avoid needing to call it on some branches and not others - add a bunch of comments deferred_lighting: - move ssao into the `!unlit` block to reflect forward behaviour correctly - fix compile error with deferred + premultiply_alpha ## Migration Guide in custom material shaders: - `pbr_functions::pbr` no longer calls to `pbr_functions::alpha_discard`. if you were using the `pbr` function in a custom shader with alpha mask mode you now also need to call alpha_discard manually - rename imports of `bevy_pbr::mesh_vertex_output` to `bevy_pbr::forward_io` - rename instances of `MeshVertexOutput` to `VertexOutput` in custom material prepass shaders: - rename instances of `VertexOutput::clip_position` to `VertexOutput::position`

# Objective bevyengine#10105 changed the ssao input color from the material base color to white. i can't actually see a difference in the example but there should be one in some cases. ## Solution change it back.

# Objective - The refactoring in bevyengine#10105 missed including the frag_coord and normal in pbr_input. ## Solution - Add them back

# Objective cleanup some pbr shader code. improve shader stage io consistency and make pbr.wgsl (probably many people's first foray into bevy shader code) a little more human-readable. also fix a couple of small issues with deferred rendering. ## Solution mesh_vertex_output: - rename to forward_io (to align with prepass_io) - rename `MeshVertexOutput` to `VertexOutput` (to align with prepass_io) - move `Vertex` from mesh.wgsl into here (to align with prepass_io) prepass_io: - remove `FragmentInput`, use `VertexOutput` directly (to align with forward_io) - rename `VertexOutput::clip_position` to `position` (to align with forward_io) pbr.wgsl: - restructure so we don't need `#ifdefs` on the actual entrypoint, use VertexOutput and FragmentOutput in all cases and use #ifdefs to import the right struct definitions. - rearrange to make the flow clearer - move alpha_discard up from `pbr_functions::pbr` to avoid needing to call it on some branches and not others - add a bunch of comments deferred_lighting: - move ssao into the `!unlit` block to reflect forward behaviour correctly - fix compile error with deferred + premultiply_alpha ## Migration Guide in custom material shaders: - `pbr_functions::pbr` no longer calls to `pbr_functions::alpha_discard`. if you were using the `pbr` function in a custom shader with alpha mask mode you now also need to call alpha_discard manually - rename imports of `bevy_pbr::mesh_vertex_output` to `bevy_pbr::forward_io` - rename instances of `MeshVertexOutput` to `VertexOutput` in custom material prepass shaders: - rename instances of `VertexOutput::clip_position` to `VertexOutput::position`

# Objective bevyengine#10105 changed the ssao input color from the material base color to white. i can't actually see a difference in the example but there should be one in some cases. ## Solution change it back.

# Objective - The refactoring in bevyengine#10105 missed including the frag_coord and normal in pbr_input. ## Solution - Add them back

robtfm added 3 commits October 13, 2023 01:37

shader cleanup

6067090

fmt

40f329c

Merge remote-tracking branch 'upstream/main' into shader-cleanup

f60b7cd

DGriffin91 reviewed Oct 13, 2023

View reviewed changes

crates/bevy_pbr/src/deferred/deferred_lighting.wgsl Outdated Show resolved Hide resolved

DGriffin91 reviewed Oct 13, 2023

View reviewed changes

examples/3d/deferred_rendering.rs Outdated Show resolved Hide resolved

DGriffin91 reviewed Oct 13, 2023

View reviewed changes

IceSentry mentioned this pull request Oct 13, 2023

Fixed: Use VertexOuput instead of FragmentInput in prepass.wgsl #10009

Closed

superdump reviewed Oct 13, 2023

View reviewed changes

crates/bevy_pbr/src/render/forward_io.wgsl Outdated Show resolved Hide resolved

crates/bevy_pbr/src/render/forward_io.wgsl Outdated Show resolved Hide resolved

review comments

c7ba672

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 labels Oct 13, 2023

superdump reviewed Oct 13, 2023

View reviewed changes

crates/bevy_pbr/src/render/pbr.wgsl Show resolved Hide resolved

superdump added this to the 0.12 milestone Oct 13, 2023

superdump approved these changes Oct 13, 2023

View reviewed changes

IceSentry approved these changes Oct 13, 2023

View reviewed changes

IceSentry 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 13, 2023

superdump added this pull request to the merge queue Oct 13, 2023

Merged via the queue into bevyengine:main with commit 979c409 Oct 13, 2023
24 checks passed

robtfm mentioned this pull request Oct 14, 2023

ssao use unlit_color instead of white #10117

Merged

DGriffin91 mentioned this pull request Oct 16, 2023

Fix unlit missing parameters #10144

Merged

github-merge-queue bot pushed a commit that referenced this pull request Oct 16, 2023

Fix unlit missing parameters (#10144)

490699c

# Objective - The refactoring in #10105 missed including the frag_coord and normal in pbr_input. ## Solution - Add them back

DGriffin91 mentioned this pull request Oct 17, 2023

allow extensions to StandardMaterial #7820

Merged

ameknite pushed a commit to ameknite/bevy that referenced this pull request Nov 6, 2023

Fix unlit missing parameters (bevyengine#10144)

59b7ce4

# Objective - The refactoring in bevyengine#10105 missed including the frag_coord and normal in pbr_input. ## Solution - Add them back

rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024

Fix unlit missing parameters (bevyengine#10144)

b400116

# Objective - The refactoring in bevyengine#10105 missed including the frag_coord and normal in pbr_input. ## Solution - Add them back

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pbr shader cleanup #10105

pbr shader cleanup #10105

robtfm commented Oct 13, 2023 •

edited

Loading

DGriffin91 Oct 13, 2023 •

edited

Loading

robtfm Oct 13, 2023

superdump Oct 13, 2023

robtfm Oct 13, 2023

superdump left a comment

IceSentry left a comment

pbr shader cleanup #10105

pbr shader cleanup #10105

Conversation

robtfm commented Oct 13, 2023 • edited Loading

Objective

Solution

Migration Guide

DGriffin91 Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

robtfm Oct 13, 2023

Choose a reason for hiding this comment

superdump Oct 13, 2023

Choose a reason for hiding this comment

robtfm Oct 13, 2023

Choose a reason for hiding this comment

superdump left a comment

Choose a reason for hiding this comment

IceSentry left a comment

Choose a reason for hiding this comment

robtfm commented Oct 13, 2023 •

edited

Loading

DGriffin91 Oct 13, 2023 •

edited

Loading