From 3d1c9ca87f0247ca5857ddbe9e65efeed4dc0938 Mon Sep 17 00:00:00 2001 From: Robin KAY Date: Tue, 30 Jul 2024 22:23:48 +0100 Subject: [PATCH] Change `SceneInstanceReady` to trigger an observer. (#13859) # Objective The `SceneInstanceReady` event would be more ergonomic (and potentially efficient) if it could be delivered to listeners attached to the scene entities becoming ready rather than into a World-global queue. This is an evolution of @Shatur's work in #9313. ## Solution The scene spawner is changed to trigger observers on the scene entity when it is ready rather than enqueue an event with `EventWriter`. This addresses the two outstanding feature requests mentioned on #2218, that i) the events should be "scoped" in some way and ii) that the `InstanceId` should be included in the event. ## Testing Modified the `scene_spawner::tests::event` test to use the new mechanism. --- ## Changelog - Changed `SceneInstanceReady` to trigger an entity observer rather than be written to an event queue. - Changed `SceneInstanceReady` to carry the `InstanceId` of the scene. ## Migration Guide If you have a system which read `SceneInstanceReady` events: > ```fn ready_system(ready_events: EventReader<'_, '_, SceneInstanceReady>) {``` It must be rewritten as an observer: > ```commands.observe(|trigger: Trigger| {``` Or, if you were expecting the event in relation to a specific entity or entities, as an entity observer: > ```commands.entity(entity).observe(|trigger: Trigger| {``` --- crates/bevy_scene/src/lib.rs | 1 - crates/bevy_scene/src/scene_spawner.rs | 195 +++++++++++-------------- 2 files changed, 84 insertions(+), 112 deletions(-) diff --git a/crates/bevy_scene/src/lib.rs b/crates/bevy_scene/src/lib.rs index fa00fde5cc510..b7bc473ca06d4 100644 --- a/crates/bevy_scene/src/lib.rs +++ b/crates/bevy_scene/src/lib.rs @@ -56,7 +56,6 @@ impl Plugin for ScenePlugin { app.init_asset::() .init_asset::() .init_asset_loader::() - .add_event::() .init_resource::() .add_systems(SpawnScene, (scene_spawner, scene_spawner_system).chain()); diff --git a/crates/bevy_scene/src/scene_spawner.rs b/crates/bevy_scene/src/scene_spawner.rs index 2f4692cdc7017..9aa59bb6fd8ed 100644 --- a/crates/bevy_scene/src/scene_spawner.rs +++ b/crates/bevy_scene/src/scene_spawner.rs @@ -13,15 +13,15 @@ use bevy_utils::{tracing::error, HashMap, HashSet}; use thiserror::Error; use uuid::Uuid; -/// Emitted when [`crate::SceneInstance`] becomes ready to use. +/// Triggered on a scene's parent entity when [`crate::SceneInstance`] becomes ready to use. /// -/// See also [`SceneSpawner::instance_is_ready`]. +/// See also [`Trigger`], [`SceneSpawner::instance_is_ready`]. +/// +/// [`Trigger`]: bevy_ecs::observer::Trigger #[derive(Clone, Copy, Debug, Eq, PartialEq, Event)] pub struct SceneInstanceReady { - /// ID of the spawned instance. - pub id: InstanceId, - /// Entity to which the scene was spawned as a child. - pub parent: Option, + /// Instance which has been spawned. + pub instance_id: InstanceId, } /// Information about a scene instance. @@ -323,10 +323,8 @@ impl SceneSpawner { // Scenes with parents need more setup before they are ready. // See `set_scene_instance_parent_sync()`. if parent.is_none() { - world.send_event(SceneInstanceReady { - id: instance_id, - parent: None, - }); + // Defer via commands otherwise SceneSpawner is not available in the observer. + world.commands().trigger(SceneInstanceReady { instance_id }); } } Err(SceneSpawnError::NonExistentScene { .. }) => { @@ -350,10 +348,8 @@ impl SceneSpawner { // Scenes with parents need more setup before they are ready. // See `set_scene_instance_parent_sync()`. if parent.is_none() { - world.send_event(SceneInstanceReady { - id: instance_id, - parent: None, - }); + // Defer via commands otherwise SceneSpawner is not available in the observer. + world.commands().trigger(SceneInstanceReady { instance_id }); } } Err(SceneSpawnError::NonExistentRealScene { .. }) => { @@ -392,10 +388,10 @@ impl SceneSpawner { } } - world.send_event(SceneInstanceReady { - id: instance_id, - parent: Some(parent), - }); + // Defer via commands otherwise SceneSpawner is not available in the observer. + world + .commands() + .trigger_targets(SceneInstanceReady { instance_id }, parent); } else { self.scenes_with_parent.push((instance_id, parent)); } @@ -479,7 +475,7 @@ mod tests { use bevy_app::App; use bevy_asset::Handle; use bevy_asset::{AssetPlugin, AssetServer}; - use bevy_ecs::event::EventReader; + use bevy_ecs::observer::Trigger; use bevy_ecs::prelude::ReflectComponent; use bevy_ecs::query::With; use bevy_ecs::system::{Commands, Res, ResMut, RunSystemOnce}; @@ -545,9 +541,13 @@ mod tests { #[reflect(Component)] struct ComponentA; + #[derive(Resource, Default)] + struct TriggerCount(u32); + fn setup() -> App { let mut app = App::new(); app.add_plugins((AssetPlugin::default(), ScenePlugin)); + app.init_resource::(); app.register_type::(); app.world_mut().spawn(ComponentA); @@ -569,84 +569,68 @@ mod tests { ) } - #[test] - fn event_scene() { - let mut app = setup(); - - // Build scene. - let scene = build_scene(&mut app); - - // Spawn scene. - let scene_id = - app.world_mut() - .run_system_once(move |mut scene_spawner: ResMut<'_, SceneSpawner>| { - scene_spawner.spawn(scene.clone()) - }); - - // Check for event arrival. - app.update(); - app.world_mut().run_system_once( - move |mut ev_scene: EventReader<'_, '_, SceneInstanceReady>| { - let mut events = ev_scene.read(); + fn build_dynamic_scene(app: &mut App) -> Handle { + app.world_mut() + .run_system_once(|world: &World, asset_server: Res<'_, AssetServer>| { + asset_server.add(DynamicScene::from_world(world)) + }) + } - let event = events.next().expect("found no `SceneInstanceReady` event"); + fn observe_trigger(app: &mut App, scene_id: InstanceId, scene_entity: Entity) { + // Add observer + app.world_mut().observe( + move |trigger: Trigger, + scene_spawner: Res, + mut trigger_count: ResMut| { assert_eq!( - event.id, scene_id, + trigger.event().instance_id, + scene_id, "`SceneInstanceReady` contains the wrong `InstanceId`" ); - - assert!(events.next().is_none(), "found more than one event"); + assert_eq!( + trigger.entity(), + scene_entity, + "`SceneInstanceReady` triggered on the wrong parent entity" + ); + assert!( + scene_spawner.instance_is_ready(trigger.event().instance_id), + "`InstanceId` is not ready" + ); + trigger_count.0 += 1; }, ); + + // Check observer is triggered once. + app.update(); + app.world_mut() + .run_system_once(|trigger_count: Res| { + assert_eq!( + trigger_count.0, 1, + "wrong number of `SceneInstanceReady` triggers" + ); + }); } #[test] - fn event_scene_as_child() { + fn observe_scene() { let mut app = setup(); // Build scene. let scene = build_scene(&mut app); - // Spawn scene as child. - let (scene_id, scene_entity) = app.world_mut().run_system_once( - move |mut commands: Commands<'_, '_>, mut scene_spawner: ResMut<'_, SceneSpawner>| { - let entity = commands.spawn_empty().id(); - let id = scene_spawner.spawn_as_child(scene.clone(), entity); - (id, entity) - }, - ); - - // Check for event arrival. - app.update(); - app.world_mut().run_system_once( - move |mut ev_scene: EventReader<'_, '_, SceneInstanceReady>| { - let mut events = ev_scene.read(); - - let event = events.next().expect("found no `SceneInstanceReady` event"); - assert_eq!( - event.id, scene_id, - "`SceneInstanceReady` contains the wrong `InstanceId`" - ); - assert_eq!( - event.parent, - Some(scene_entity), - "`SceneInstanceReady` contains the wrong parent entity" - ); - - assert!(events.next().is_none(), "found more than one event"); - }, - ); - } + // Spawn scene. + let scene_id = + app.world_mut() + .run_system_once(move |mut scene_spawner: ResMut<'_, SceneSpawner>| { + scene_spawner.spawn(scene.clone()) + }); - fn build_dynamic_scene(app: &mut App) -> Handle { - app.world_mut() - .run_system_once(|world: &World, asset_server: Res<'_, AssetServer>| { - asset_server.add(DynamicScene::from_world(world)) - }) + // Check trigger. + observe_trigger(&mut app, scene_id, Entity::PLACEHOLDER); } #[test] - fn event_dynamic_scene() { + fn observe_dynamic_scene() { let mut app = setup(); // Build scene. @@ -659,25 +643,32 @@ mod tests { scene_spawner.spawn_dynamic(scene.clone()) }); - // Check for event arrival. - app.update(); - app.world_mut().run_system_once( - move |mut ev_scene: EventReader<'_, '_, SceneInstanceReady>| { - let mut events = ev_scene.read(); + // Check trigger. + observe_trigger(&mut app, scene_id, Entity::PLACEHOLDER); + } - let event = events.next().expect("found no `SceneInstanceReady` event"); - assert_eq!( - event.id, scene_id, - "`SceneInstanceReady` contains the wrong `InstanceId`" - ); + #[test] + fn observe_scene_as_child() { + let mut app = setup(); + + // Build scene. + let scene = build_scene(&mut app); - assert!(events.next().is_none(), "found more than one event"); + // Spawn scene as child. + let (scene_id, scene_entity) = app.world_mut().run_system_once( + move |mut commands: Commands<'_, '_>, mut scene_spawner: ResMut<'_, SceneSpawner>| { + let entity = commands.spawn_empty().id(); + let id = scene_spawner.spawn_as_child(scene.clone(), entity); + (id, entity) }, ); + + // Check trigger. + observe_trigger(&mut app, scene_id, scene_entity); } #[test] - fn event_dynamic_scene_as_child() { + fn observe_dynamic_scene_as_child() { let mut app = setup(); // Build scene. @@ -692,26 +683,8 @@ mod tests { }, ); - // Check for event arrival. - app.update(); - app.world_mut().run_system_once( - move |mut ev_scene: EventReader<'_, '_, SceneInstanceReady>| { - let mut events = ev_scene.read(); - - let event = events.next().expect("found no `SceneInstanceReady` event"); - assert_eq!( - event.id, scene_id, - "`SceneInstanceReady` contains the wrong `InstanceId`" - ); - assert_eq!( - event.parent, - Some(scene_entity), - "`SceneInstanceReady` contains the wrong parent entity" - ); - - assert!(events.next().is_none(), "found more than one event"); - }, - ); + // Check trigger. + observe_trigger(&mut app, scene_id, scene_entity); } #[test]