From 73bf730da98c06ddf30a49e9099cddff5d9d37ff Mon Sep 17 00:00:00 2001
From: robtfm <50659922+robtfm@users.noreply.github.com>
Date: Wed, 14 Feb 2024 00:31:45 +0000
Subject: [PATCH] fix shadow batching (#11645)

# Objective

`RenderMeshInstance::material_bind_group_id` is only set from
`queue_material_meshes::<M>`. this field is used (only) for determining
batch groups, so some items may be batched incorrectly if they have
never been in the camera's view or if they don't use the Material
abstraction.

in particular, shadow views render more meshes than the main camera, and
currently batch some meshes where the object has never entered the
camera view together. this is quite hard to trigger, but should occur in
a scene with out-of-view alpha-mask materials (so that the material
instance actually affects the shadow) in the path of a light.

this is also a footgun for custom pipelines: failing to set the
material_bind_group_id will result in all meshes being batched together
and all using the closest/furthest material to the camera (depending on
sort order).

## Solution

- queue_shadows now sets the material_bind_group_id correctly
- `MeshPipeline` doesn't attempt to batch meshes if the
material_bind_group_id has not been set. custom pipelines still need to
set this field to take advantage of batching, but will at least render
correctly if it is not set
---
 crates/bevy_pbr/src/render/light.rs | 6 ++++--
 crates/bevy_pbr/src/render/mesh.rs  | 8 +++++++-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/crates/bevy_pbr/src/render/light.rs b/crates/bevy_pbr/src/render/light.rs
index 40319be1ee..d97ab7a5cd 100644
--- a/crates/bevy_pbr/src/render/light.rs
+++ b/crates/bevy_pbr/src/render/light.rs
@@ -1591,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>>>,
@@ -1636,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 {
                     continue;
                 };
                 if !mesh_instance.shadow_caster {
@@ -1686,6 +1686,8 @@ pub fn queue_shadows<M: Material>(
                     }
                 };
 
+                mesh_instance.material_bind_group_id = material.get_bind_group_id();
+
                 shadow_phase.add(Shadow {
                     draw_function: draw_shadow_mesh,
                     pipeline: pipeline_id,
diff --git a/crates/bevy_pbr/src/render/mesh.rs b/crates/bevy_pbr/src/render/mesh.rs
index b8d8b4dfd2..bf9d8bbb81 100644
--- a/crates/bevy_pbr/src/render/mesh.rs
+++ b/crates/bevy_pbr/src/render/mesh.rs
@@ -254,6 +254,12 @@ pub struct RenderMeshInstance {
     pub automatic_batching: bool,
 }
 
+impl RenderMeshInstance {
+    pub fn should_batch(&self) -> bool {
+        self.automatic_batching && self.material_bind_group_id.is_some()
+    }
+}
+
 #[derive(Default, Resource, Deref, DerefMut)]
 pub struct RenderMeshInstances(EntityHashMap<RenderMeshInstance>);
 
@@ -466,7 +472,7 @@ impl GetBatchData for MeshPipeline {
                 &mesh_instance.transforms,
                 maybe_lightmap.map(|lightmap| lightmap.uv_rect),
             ),
-            mesh_instance.automatic_batching.then_some((
+            mesh_instance.should_batch().then_some((
                 mesh_instance.material_bind_group_id,
                 mesh_instance.mesh_asset_id,
                 maybe_lightmap.map(|lightmap| lightmap.image),