shadow performance #11647

robtfm · 2024-02-01T12:56:34Z

Objective

some minor optimisations for shadow rendering performance

Solution

added --shadows option to many_cubes for testing
added mesh-id sorting for shadow phase items.
when rendering shadows, added material batching for materials that have a common depth-prepass output (such as opaque StandardMaterials). the material must support the batching via Material::shadow_material_key
added shadow batching for extended materials, using ~~the base material's key~~ no batching by default. i am not sure if this is the right thing to do, it will cause some existing ExtendedMaterials to render incorrectly (e.g. if they move vertices based on material data). the alternative is to by default disable shadow batching for extended materials (it could still be enabled by authors)

perf impacts:

command line	current fps	new fps
cargo run --example many_cubes --release -- --benchmark --shadows	27	27
cargo run --example many_cubes --release -- --benchmark --shadows --vary-per-instance	2.5	12

when sharing a material handle there is no impact as expected. when using distinct materials the frame rate increases ~5x.

this is obviously an exaggeration of real world impacts, but on a random real world scene i had

scenario	fps
without shadows	110
with shadows current	50
with shadows new	64
with shadows just sorting	57
with shadows just batching	60

a second test scene (thanks @Elabajaba) showed a smaller change from 56fps to 60fps.

robtfm · 2024-02-01T13:07:19Z

note this also fixes the issues from #11645 but is maybe more contentious

robtfm · 2024-02-03T01:33:00Z

modified to sort shadows by mesh id, since we only batch entities with the same mesh (we still sort by pipeline first)

Elabajaba · 2024-02-03T04:49:06Z

This is saving ~7000 draw calls in my castle stress test view (total went from ~19.8k -> ~12.6k, which is still insane, but less insane).

I wonder if sorting the opaque pass by mesh id would net us a similar decrease?

robtfm · 2024-02-03T10:15:56Z

I wonder if sorting the opaque pass by mesh id would net us a similar decrease?

I guess that would depend on frag shader cost (more overdraw / less drawcalls or vice versa) … might be nice to make it an option somehow.

Elabajaba · 2024-02-03T04:52:01Z

crates/bevy_pbr/src/pbr_material.rs

@@ -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


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

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

Elabajaba · 2024-02-03T06:33:34Z

examples/stress_tests/many_cubes.rs

-    commands.spawn(DirectionalLightBundle { ..default() });
+    commands.spawn(DirectionalLightBundle {
+        directional_light: DirectionalLight {
+            shadows_enabled: args.shadows,


Does this default to shadows on or off?

defaults to off

Elabajaba · 2024-02-08T23:48:13Z

crates/bevy_pbr/src/material.rs

+    /// 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> {
+        None


So why is this None by default?

this is in the Material trait. it's overridden in the StandardMaterial impl

Elabajaba · 2024-02-08T23:52:09Z

crates/bevy_pbr/src/render/light.rs

@@ -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 {


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

it's modified @1689 to add the shadow_bind_group_id

Ah, not sure how I missed that.

Elabajaba · 2024-02-08T23:54:26Z

crates/bevy_pbr/src/render/light.rs

@@ -1699,7 +1702,7 @@ pub fn queue_shadows<M: Material>(
 }

 pub struct Shadow {
-    pub distance: f32,
+    pub asset_id: AssetId<Mesh>,


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).

definitely, should be easy to do.

crates/bevy_pbr/src/render/light.rs

re0312 · 2024-02-10T01:06:54Z

crates/bevy_pbr/src/render/mesh.rs

+        entity: Entity,
+    ) -> Option<(Self::BufferData, Option<Self::CompareData>)> {
+        let mesh_instance = mesh_instances.get(&entity)?;
+        let maybe_lightmap = lightmaps.render_lightmaps.get(&entity);


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

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.

JMS55 · 2024-02-10T06:13:32Z

crates/bevy_pbr/src/material.rs

+    /// Specify a batch key to use for shadows. Returning Some(value) allows batching of distinct materials which will produce
+    /// 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> {


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

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.

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.

JMS55 · 2024-02-10T06:14:42Z

crates/bevy_pbr/src/material.rs

@@ -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


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.

alice-i-cecile · 2024-02-14T01:23:08Z

This is nice but not vital: bumping from the milestone.

shadow perf

3e0695c

robtfm added A-Rendering Drawing game state to the screen C-Performance A change motivated by improving speed, memory usage or compile times labels Feb 1, 2024

robtfm added 2 commits February 1, 2024 13:27

fmt

3ad54d7

batch by mesh

a64050e

Elabajaba mentioned this pull request Feb 5, 2024

Look into sorting rendering pipelines by bindgroups. #11715

Open

Elabajaba reviewed Feb 8, 2024

View reviewed changes

robtfm added 4 commits February 9, 2024 00:22

rename prepass_material_bind_group_id

c5b09e0

sort_unstable_by_key

2bb1617

extended material defaults to no shadow batching

6b31729

super picky ci

2ea247b

Elabajaba approved these changes Feb 9, 2024

View reviewed changes

re0312 reviewed Feb 10, 2024

View reviewed changes

JMS55 added this to the 0.13 milestone Feb 10, 2024

JMS55 reviewed Feb 10, 2024

View reviewed changes

alice-i-cecile modified the milestones: 0.13, 0.14 Feb 14, 2024

JMS55 removed this from the 0.14 milestone May 14, 2024

janhohenheim added S-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels Sep 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

shadow performance #11647

shadow performance #11647

robtfm commented Feb 1, 2024 •

edited

Loading

robtfm commented Feb 1, 2024

robtfm commented Feb 3, 2024

Elabajaba commented Feb 3, 2024

robtfm commented Feb 3, 2024 •

edited

Loading

Elabajaba Feb 3, 2024

robtfm Feb 9, 2024

Elabajaba Feb 3, 2024

robtfm Feb 9, 2024

Elabajaba Feb 8, 2024

robtfm Feb 9, 2024

Elabajaba Feb 8, 2024

robtfm Feb 9, 2024

Elabajaba Feb 9, 2024

Elabajaba Feb 8, 2024

robtfm Feb 9, 2024

re0312 Feb 10, 2024

robtfm Feb 10, 2024

JMS55 Feb 10, 2024

robtfm Feb 10, 2024

JMS55 Feb 10, 2024

JMS55 Feb 10, 2024

alice-i-cecile commented Feb 14, 2024

shadow performance #11647

Are you sure you want to change the base?

shadow performance #11647

Conversation

robtfm commented Feb 1, 2024 • edited Loading

Objective

Solution

perf impacts:

robtfm commented Feb 1, 2024

robtfm commented Feb 3, 2024

Elabajaba commented Feb 3, 2024

robtfm commented Feb 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alice-i-cecile commented Feb 14, 2024

robtfm commented Feb 1, 2024 •

edited

Loading

robtfm commented Feb 3, 2024 •

edited

Loading