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

shadow performance #11647

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 13 additions & 0 deletions crates/bevy_pbr/src/extended_material.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,14 @@ pub trait MaterialExtension: Asset + AsBindGroup + Clone + Sized {
ShaderRef::Default
}

/// Specifies the shadow batch key (see [`Material::shadow_material_key`]). By default, extended materials
/// do not batch for shadows.
/// If the extension doesn't modify vertices or apply discards based on extended material properties, you
/// may be able to improve shadow performance by returning the base material key.
fn shadow_material_key(&self, _base_key: Option<u64>) -> Option<u64> {
None
}

/// Customizes the default [`RenderPipelineDescriptor`] for a specific entity using the entity's
/// [`MaterialPipelineKey`] and [`MeshVertexBufferLayout`] as input.
/// Specialization for the base material is applied before this function is called.
Expand Down Expand Up @@ -211,6 +219,11 @@ impl<B: Material, E: MaterialExtension> Material for ExtendedMaterial<B, E> {
}
}

fn shadow_material_key(&self) -> Option<u64> {
self.extension
.shadow_material_key(self.base.shadow_material_key())
}

fn specialize(
pipeline: &MaterialPipeline<Self>,
descriptor: &mut RenderPipelineDescriptor,
Expand Down
35 changes: 32 additions & 3 deletions crates/bevy_pbr/src/material.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,13 @@ pub trait Material: Asset + AsBindGroup + Clone + Sized {
) -> Result<(), SpecializedMeshPipelineError> {
Ok(())
}

/// Specify a batch key to use for shadows. Returning Some(value) allows batching of distinct materials which will produce
Copy link
Contributor

Choose a reason for hiding this comment

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

Wording isn't very clear. It took me some time to realize that this would basically force-batch different materials together by grouping them by this arbitrary value, and then using the first material of the batch for all meshes in the batch.

/// the same depth-prepass output (such as opaque [`StandardMaterial`]s with different textures), rendering with the first
/// material of the batch for all meshes.
fn shadow_material_key(&self) -> Option<u64> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels pretty hacky :/. I'd rather we just have a separate set of bindings for shadow passes, or split bindings between vertex/fragment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered how to do this without requiring large user changes, I couldn’t see a way.

Ideally you want this to work for standard materials without extra user input, so

  • a separate component (ShadowMaterial(Handle<M>)) doesn't really work unless we also add a system to add the component automatically where appropriate, and that system would need to do lookups into the assets resource to make the determination, which sounds expensive and still messy.
  • have the key function return a handle/asset id, this requires either adding the handle(s) to every material or using some resource to store them and giving access in the key function. This gets at least as awkward, particularly if you have more than one groupable set (I have this case in my work code where some opaque materials can be grouped but not all, based on the “plot” they belong to in the world). Also adds effort around extracting/preparing the new handles, basically seems like a lot of extra complexity for limited reward.

If you can see a clean way to do this I’d be happy to change it, but I don’t see it right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose for now this is the best we can do. I think for 0.14 we might want to look into redoing the Material API so we can support batching, bindless, etc easier.

None
Copy link
Contributor

Choose a reason for hiding this comment

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

So why is this None by default?

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 in the Material trait. it's overridden in the StandardMaterial impl

}
}

/// Adds the necessary ECS resources and render logic to enable rendering entities using the given [`Material`]
Expand Down Expand Up @@ -773,16 +780,33 @@ pub struct MaterialProperties {
pub struct PreparedMaterial<T: Material> {
pub bindings: Vec<(u32, OwnedBindingResource)>,
pub bind_group: BindGroup,
pub shadow_bind_group_id: MaterialBindGroupId,
pub key: T::Data,
pub properties: MaterialProperties,
}

#[derive(Component, Clone, Copy, Default, PartialEq, Eq, Deref, DerefMut)]
pub struct MaterialBindGroupId(Option<BindGroupId>);
#[derive(Component, Clone, Copy, Default)]
pub enum MaterialBindGroupId {
#[default]
Unbatched,
Instance(BindGroupId),
Group(u64),
}

impl PartialEq for MaterialBindGroupId {
fn eq(&self, other: &Self) -> bool {
match (self, other) {
(Self::Instance(l0), Self::Instance(r0)) => l0 == r0,
(Self::Group(l0), Self::Group(r0)) => l0 == r0,
// unbatched should never report equal to prevent batching
_ => false,
}
}
}

impl<T: Material> PreparedMaterial<T> {
pub fn get_bind_group_id(&self) -> MaterialBindGroupId {
MaterialBindGroupId(Some(self.bind_group.id()))
MaterialBindGroupId::Instance(self.bind_group.id())
}
}

Expand Down Expand Up @@ -937,9 +961,14 @@ fn prepare_material<M: Material>(
OpaqueRendererMethod::Deferred => OpaqueRendererMethod::Deferred,
OpaqueRendererMethod::Auto => default_opaque_render_method,
};
let shadow_bind_group_id = match material.shadow_material_key() {
Some(key) => MaterialBindGroupId::Group(key),
None => MaterialBindGroupId::Instance(prepared.bind_group.id()),
};
Ok(PreparedMaterial {
bindings: prepared.bindings,
bind_group: prepared.bind_group,
shadow_bind_group_id,
key: prepared.data,
properties: MaterialProperties {
alpha_mode: material.alpha_mode(),
Expand Down
5 changes: 5 additions & 0 deletions crates/bevy_pbr/src/pbr_material.rs
Original file line number Diff line number Diff line change
Expand Up @@ -801,6 +801,11 @@ impl Material for StandardMaterial {
PBR_SHADER_HANDLE.into()
}

fn shadow_material_key(&self) -> Option<u64> {
// we can batch all pure opaque materials together for shadow rendering
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it just meshes that break our batching for shadows then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for standard materials after this pr, yes. part of this pr is to make all opaque standard materials share a material instance for shadow casting

(self.alpha_mode == AlphaMode::Opaque).then_some(0)
}

fn specialize(
_pipeline: &MaterialPipeline<Self>,
descriptor: &mut RenderPipelineDescriptor,
Expand Down
22 changes: 13 additions & 9 deletions crates/bevy_pbr/src/render/light.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use bevy_asset::AssetId;
use bevy_core_pipeline::core_3d::{Transparent3d, CORE_3D_DEPTH_FORMAT};
use bevy_ecs::prelude::*;
use bevy_math::{Mat4, UVec3, UVec4, Vec2, Vec3, Vec3Swizzles, Vec4, Vec4Swizzles};
Expand Down Expand Up @@ -1590,7 +1591,7 @@ pub fn queue_shadows<M: Material>(
shadow_draw_functions: Res<DrawFunctions<Shadow>>,
prepass_pipeline: Res<PrepassPipeline<M>>,
render_meshes: Res<RenderAssets<Mesh>>,
render_mesh_instances: Res<RenderMeshInstances>,
mut render_mesh_instances: ResMut<RenderMeshInstances>,
render_materials: Res<RenderMaterials<M>>,
render_material_instances: Res<RenderMaterialInstances<M>>,
mut pipelines: ResMut<SpecializedMeshPipelines<PrepassPipeline<M>>>,
Expand Down Expand Up @@ -1635,7 +1636,7 @@ pub fn queue_shadows<M: Material>(
// NOTE: Lights with shadow mapping disabled will have no visible entities
// so no meshes will be queued
for entity in visible_entities.iter().copied() {
let Some(mesh_instance) = render_mesh_instances.get(&entity) else {
let Some(mesh_instance) = render_mesh_instances.get_mut(&entity) else {
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look like it needs to be mut?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's modified @1689 to add the shadow_bind_group_id

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, not sure how I missed that.

continue;
};
if !mesh_instance.shadow_caster {
Expand Down Expand Up @@ -1685,11 +1686,13 @@ pub fn queue_shadows<M: Material>(
}
};

mesh_instance.shadow_material_bind_group_id = material.shadow_bind_group_id;

shadow_phase.add(Shadow {
draw_function: draw_shadow_mesh,
pipeline: pipeline_id,
entity,
distance: 0.0, // TODO: sort front-to-back
asset_id: mesh_instance.mesh_asset_id,
batch_range: 0..1,
dynamic_offset: None,
});
Expand All @@ -1699,7 +1702,7 @@ pub fn queue_shadows<M: Material>(
}

pub struct Shadow {
pub distance: f32,
pub asset_id: AssetId<Mesh>,
Copy link
Contributor

Choose a reason for hiding this comment

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

In the future we probably want to be able to represent AssetID as a u64 or something so we can go back to using radsort, as I noticed decreases in sort performance in the opaque sorting by pipeline PR (that were more than made up for by the better batching).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

definitely, should be easy to do.

pub entity: Entity,
pub pipeline: CachedRenderPipelineId,
pub draw_function: DrawFunctionId,
Expand All @@ -1708,7 +1711,7 @@ pub struct Shadow {
}

impl PhaseItem for Shadow {
type SortKey = usize;
type SortKey = (usize, AssetId<Mesh>);

#[inline]
fn entity(&self) -> Entity {
Expand All @@ -1717,7 +1720,7 @@ impl PhaseItem for Shadow {

#[inline]
fn sort_key(&self) -> Self::SortKey {
self.pipeline.id()
(self.pipeline.id(), self.asset_id)
}

#[inline]
Expand All @@ -1727,10 +1730,11 @@ impl PhaseItem for Shadow {

#[inline]
fn sort(items: &mut [Self]) {
// The shadow phase is sorted by pipeline id for performance reasons.
// The shadow phase is sorted by pipeline id and mesh id for performance reasons.
// Grouping all draw commands using the same pipeline together performs
// better than rebinding everything at a high rate.
radsort::sort_by_key(items, |item| item.sort_key());
// better than rebinding everything at a high rate, and grouping by mesh
// allows batching to reduce draw calls.
items.sort_unstable_by_key(Self::sort_key);
}

#[inline]
Expand Down
34 changes: 33 additions & 1 deletion crates/bevy_pbr/src/render/mesh.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ impl Plugin for MeshRenderPlugin {
batch_and_prepare_render_phase::<Transmissive3d, MeshPipeline>,
batch_and_prepare_render_phase::<Transparent3d, MeshPipeline>,
batch_and_prepare_render_phase::<AlphaMask3d, MeshPipeline>,
batch_and_prepare_render_phase::<Shadow, MeshPipeline>,
batch_and_prepare_render_phase::<Shadow, ShadowMeshPipeline>,
batch_and_prepare_render_phase::<Opaque3dDeferred, MeshPipeline>,
batch_and_prepare_render_phase::<AlphaMask3dDeferred, MeshPipeline>,
)
Expand Down Expand Up @@ -257,6 +257,7 @@ pub struct RenderMeshInstance {
pub transforms: MeshTransforms,
pub mesh_asset_id: AssetId<Mesh>,
pub material_bind_group_id: MaterialBindGroupId,
pub shadow_material_bind_group_id: MaterialBindGroupId,
pub shadow_caster: bool,
pub automatic_batching: bool,
}
Expand Down Expand Up @@ -328,6 +329,7 @@ pub fn extract_meshes(
transforms,
shadow_caster: !not_shadow_caster,
material_bind_group_id: MaterialBindGroupId::default(),
shadow_material_bind_group_id: MaterialBindGroupId::default(),
automatic_batching: !no_automatic_batching,
},
));
Expand Down Expand Up @@ -503,6 +505,36 @@ impl GetBatchData for MeshPipeline {
}
}

pub struct ShadowMeshPipeline;
impl GetBatchData for ShadowMeshPipeline {
type Param = (SRes<RenderMeshInstances>, SRes<RenderLightmaps>);
// The material bind group ID, the mesh ID, and the lightmap ID,
// respectively.
type CompareData = (MaterialBindGroupId, AssetId<Mesh>, Option<AssetId<Image>>);

type BufferData = MeshUniform;

fn get_batch_data(
(mesh_instances, lightmaps): &SystemParamItem<Self::Param>,
entity: Entity,
) -> Option<(Self::BufferData, Option<Self::CompareData>)> {
let mesh_instance = mesh_instances.get(&entity)?;
let maybe_lightmap = lightmaps.render_lightmaps.get(&entity);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would there be any issues with removing lightmaps in CompareData for shadow pass?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For standard materials, no… in general, possibly I guess? But probably not.

If it did matter for a custom material then the author could add some data into the material and use it to differentiate in the shadow_material_key fn. I’ll remove it from here.


Some((
MeshUniform::new(
&mesh_instance.transforms,
maybe_lightmap.map(|lightmap| lightmap.uv_rect),
),
mesh_instance.automatic_batching.then_some((
mesh_instance.shadow_material_bind_group_id,
mesh_instance.mesh_asset_id,
maybe_lightmap.map(|lightmap| lightmap.image),
)),
))
}
}

bitflags::bitflags! {
#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
#[repr(transparent)]
Expand Down
12 changes: 11 additions & 1 deletion examples/stress_tests/many_cubes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ struct Args {
/// the number of different textures from which to randomly select the material base color. 0 means no textures.
#[argh(option, default = "0")]
material_texture_count: usize,

/// enable cascaded shadow map
#[argh(switch)]
shadows: bool,
}

#[derive(Default, Clone)]
Expand Down Expand Up @@ -185,7 +189,13 @@ fn setup(
}
}

commands.spawn(DirectionalLightBundle { ..default() });
commands.spawn(DirectionalLightBundle {
directional_light: DirectionalLight {
shadows_enabled: args.shadows,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this default to shadows on or off?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

defaults to off

..default()
},
..default()
});
}

fn init_textures(args: &Args, images: &mut Assets<Image>) -> Vec<Handle<Image>> {
Expand Down