Despawn replaced linked-spawn related entities when applying a scene

_release-content/migration-guides/apply_scene_despawns_replaced_children.md

@@ -0,0 +1,19 @@
+---
+title: Applying a scene now despawns the related entities it replaces
+pull_requests: []
+---
+
+Previously, applying a `Scene` to an existing entity (ex: via `apply_scene` or `queue_apply_scene`)
+would replace and _orphan_ the entity's pre-existing related entities (ex: `Children`). The orphaned
+entities lingered in the world, which produced "ghost" UI nodes when re-applying widget scenes
+(ex: updating the rows of a `FeathersListView` with a scene patch).
+
+Now, when an applied scene defines related entities for a relationship, the pre-existing related
+entities are _despawned_ if the relationship uses "linked spawn" semantics
+(see `RelationshipTarget::LINKED_SPAWN`). This is the case for `Children`, so re-applying a scene
+that defines children replaces them cleanly. Relationships without "linked spawn" semantics still
+orphan the replaced entities.
+
+If you relied on the previous behavior, break the relationship before applying the scene (ex: by
+calling `remove_children` or removing the relationship component) so the entities you want to keep
+are no longer related when the scene is applied.

crates/bevy_scene/src/resolved_scene.rs

@@ -264,6 +264,16 @@ impl ResolvedScene {
                         error: Box::new(e),
                     })?;
                 self.apply_templates_without_bundle_write(context, &mut bundle_writer, ())?;
+                // Despawn the entity's pre-existing related entities for each relationship this scene
+                // defines, before the new related entities replace them. Without this, the replaced
+                // entities would linger in the world as orphans (only relationships with "linked spawn"
+                // semantics, such as Children, are despawned).
+                for related in resolved_cached.scene.related.values() {
+                    (related.despawn_existing_related)(context.entity);
+                }
+                for related in self.related.values() {
+                    (related.despawn_existing_related)(context.entity);
+                }
                 // SAFETY: World is only used for component registration, which does not affect
                 // the entity location
                 let components = &mut context.entity.world_mut().components_registrator();
@@ -290,6 +300,13 @@ impl ResolvedScene {
             // SAFETY: bundle_writer was used with the same World across all cases in this function,
             unsafe {
                 self.apply_templates_without_bundle_write(context, &mut bundle_writer, ())?;
+                // Despawn the entity's pre-existing related entities for each relationship this scene
+                // defines, before the new related entities replace them. Without this, the replaced
+                // entities would linger in the world as orphans (only relationships with "linked spawn"
+                // semantics, such as Children, are despawned).
+                for related in self.related.values() {
+                    (related.despawn_existing_related)(context.entity);
+                }
                 // SAFETY: World is only used for component registration, which does not affect
                 // the entity location
                 let components = &mut context.entity.world_mut().components_registrator();
@@ -655,6 +672,11 @@ pub struct RelatedResolvedScenes {
         unsafe fn(&mut BundleWriter, &mut ComponentsRegistrator, target: Entity),
     /// The function that will be called to add the relationship target to the spawned scene with the given capacity.
     pub insert_relationship_target: unsafe fn(&mut BundleWriter, &mut ComponentsRegistrator, usize),
+    /// The function that will be called to despawn the entity's pre-existing related entities before
+    /// this scene's related entities replace them. This only despawns entities for relationships with
+    /// "linked spawn" semantics (see [`RelationshipTarget::LINKED_SPAWN`]), such as [`Children`](bevy_ecs::hierarchy::Children).
+    /// For other relationships this is a no-op, and the pre-existing related entities are orphaned instead.
+    pub despawn_existing_related: fn(&mut EntityWorldMut),
     /// The type name of the relationship. This is used for more helpful error message.
     pub relationship_name: &'static str,
 }
@@ -686,6 +708,11 @@ impl RelatedResolvedScenes {
                     bundle_writer.push_component(components_registrator, relationship_target);
                 };
             },
+            despawn_existing_related: |entity| {
+                if <R::RelationshipTarget as RelationshipTarget>::LINKED_SPAWN {
+                    entity.despawn_related::<R::RelationshipTarget>();
+                }
+            },
             relationship_name: core::any::type_name::<R>(),
         }
     }

crates/bevy_scene/src/spawn.rs

@@ -454,7 +454,13 @@ pub trait EntityWorldMutSceneExt {
     /// Applies the given [`Scene`] to the current entity immediately. This will resolve the Scene (using [`Scene::resolve`]). If that fails (for example, if there are dependencies that have not been
     /// loaded yet), it will return a [`SpawnSceneError`]. If resolving the [`Scene`] is successful, the scene will be spawned.
     ///
-    /// When a scene is resolved, it will replace and orphan the current entity's children.
+    /// When a scene is resolved, it will replace the current entity's related entities for each
+    /// relationship the scene defines (ex: [`Children`]). Pre-existing related entities are despawned
+    /// if the relationship uses "linked spawn" semantics (see [`RelationshipTarget::LINKED_SPAWN`]),
+    /// which is the case for [`Children`]. Otherwise, they are orphaned.
+    ///
+    /// [`Children`]: bevy_ecs::hierarchy::Children
+    /// [`RelationshipTarget::LINKED_SPAWN`]: bevy_ecs::relationship::RelationshipTarget::LINKED_SPAWN
     ///
     /// If resolving and spawning is successful, the entity will contain the full contents of the spawned scene.
     ///
@@ -870,7 +876,7 @@ impl QueuedScenes {
 #[cfg(test)]
 mod tests {
     use super::EntityWorldMutSceneExt;
-    use crate::{self as bevy_scene, bsn, ScenePlugin};
+    use crate::{self as bevy_scene, bsn, bsn_list, ScenePlugin};
     use bevy_app::{App, TaskPoolPlugin};
     use bevy_asset::AssetPlugin;
     use bevy_ecs::{name::Name, prelude::*, template::FromTemplate};
@@ -893,7 +899,7 @@ mod tests {
 
     /// Tests that documented behavior of [`EntityWorldMutSceneExt::apply_scene`] is correct.
     #[test]
-    fn apply_scene_replaces_and_orphans_children() {
+    fn apply_scene_replaces_and_despawns_children() {
         let mut app = test_app();
         let world = app.world_mut();
 
@@ -920,8 +926,69 @@ mod tests {
         assert_eq!(children.len(), 1);
         assert!(world.entity(children[0]).contains::<SceneChild>());
 
-        // Pre-existing child entity still exists, but is no longer listed under root.
+        // Pre-existing child entity is despawned, because `Children` uses "linked spawn" semantics.
+        assert!(world.get_entity(pre_existing).is_err());
+    }
+
+    /// Applying the same scene to an entity multiple times must not accumulate replaced children
+    /// as orphans (see issue #24939, where re-applying a widget scene left "ghost" UI nodes behind).
+    #[test]
+    fn reapply_scene_does_not_leak_children() {
+        let mut app = test_app();
+        let world = app.world_mut();
+
+        let root = world.spawn(Name::new("root")).id();
+        for _ in 0..3 {
+            let scene = bsn! {
+                Children [ SceneChild, SceneChild ]
+            };
+            world.entity_mut(root).apply_scene(scene).unwrap();
+        }
+
+        assert_eq!(
+            world.entity(root).get::<Children>().map(Children::len),
+            Some(2)
+        );
+
+        // Only the children from the most recent application exist in the world.
+        let scene_children = world
+            .query_filtered::<(), With<SceneChild>>()
+            .iter(world)
+            .count();
+        assert_eq!(scene_children, 2);
+    }
+
+    /// Relationships without "linked spawn" semantics must orphan (not despawn) the
+    /// pre-existing related entities they replace.
+    #[test]
+    fn apply_scene_orphans_related_without_linked_spawn() {
+        use crate::RelatedScenes;
+        use bevy_ecs::relationship::RelationshipTarget as _;
+
+        #[derive(Component, Clone)]
+        #[relationship(relationship_target = LikedBy)]
+        struct Liking(Entity);
+
+        #[derive(Component)]
+        #[relationship_target(relationship = Liking)]
+        struct LikedBy(Vec<Entity>);
+
+        let mut app = test_app();
+        let world = app.world_mut();
+
+        let root = world.spawn(Name::new("root")).id();
+        let pre_existing = world.spawn(Liking(root)).id();
+
+        let scene = RelatedScenes::<Liking, _>::new(bsn_list![SceneChild]);
+        world.entity_mut(root).apply_scene(scene).unwrap();
+
+        // The pre-existing related entity is orphaned, not despawned, because `LikedBy`
+        // does not use "linked spawn" semantics.
         assert!(world.get_entity(pre_existing).is_ok());
-        assert!(!children.contains(&pre_existing));
+        assert!(world.entity(pre_existing).get::<Liking>().is_none());
+
+        // The scene's related entity replaced it.
+        let liked_by = world.entity(root).get::<LikedBy>().unwrap();
+        assert_eq!(liked_by.collection().len(), 1);
     }
 }