Skip to content

Add prepass_reads_material flag in material that sets PREPASS_READS_M… #24716

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.

Sign up for GitHub

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

Open
VictorElHajj wants to merge 1 commit into
base: main
Choose a base branch
from
+33 −2
Open

Add prepass_reads_material flag in material that sets PREPASS_READS_M… #24716

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions crates/bevy_material/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ pub struct MaterialProperties {
pub shadows_enabled: bool,
/// Whether prepass is enabled for this material
pub prepass_enabled: bool,
/// Whether prepass needs to read the material
pub prepass_reads_material: bool,
}

impl MaterialProperties {
Expand Down
9 changes: 9 additions & 0 deletions crates/bevy_pbr/src/extended_material.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,11 @@ pub trait MaterialExtension: Asset + AsBindGroup + Clone + Sized {
true
}

#[inline]
fn prepass_reads_material() -> bool {
false
}

/// Returns this material's prepass vertex shader. If [`ShaderRef::Default`] is returned, the base material prepass vertex shader
/// will be used.
fn prepass_vertex_shader() -> ShaderRef {
Expand Down Expand Up @@ -355,6 +360,10 @@ impl<B: Material, E: MaterialExtension> Material for ExtendedMaterial<B, E> {
E::enable_shadows()
}

fn prepass_reads_material() -> bool {
E::prepass_reads_material()
}

fn prepass_vertex_shader() -> ShaderRef {
match E::prepass_vertex_shader() {
ShaderRef::Default => B::prepass_vertex_shader(),
Expand Down
7 changes: 7 additions & 0 deletions crates/bevy_pbr/src/material.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,11 @@ pub trait Material: Asset + AsBindGroup + Clone + Sized {
true
}

#[inline]
fn prepass_reads_material() -> bool {

@beicause beicause Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a way to do this automatically?

I suspect we can get whether the bind group is empty from the material, thus eliminating the need for this function.

@VictorElHajj VictorElHajj Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. The issue here is that the depth only prepass for shadow maps skips the material bind group for performance reasons, as when vertex displacement is not used the material is not needed to determine depth. But when custom vertex shaders are used the vertex prepass shader needs to access the material to match the vertex displacement.

So this flag essentially says to always include the material even in depth prepass, because the prepass needs to read it.

This isn't related to the material bind group being empty or not, but to prevent skipping the material bind group for prepasses that assume its not needed.

@VictorElHajj VictorElHajj Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Digging a little deeper, I think its impossible to know from the rust side which material bindings the prepass shader actually uses, so that is why the flag is needed. We can only know what the material provides, not what the shader uses.

@beicause beicause Jun 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Won't the resource bindings used by the shader match those from AsBindGroup?

@VictorElHajj VictorElHajj Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

AsBindGroup can't distinguish between a shader that has material bindings that is used in the prepass vertex shaders and a material that has bindings that is not used in the prepass vertex shader. Just because they exist doesnt mean they're used in that specific shader. (Could be used in the normal fragment shader for example but not the prepass vertex shader).

In the case where its not used we still want to keep the existing behaviour to speed up the depth prepass.

@beicause beicause Jun 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the current solution could only batch depth prepasses for materials that have a default prepass vertex shader and a default prepass fragment shader.

@VictorElHajj VictorElHajj Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we can only use the same material bind group layout. But that doesn't make sense - usually depth prepass doesn't need the material bind group.

This is why a optimization was introduced in 0.19 where the material bind group was skipped for the depth prepass, but its still needed in some cases, which is why I added this flag.

Im not sure what you want me to change?

@beicause beicause Jul 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See #24843

@VictorElHajj VictorElHajj Jul 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The drawback with that solution is that the performance gain is always lost work custom shaders, while avoiding a new flag. While this PR adds a new flag but still allows the performance gain for custom shaders if not set (by default)

@beicause beicause Jul 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The drawback with that solution is that the performance gain is always lost work custom shaders, while avoiding a new flag. While this PR adds a new flag but still allows the performance gain for custom shaders if not set (by default)

It's ture. But my point is:

  1. We shouldn't expose this setting. It's an internal detail and error-prone. Here correctness is more important than performance.
  2. In the future, we could allow using different materials for different passes, so you can have full control over the prepass bind groups.

false
}

/// Returns this material's prepass vertex shader. If [`ShaderRef::Default`] is returned, the default prepass vertex shader
/// will be used.
///
Expand Down Expand Up @@ -1690,6 +1695,7 @@ where

let shadows_enabled = M::enable_shadows();
let prepass_enabled = M::enable_prepass();
let prepass_reads_material = M::prepass_reads_material();

let draw_opaque_pbr = opaque_draw_functions.read().id::<DrawMaterial>();
let draw_alpha_mask_pbr = alpha_mask_draw_functions.read().id::<DrawMaterial>();
Expand Down Expand Up @@ -1790,6 +1796,7 @@ where
material_key,
shadows_enabled,
prepass_enabled,
prepass_reads_material,
}),
})
}
Expand Down
13 changes: 11 additions & 2 deletions crates/bevy_pbr/src/prepass/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,9 @@ impl SpecializedMeshPipeline for PrepassPipelineSpecializer {
}

fn is_depth_only_opaque_prepass(mesh_key: MeshPipelineKey) -> bool {
mesh_key.intersection(MeshPipelineKey::ALL_PREPASS_BITS) == MeshPipelineKey::DEPTH_PREPASS
!mesh_key.contains(MeshPipelineKey::PREPASS_READS_MATERIAL)
&& mesh_key.intersection(MeshPipelineKey::ALL_PREPASS_BITS)
== MeshPipelineKey::DEPTH_PREPASS
}

impl PrepassPipeline {
Expand Down Expand Up @@ -421,7 +423,10 @@ impl PrepassPipeline {
// or emulated by setting depth in the fragment shader for GPUs that don't support it natively.
let emulate_unclipped_depth = mesh_key.contains(MeshPipelineKey::UNCLIPPED_DEPTH_ORTHO)
&& !self.depth_clip_control_supported;
if is_depth_only_opaque_prepass(mesh_key) && !emulate_unclipped_depth {
if is_depth_only_opaque_prepass(mesh_key)
&& !emulate_unclipped_depth
&& !material_properties.prepass_reads_material
{
bind_group_layouts.push(self.empty_layout.clone());
} else {
bind_group_layouts.push(
Expand Down Expand Up @@ -1114,6 +1119,10 @@ pub(crate) fn specialize_prepass_material_meshes(
mesh_key |= MeshPipelineKey::VISIBILITY_RANGE_DITHER;
}

if material.properties.prepass_reads_material {
mesh_key |= MeshPipelineKey::PREPASS_READS_MATERIAL;
}

// If the previous frame has skins or morph targets, note that.
if motion_vector_prepass.is_some() {
if mesh_instance
Expand Down
4 changes: 4 additions & 0 deletions crates/bevy_pbr/src/render/light.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2448,6 +2448,10 @@ pub(crate) fn specialize_shadows(
_ => MeshPipelineKey::NONE,
};

if material.properties.prepass_reads_material {
mesh_key |= MeshPipelineKey::PREPASS_READS_MATERIAL;
}

work_items.push(ShadowSpecializationWorkItem {
visible_entity: *visible_entity,
retained_view_entity: extracted_view_light.retained_view_entity,
Expand Down