Skip to content

Commit

Permalink
Update the Children component of the parent entity when a scene get…
Browse files Browse the repository at this point in the history
…s deleted (#12710)

# Objective

- A scene usually gets created using the `SceneBundle` or
`DynamicSceneBundle`. This means that the scene's entities get added as
children of the root entity (the entity on which the `SceneBundle` gets
added)
- When the scene gets deleted using the `SceneSpawner`, the scene's
entities are deleted, but the `Children` component of the root entity
doesn't get updated. This means that the hierarchy becomes unsound, with
Children linking to non-existing components.

## Solution

- Update the `despawn_sync` logic to also update the `Children` from any
parents of the scene, if there are any
- Adds a test where a Scene gets despawned and checks for dangling
Children references on the parent. The test fails on `main` but works
here.

## Alternative implementations

- One option could be to add a `parent: Option<Entity>` on the
[InstanceInfo](https://github.com/bevyengine/bevy/blob/df15cd7dcc09869f7ce9c7865378591b91b31749/crates/bevy_scene/src/scene_spawner.rs#L27)
struct that tracks if the SceneInstance was added as a child of a root
entity
  • Loading branch information
cBournhonesque committed Mar 29, 2024
1 parent bcdb20d commit afff818
Show file tree
Hide file tree
Showing 2 changed files with 80 additions and 2 deletions.
75 changes: 75 additions & 0 deletions crates/bevy_scene/src/bundle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,3 +98,78 @@ pub fn scene_spawner(
}
}
}

#[cfg(test)]
mod tests {
use crate::{DynamicScene, DynamicSceneBundle, ScenePlugin, SceneSpawner};
use bevy_app::{App, ScheduleRunnerPlugin};
use bevy_asset::{AssetPlugin, Assets};
use bevy_ecs::component::Component;
use bevy_ecs::entity::Entity;
use bevy_ecs::prelude::{AppTypeRegistry, ReflectComponent, World};
use bevy_hierarchy::{Children, HierarchyPlugin};
use bevy_reflect::Reflect;
use bevy_utils::default;

#[derive(Component, Reflect, Default)]
#[reflect(Component)]
struct ComponentA {
pub x: f32,
pub y: f32,
}

#[test]
fn spawn_and_delete() {
let mut app = App::new();

app.add_plugins(ScheduleRunnerPlugin::default())
.add_plugins(HierarchyPlugin)
.add_plugins(AssetPlugin::default())
.add_plugins(ScenePlugin)
.register_type::<ComponentA>();
app.update();

let mut scene_world = World::new();

// create a new DynamicScene manually
let type_registry = app.world.resource::<AppTypeRegistry>().clone();
scene_world.insert_resource(type_registry);
scene_world.spawn(ComponentA { x: 3.0, y: 4.0 });
let scene = DynamicScene::from_world(&scene_world);
let scene_handle = app.world.resource_mut::<Assets<DynamicScene>>().add(scene);

// spawn the scene as a child of `entity` using the `DynamicSceneBundle`
let entity = app
.world
.spawn(DynamicSceneBundle {
scene: scene_handle.clone(),
..default()
})
.id();

// run the app's schedule once, so that the scene gets spawned
app.update();

// make sure that the scene was added as a child of the root entity
let (scene_entity, scene_component_a) = app
.world
.query::<(Entity, &ComponentA)>()
.single(&app.world);
assert_eq!(scene_component_a.x, 3.0);
assert_eq!(scene_component_a.y, 4.0);
assert_eq!(app.world.entity(entity).get::<Children>().unwrap().len(), 1);

// let's try to delete the scene
let mut scene_spawner = app.world.resource_mut::<SceneSpawner>();
scene_spawner.despawn(&scene_handle);

// run the scene spawner system to despawn the scene
app.update();

// the scene entity does not exist anymore
assert!(app.world.get_entity(scene_entity).is_none());

// the root entity does not have any children anymore
assert!(app.world.entity(entity).get::<Children>().is_none());
}
}
7 changes: 5 additions & 2 deletions crates/bevy_scene/src/scene_spawner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use bevy_ecs::{
system::Resource,
world::{Command, Mut, World},
};
use bevy_hierarchy::{Parent, PushChild};
use bevy_hierarchy::{BuildWorldChildren, DespawnRecursiveExt, Parent, PushChild};
use bevy_utils::{tracing::error, HashMap, HashSet};
use thiserror::Error;
use uuid::Uuid;
Expand Down Expand Up @@ -189,7 +189,10 @@ impl SceneSpawner {
pub fn despawn_instance_sync(&mut self, world: &mut World, instance_id: &InstanceId) {
if let Some(instance) = self.spawned_instances.remove(instance_id) {
for &entity in instance.entity_map.values() {
let _ = world.despawn(entity);
if let Some(mut entity_mut) = world.get_entity_mut(entity) {
entity_mut.remove_parent();
entity_mut.despawn_recursive();
};
}
}
}
Expand Down

0 comments on commit afff818

Please sign in to comment.