Always set PREPASS_FRAGMENT when frag shader being used in prepass. #21106

tychedelia · 2025-09-17T22:20:31Z

@ChristopherBiscardi was running into an issue where PREPASS_FRAGMENT was not being set when using a prepass fragment shader that only writes to depth. Previously, we were setting the PREPASS_FRAGMENT shader def when there are targets or emulate_unclipped_depth is true, but not when using a material with alpha masking. Instead, we should set the shader def unconditionally when using a fragment shader in the prepass.

github-actions · 2025-09-17T22:27:34Z

Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke!
You can review it at https://pixel-eagle.com/project/B04F67C0-C054-4A6F-92EC-F599FEC2FD1D?filter=PR-21106

If it's expected, please add the M-Deliberate-Rendering-Change label.

If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it.

github-actions · 2025-09-17T22:29:07Z

Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke!
You can review it at https://pixel-eagle.com/project/B04F67C0-C054-4A6F-92EC-F599FEC2FD1D?filter=PR-21106

If it's expected, please add the M-Deliberate-Rendering-Change label.

If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it.

ChristopherBiscardi · 2025-09-17T22:38:55Z

The prepass shaders I've been using have notably not been writing to depth in the shader, so I'm not sure this is a bug but rather a complex set of conditions that could use some additional documentation. Specifically:

What I seem to be running into is that PREPASS_FRAGMENT is used to control whether FragmentOutput exists or not. There are then additional shaderdefs used to control whether the FragmentOutput includes the normal, motion vector, deferred, and unclipped depth outputs. The normal/motion vector write to the prepass textures that are controlled by placing a NormalPrepass/MotionVectorPrepass on the camera.

The shadows pipeline additionally uses the prepasspipeline unmodified, which doesn't set the PREPASS_FRAGMENT shader def.

So you need two branches in a prepass shader, one for potentially writing normal/motion/deferred/unclipped data, if any are defined to be used, and one for shadows. Shadows also differs in that it doesn't have a FragmentOutput on the entrypoint (and FragmentOutput won't even exist as a type).

This is additionally complicated through AlphaMode choice.

AlphaMode::Opaque and Multiply, won't run the shadows prepass, wheras Add, Blend, Mask, and Premultiplied will.
The NormalPrepass texture will additionally be written to if Mask is used, but not Add (for example). adding another set of options to think about.

So PREPASS_FRAGMENT does seem like a poor name, since it doesn't control whether the fragment shader is running, but rather controls whether any of the aforementioned outputs could be enabled. and is additionally being used to "force" the fragment shader to run, according to some comments in the source code.

Overall this seems like a bit of a documentation issue more than a bug, but might also benefit from some renaming/refactoring.

IceSentry · 2025-09-17T22:41:07Z

crates/bevy_pbr/src/prepass/mod.rs

        // is enabled, the material uses alpha cutoff values and doesn't rely on the standard
        // prepass shader, or we are emulating unclipped depth in the fragment shader.
        let fragment_required = !targets.is_empty()
+            // PERF: This line forces the "prepass fragment shader" to always run in


shouldn't that be above the line that pushes the shader def?

Looking at the issue and where it was located before, this is relevant to unclipped depth rather than the shader def being pushed generally, which is why I moved it here.

IceSentry

Not sure about the location of the perf comment but other than that LGTM

I like this approach better I think.

IceSentry · 2025-09-17T22:43:07Z

PREPASS_FRAGMENT was originally meant to mean "The prepass fragment shader is running", so I think it makes sense to just always define it if the frag shader is being used.

tychedelia · 2025-09-17T22:55:41Z

Interestingly, this does change rendering output as seen from pixel eagle. I'm not sure which is correct or what the concrete difference is on the helmet example. We should investigate further.

ChristopherBiscardi · 2025-09-17T22:56:17Z

yeah, with the knowledge that PREPASS_FRAGMENT was meant to let a fragment entrypoint exist, it makes sense that enabling it always if any prepass fragment shader version is running makes sense. That would also remove the branching

This PR will enable the meshlet fragment prepass:

bevy/crates/bevy_pbr/src/meshlet/meshlet_mesh_material.wgsl

Line 26 in 4b138af

#ifdef PREPASS_FRAGMENT

and will require a small refactor in bevy_pbr/src/render/pbr_prepass.wgsl, since the branch would be removed:

bevy/crates/bevy_pbr/src/render/pbr_prepass.wgsl

Lines 23 to 151 in 4b138af

    
           #ifdef PREPASS_FRAGMENT 
        
           @fragment 
        
           fn fragment( 
        
           #ifdef MESHLET_MESH_MATERIAL_PASS 
        
               @builtin(position) frag_coord: vec4<f32>, 
        
           #else 
        
               in: prepass_io::VertexOutput, 
        
               @builtin(front_facing) is_front: bool, 
        
           #endif 
        
           ) -> prepass_io::FragmentOutput { 
        
           #ifdef MESHLET_MESH_MATERIAL_PASS 
        
               let in = resolve_vertex_output(frag_coord); 
        
               let is_front = true; 
        
           #else   // MESHLET_MESH_MATERIAL_PASS 
        
           #ifdef BINDLESS 
        
               let slot = mesh[in.instance_index].material_and_lightmap_bind_group_slot & 0xffffu; 
        
               let flags = pbr_bindings::material_array[material_indices[slot].material].flags; 
        
               let uv_transform = pbr_bindings::material_array[material_indices[slot].material].uv_transform; 
        
           #else   // BINDLESS 
        
               let flags = pbr_bindings::material.flags; 
        
               let uv_transform = pbr_bindings::material.uv_transform; 
        
           #endif  // BINDLESS 
        
               // If we're in the crossfade section of a visibility range, conditionally 
        
               // discard the fragment according to the visibility pattern. 
        
           #ifdef VISIBILITY_RANGE_DITHER 
        
               pbr_functions::visibility_range_dither(in.position, in.visibility_range_dither); 
        
           #endif  // VISIBILITY_RANGE_DITHER 
        
               pbr_prepass_functions::prepass_alpha_discard(in); 
        
           #endif  // MESHLET_MESH_MATERIAL_PASS 
        
               var out: prepass_io::FragmentOutput; 
        
           #ifdef UNCLIPPED_DEPTH_ORTHO_EMULATION 
        
               out.frag_depth = in.unclipped_depth; 
        
           #endif // UNCLIPPED_DEPTH_ORTHO_EMULATION 
        
           #ifdef NORMAL_PREPASS 
        
               // NOTE: Unlit bit not set means == 0 is true, so the true case is if lit 
        
               if (flags & pbr_types::STANDARD_MATERIAL_FLAGS_UNLIT_BIT) == 0u { 
        
                   let double_sided = (flags & pbr_types::STANDARD_MATERIAL_FLAGS_DOUBLE_SIDED_BIT) != 0u; 
        
                   let world_normal = pbr_functions::prepare_world_normal( 
        
                       in.world_normal, 
        
                       double_sided, 
        
                       is_front, 
        
                   ); 
        
                   var normal = world_normal; 
        
           #ifdef VERTEX_UVS 
        
           #ifdef VERTEX_TANGENTS 
        
           #ifdef STANDARD_MATERIAL_NORMAL_MAP 
        
           // TODO: Transforming UVs mean we need to apply derivative chain rule for meshlet mesh material pass 
        
           #ifdef STANDARD_MATERIAL_NORMAL_MAP_UV_B 
        
                   let uv = (uv_transform * vec3(in.uv_b, 1.0)).xy; 
        
           #else 
        
                   let uv = (uv_transform * vec3(in.uv, 1.0)).xy; 
        
           #endif 
        
                   // Fill in the sample bias so we can sample from textures. 
        
                   var bias: SampleBias; 
        
           #ifdef MESHLET_MESH_MATERIAL_PASS 
        
                   bias.ddx_uv = in.ddx_uv; 
        
                   bias.ddy_uv = in.ddy_uv; 
        
           #else   // MESHLET_MESH_MATERIAL_PASS 
        
                   bias.mip_bias = view.mip_bias; 
        
           #endif  // MESHLET_MESH_MATERIAL_PASS 
        
                   let Nt = 
        
           #ifdef MESHLET_MESH_MATERIAL_PASS 
        
                       textureSampleGrad( 
        
           #else   // MESHLET_MESH_MATERIAL_PASS 
        
                       textureSampleBias( 
        
           #endif  // MESHLET_MESH_MATERIAL_PASS 
        
           #ifdef BINDLESS 
        
                           bindless_textures_2d[material_indices[slot].normal_map_texture], 
        
                           bindless_samplers_filtering[material_indices[slot].normal_map_sampler], 
        
           #else   // BINDLESS 
        
                           pbr_bindings::normal_map_texture, 
        
                           pbr_bindings::normal_map_sampler, 
        
           #endif  // BINDLESS 
        
                           uv, 
        
           #ifdef MESHLET_MESH_MATERIAL_PASS 
        
                           bias.ddx_uv, 
        
                           bias.ddy_uv, 
        
           #else   // MESHLET_MESH_MATERIAL_PASS 
        
                           bias.mip_bias, 
        
           #endif  // MESHLET_MESH_MATERIAL_PASS 
        
                       ).rgb; 
        
                   let TBN = pbr_functions::calculate_tbn_mikktspace(normal, in.world_tangent); 
        
                   normal = pbr_functions::apply_normal_mapping( 
        
                       flags, 
        
                       TBN, 
        
                       double_sided, 
        
                       is_front, 
        
                       Nt, 
        
                   ); 
        
           #endif  // STANDARD_MATERIAL_NORMAL_MAP 
        
           #endif  // VERTEX_TANGENTS 
        
           #endif  // VERTEX_UVS 
        
                   out.normal = vec4(normal * 0.5 + vec3(0.5), 1.0); 
        
               } else { 
        
                   out.normal = vec4(in.world_normal * 0.5 + vec3(0.5), 1.0); 
        
               } 
        
           #endif // NORMAL_PREPASS 
        
           #ifdef MOTION_VECTOR_PREPASS 
        
           #ifdef MESHLET_MESH_MATERIAL_PASS 
        
               out.motion_vector = in.motion_vector; 
        
           #else 
        
               out.motion_vector = pbr_prepass_functions::calculate_motion_vector(in.world_position, in.previous_world_position); 
        
           #endif 
        
           #endif 
        
               return out; 
        
           } 
        
           #else 
        
           @fragment 
        
           fn fragment(in: prepass_io::VertexOutput) { 
        
               pbr_prepass_functions::prepass_alpha_discard(in); 
        
           } 
        
           #endif // PREPASS_FRAGMENT

tychedelia · 2025-09-17T23:46:35Z

and will require a small refactor in bevy_pbr/src/render/pbr_prepass.wgsl, since the branch would be removed:

Okay, yup, this could be responsible for the change in the helmet output. Will double check.

ChristopherBiscardi · 2025-09-20T23:55:30Z

Actually structs can't be empty, so I think the branch is required?

This is from cargo run --example load_gltf

2025-09-20T23:51:13.837940Z ERROR bevy_render::render_resource::pipeline_cache: failed to process shader error:
\x1b[0m\x1b[1m\x1b[38;5;9merror\x1b[0m\x1b[1m: naga_oil bug, please file a report: composer failed to build a valid header: Type [6] 'bevy_pbr::prepass_io::FragmentOutput' is invalid\x1b[0m
   \x1b[0m\x1b[34m┌─\x1b[0m embedded://bevy_pbr/prepass/prepass_io.wgsl:82:1
   \x1b[0m\x1b[34m│\x1b[0m
\x1b[0m\x1b[34m82\x1b[0m \x1b[0m\x1b[34m│\x1b[0m \x1b[0m\x1b[31mstruct FragmentOutput {\x1b[0m
   \x1b[0m\x1b[34m│\x1b[0m \x1b[0m\x1b[31m^^^^^^^^^^^^^^^^^^^^^^^\x1b[0m \x1b[0m\x1b[31mnaga::ir::Type [6]\x1b[0m
   \x1b[0m\x1b[34m│\x1b[0m
   \x1b[0m\x1b[34m=\x1b[0m Structure types must have at least one member

Always set PREPASS_FRAGMENT when frag shader being used in prepass.

63d97ed

tychedelia added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 17, 2025

github-project-automation bot added this to Rendering Sep 17, 2025

Merge branch 'main' into prepass_shader_def

f0d4850

IceSentry reviewed Sep 17, 2025

View reviewed changes

IceSentry approved these changes Sep 17, 2025

View reviewed changes

tychedelia added S-Needs-Investigation This issue requires detective work to figure out what's going wrong and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 17, 2025

hukasu added S-Needs-SME Decision or review from an SME is required and removed S-Needs-Investigation This issue requires detective work to figure out what's going wrong labels Sep 17, 2025

tychedelia added S-Needs-Investigation This issue requires detective work to figure out what's going wrong and removed S-Needs-SME Decision or review from an SME is required labels Sep 17, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Always set PREPASS_FRAGMENT when frag shader being used in prepass. #21106

Always set PREPASS_FRAGMENT when frag shader being used in prepass. #21106

tychedelia commented Sep 17, 2025

Uh oh!

github-actions bot commented Sep 17, 2025

Uh oh!

github-actions bot commented Sep 17, 2025

Uh oh!

ChristopherBiscardi commented Sep 17, 2025

Uh oh!

IceSentry Sep 17, 2025

Uh oh!

tychedelia Sep 17, 2025

Uh oh!

IceSentry left a comment

Uh oh!

IceSentry commented Sep 17, 2025

Uh oh!

tychedelia commented Sep 17, 2025

Uh oh!

ChristopherBiscardi commented Sep 17, 2025

Uh oh!

tychedelia commented Sep 17, 2025

Uh oh!

ChristopherBiscardi commented Sep 20, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Always set PREPASS_FRAGMENT when frag shader being used in prepass. #21106

Are you sure you want to change the base?

Always set PREPASS_FRAGMENT when frag shader being used in prepass. #21106

Conversation

tychedelia commented Sep 17, 2025

Uh oh!

github-actions bot commented Sep 17, 2025

Uh oh!

github-actions bot commented Sep 17, 2025

Uh oh!

ChristopherBiscardi commented Sep 17, 2025

Uh oh!

IceSentry Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

tychedelia Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

IceSentry left a comment

Choose a reason for hiding this comment

Uh oh!

IceSentry commented Sep 17, 2025

Uh oh!

tychedelia commented Sep 17, 2025

Uh oh!

ChristopherBiscardi commented Sep 17, 2025

Uh oh!

tychedelia commented Sep 17, 2025

Uh oh!

ChristopherBiscardi commented Sep 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ChristopherBiscardi commented Sep 20, 2025 •

edited

Loading