From d99053cc8ad08b36c9c2215c4e974bbb066ec35c Mon Sep 17 00:00:00 2001
From: Matthew Gries <46942482+matthew-gries@users.noreply.github.com>
Date: Mon, 18 Dec 2023 15:55:12 -0500
Subject: [PATCH] Update AABB when Sprite component changes in
 calculate_bounds_2d() (#11016)

# Objective

- Fixes #10587, where the `Aabb` component of entities with `Sprite` and
`Handle<Image>` components was not automatically updated when
`Sprite::custom_size` changed.

## Solution

- In the query for entities with `Sprite` components in
`calculate_bounds_2d`, use the `Changed` filter to detect for `Sprites`
that changed as well as sprites that do not have `Aabb` components. As
noted in the issue, this will cause the `Aabb` to be recalculated when
other fields of the `Sprite` component change, but calculating the
`Aabb` for sprites is trivial.

---

## Changelog
- Modified query for entities with `Sprite` components in
`calculate_bounds_2d`, so that entities with `Sprite` components that
changed will also have their AABB recalculated.
---
 crates/bevy_sprite/src/lib.rs | 121 +++++++++++++++++++++++++++++++++-
 1 file changed, 118 insertions(+), 3 deletions(-)

diff --git a/crates/bevy_sprite/src/lib.rs b/crates/bevy_sprite/src/lib.rs
index a029ee6d6a..e64fb9808d 100644
--- a/crates/bevy_sprite/src/lib.rs
+++ b/crates/bevy_sprite/src/lib.rs
@@ -120,9 +120,12 @@ pub fn calculate_bounds_2d(
     images: Res<Assets<Image>>,
     atlases: Res<Assets<TextureAtlas>>,
     meshes_without_aabb: Query<(Entity, &Mesh2dHandle), (Without<Aabb>, Without<NoFrustumCulling>)>,
-    sprites_without_aabb: Query<
+    sprites_to_recalculate_aabb: Query<
         (Entity, &Sprite, &Handle<Image>),
-        (Without<Aabb>, Without<NoFrustumCulling>),
+        (
+            Or<(Without<Aabb>, Changed<Sprite>)>,
+            Without<NoFrustumCulling>,
+        ),
     >,
     atlases_without_aabb: Query<
         (Entity, &TextureAtlasSprite, &Handle<TextureAtlas>),
@@ -136,7 +139,7 @@ pub fn calculate_bounds_2d(
             }
         }
     }
-    for (entity, sprite, texture_handle) in &sprites_without_aabb {
+    for (entity, sprite, texture_handle) in &sprites_to_recalculate_aabb {
         if let Some(size) = sprite
             .custom_size
             .or_else(|| images.get(texture_handle).map(|image| image.size_f32()))
@@ -163,3 +166,115 @@ pub fn calculate_bounds_2d(
         }
     }
 }
+
+#[cfg(test)]
+mod test {
+
+    use bevy_math::Vec2;
+    use bevy_utils::default;
+
+    use super::*;
+
+    #[test]
+    fn calculate_bounds_2d_create_aabb_for_image_sprite_entity() {
+        // Setup app
+        let mut app = App::new();
+
+        // Add resources and get handle to image
+        let mut image_assets = Assets::<Image>::default();
+        let image_handle = image_assets.add(Image::default());
+        app.insert_resource(image_assets);
+        let mesh_assets = Assets::<Mesh>::default();
+        app.insert_resource(mesh_assets);
+        let texture_atlas_assets = Assets::<TextureAtlas>::default();
+        app.insert_resource(texture_atlas_assets);
+
+        // Add system
+        app.add_systems(Update, calculate_bounds_2d);
+
+        // Add entites
+        let entity = app.world.spawn((Sprite::default(), image_handle)).id();
+
+        // Verify that the entity does not have an AABB
+        assert!(!app
+            .world
+            .get_entity(entity)
+            .expect("Could not find entity")
+            .contains::<Aabb>());
+
+        // Run system
+        app.update();
+
+        // Verify the AABB exists
+        assert!(app
+            .world
+            .get_entity(entity)
+            .expect("Could not find entity")
+            .contains::<Aabb>());
+    }
+
+    #[test]
+    fn calculate_bounds_2d_update_aabb_when_sprite_custom_size_changes_to_some() {
+        // Setup app
+        let mut app = App::new();
+
+        // Add resources and get handle to image
+        let mut image_assets = Assets::<Image>::default();
+        let image_handle = image_assets.add(Image::default());
+        app.insert_resource(image_assets);
+        let mesh_assets = Assets::<Mesh>::default();
+        app.insert_resource(mesh_assets);
+        let texture_atlas_assets = Assets::<TextureAtlas>::default();
+        app.insert_resource(texture_atlas_assets);
+
+        // Add system
+        app.add_systems(Update, calculate_bounds_2d);
+
+        // Add entites
+        let entity = app
+            .world
+            .spawn((
+                Sprite {
+                    custom_size: Some(Vec2::ZERO),
+                    ..default()
+                },
+                image_handle,
+            ))
+            .id();
+
+        // Create initial AABB
+        app.update();
+
+        // Get the initial AABB
+        let first_aabb = *app
+            .world
+            .get_entity(entity)
+            .expect("Could not find entity")
+            .get::<Aabb>()
+            .expect("Could not find initial AABB");
+
+        // Change `custom_size` of sprite
+        let mut binding = app
+            .world
+            .get_entity_mut(entity)
+            .expect("Could not find entity");
+        let mut sprite = binding
+            .get_mut::<Sprite>()
+            .expect("Could not find sprite component of entity");
+        sprite.custom_size = Some(Vec2::ONE);
+
+        // Re-run the `calculate_bounds_2d` system to get the new AABB
+        app.update();
+
+        // Get the re-calculated AABB
+        let second_aabb = *app
+            .world
+            .get_entity(entity)
+            .expect("Could not find entity")
+            .get::<Aabb>()
+            .expect("Could not find second AABB");
+
+        // Check that the AABBs are not equal
+        assert_ne!(first_aabb, second_aabb);
+    }
+}