From 5ed8e0639aa643b1697a473bd7c6a0e02850e775 Mon Sep 17 00:00:00 2001 From: re0312 <45868716+re0312@users.noreply.github.com> Date: Tue, 6 May 2025 08:12:27 +0800 Subject: [PATCH] Merge ObserverState and Observer into single component (#18728) # Objective - bevy removed `Observe` type parameters in #15151 ,it enables merging `Observer` and `ObserverState ` into a single component. with this consolidation ,we can improve efficiency while reducing boilerplate. ## Solution - remove `ObserverState `and merge it into `Observer` ## Testing 40%~60% performance win due to removal of redundant look up. ![image](https://github.com/user-attachments/assets/eb1d46cb-cca3-4c2b-948c-bf4ecb617de9) This also improves ergonomics when using dynamic observer ```rust // previously world.spawn(ObserverState { // SAFETY: we registered `event_a` above and it matches the type of EventA descriptor: unsafe { ObserverDescriptor::default().with_events(vec![event_a]) }, runner: |mut world, _trigger, _ptr, _propagate| { world.resource_mut::().observed("event_a"); }, ..Default::default() }); // now let observe = unsafe { Observer::with_dynamic_runner(|mut world, _trigger, _ptr, _propagate| { world.resource_mut::().observed("event_a"); }) .with_event(event_a) }; world.spawn(observe); ``` --- .../bevy_ecs/src/observer/entity_observer.rs | 11 +- crates/bevy_ecs/src/observer/mod.rs | 30 +-- crates/bevy_ecs/src/observer/runner.rs | 201 ++++++------------ ...observerState_observer_single_component.md | 17 ++ 4 files changed, 101 insertions(+), 158 deletions(-) create mode 100644 release-content/migration-guides/merge_observerState_observer_single_component.md diff --git a/crates/bevy_ecs/src/observer/entity_observer.rs b/crates/bevy_ecs/src/observer/entity_observer.rs index d69f7764fe..2c2d42b1c9 100644 --- a/crates/bevy_ecs/src/observer/entity_observer.rs +++ b/crates/bevy_ecs/src/observer/entity_observer.rs @@ -3,11 +3,12 @@ use crate::{ Component, ComponentCloneBehavior, ComponentHook, HookContext, Mutable, StorageType, }, entity::{ComponentCloneCtx, Entity, EntityClonerBuilder, EntityMapper, SourceComponent}, - observer::ObserverState, world::World, }; use alloc::vec::Vec; +use super::Observer; + /// Tracks a list of entity observers for the [`Entity`] [`ObservedBy`] is added to. #[derive(Default)] pub struct ObservedBy(pub(crate) Vec); @@ -27,7 +28,7 @@ impl Component for ObservedBy { let Ok(mut entity_mut) = world.get_entity_mut(e) else { continue; }; - let Some(mut state) = entity_mut.get_mut::() else { + let Some(mut state) = entity_mut.get_mut::() else { continue; }; state.despawned_watched_entities += 1; @@ -77,10 +78,10 @@ fn component_clone_observed_by(_source: &SourceComponent, ctx: &mut ComponentClo .entity_mut(target) .insert(ObservedBy(observed_by.clone())); - for observer in &observed_by { + for observer_entity in observed_by.iter().copied() { let mut observer_state = world - .get_mut::(*observer) - .expect("Source observer entity must have ObserverState"); + .get_mut::(observer_entity) + .expect("Source observer entity must have Observer"); observer_state.descriptor.entities.push(target); let event_types = observer_state.descriptor.events.clone(); let components = observer_state.descriptor.components.clone(); diff --git a/crates/bevy_ecs/src/observer/mod.rs b/crates/bevy_ecs/src/observer/mod.rs index 3c1a7a037c..783dafd2d1 100644 --- a/crates/bevy_ecs/src/observer/mod.rs +++ b/crates/bevy_ecs/src/observer/mod.rs @@ -315,13 +315,6 @@ impl ObserverDescriptor { self } - pub(crate) fn merge(&mut self, descriptor: &ObserverDescriptor) { - self.events.extend(descriptor.events.iter().copied()); - self.components - .extend(descriptor.components.iter().copied()); - self.entities.extend(descriptor.entities.iter().copied()); - } - /// Returns the `events` that the observer is watching. pub fn events(&self) -> &[ComponentId] { &self.events @@ -728,11 +721,10 @@ impl World { pub(crate) fn register_observer(&mut self, observer_entity: Entity) { // SAFETY: References do not alias. let (observer_state, archetypes, observers) = unsafe { - let observer_state: *const ObserverState = - self.get::(observer_entity).unwrap(); + let observer_state: *const Observer = self.get::(observer_entity).unwrap(); // Populate ObservedBy for each observed entity. - for watched_entity in &(*observer_state).descriptor.entities { - let mut entity_mut = self.entity_mut(*watched_entity); + for watched_entity in (*observer_state).descriptor.entities.iter().copied() { + let mut entity_mut = self.entity_mut(watched_entity); let mut observed_by = entity_mut.entry::().or_default().into_mut(); observed_by.0.push(observer_entity); } @@ -853,7 +845,7 @@ mod tests { use crate::component::ComponentId; use crate::{ change_detection::MaybeLocation, - observer::{Observer, ObserverDescriptor, ObserverState, OnReplace}, + observer::{Observer, OnReplace}, prelude::*, traversal::Traversal, }; @@ -1368,14 +1360,14 @@ mod tests { world.init_resource::(); let event_a = OnRemove::register_component_id(&mut world); - world.spawn(ObserverState { - // SAFETY: we registered `event_a` above and it matches the type of EventA - descriptor: unsafe { ObserverDescriptor::default().with_events(vec![event_a]) }, - runner: |mut world, _trigger, _ptr, _propagate| { + // SAFETY: we registered `event_a` above and it matches the type of EventA + let observe = unsafe { + Observer::with_dynamic_runner(|mut world, _trigger, _ptr, _propagate| { world.resource_mut::().observed("event_a"); - }, - ..Default::default() - }); + }) + .with_event(event_a) + }; + world.spawn(observe); world.commands().queue(move |world: &mut World| { // SAFETY: we registered `event_a` above and it matches the type of EventA diff --git a/crates/bevy_ecs/src/observer/runner.rs b/crates/bevy_ecs/src/observer/runner.rs index 660460d1ce..ac1caa5ad0 100644 --- a/crates/bevy_ecs/src/observer/runner.rs +++ b/crates/bevy_ecs/src/observer/runner.rs @@ -1,4 +1,4 @@ -use alloc::{boxed::Box, vec, vec::Vec}; +use alloc::{boxed::Box, vec}; use core::any::Any; use crate::{ @@ -12,85 +12,6 @@ use crate::{ }; use bevy_ptr::PtrMut; -/// Contains [`Observer`] information. This defines how a given observer behaves. It is the -/// "source of truth" for a given observer entity's behavior. -pub struct ObserverState { - pub(crate) descriptor: ObserverDescriptor, - pub(crate) runner: ObserverRunner, - pub(crate) last_trigger_id: u32, - pub(crate) despawned_watched_entities: u32, -} - -impl Default for ObserverState { - fn default() -> Self { - Self { - runner: |_, _, _, _| {}, - last_trigger_id: 0, - despawned_watched_entities: 0, - descriptor: Default::default(), - } - } -} - -impl ObserverState { - /// Observe the given `event`. This will cause the [`Observer`] to run whenever an event with the given [`ComponentId`] - /// is triggered. - pub fn with_event(mut self, event: ComponentId) -> Self { - self.descriptor.events.push(event); - self - } - - /// Observe the given event list. This will cause the [`Observer`] to run whenever an event with any of the given [`ComponentId`]s - /// is triggered. - pub fn with_events(mut self, events: impl IntoIterator) -> Self { - self.descriptor.events.extend(events); - self - } - - /// Observe the given [`Entity`] list. This will cause the [`Observer`] to run whenever the [`Event`] is triggered - /// for any [`Entity`] target in the list. - pub fn with_entities(mut self, entities: impl IntoIterator) -> Self { - self.descriptor.entities.extend(entities); - self - } - - /// Observe the given [`ComponentId`] list. This will cause the [`Observer`] to run whenever the [`Event`] is triggered - /// for any [`ComponentId`] target in the list. - pub fn with_components(mut self, components: impl IntoIterator) -> Self { - self.descriptor.components.extend(components); - self - } -} - -impl Component for ObserverState { - const STORAGE_TYPE: StorageType = StorageType::SparseSet; - type Mutability = Mutable; - - fn on_add() -> Option { - Some(|mut world, HookContext { entity, .. }| { - world.commands().queue(move |world: &mut World| { - world.register_observer(entity); - }); - }) - } - - fn on_remove() -> Option { - Some(|mut world, HookContext { entity, .. }| { - let descriptor = core::mem::take( - &mut world - .entity_mut(entity) - .get_mut::() - .unwrap() - .as_mut() - .descriptor, - ); - world.commands().queue(move |world: &mut World| { - world.unregister_observer(entity, descriptor); - }); - }) - } -} - /// Type for function that is run when an observer is triggered. /// /// Typically refers to the default runner that runs the system stored in the associated [`Observer`] component, @@ -264,16 +185,17 @@ pub type ObserverRunner = fn(DeferredWorld, ObserverTrigger, PtrMut, propagate: /// Note that the [`Observer`] component is not added to the entity it is observing. Observers should always be their own entities! /// /// You can call [`Observer::watch_entity`] more than once, which allows you to watch multiple entities with the same [`Observer`]. -/// -/// When first added, [`Observer`] will also create an [`ObserverState`] component, which registers the observer with the [`World`] and /// serves as the "source of truth" of the observer. /// /// [`SystemParam`]: crate::system::SystemParam pub struct Observer { - system: Box, - descriptor: ObserverDescriptor, hook_on_add: ComponentHook, error_handler: Option, + system: Box, + pub(crate) descriptor: ObserverDescriptor, + pub(crate) last_trigger_id: u32, + pub(crate) despawned_watched_entities: u32, + pub(crate) runner: ObserverRunner, } impl Observer { @@ -298,6 +220,35 @@ impl Observer { descriptor: Default::default(), hook_on_add: hook_on_add::, error_handler: None, + runner: observer_system_runner::, + despawned_watched_entities: 0, + last_trigger_id: 0, + } + } + + /// Creates a new [`Observer`] with custom runner, this is mostly used for dynamic event observer + pub fn with_dynamic_runner(runner: ObserverRunner) -> Self { + Self { + system: Box::new(|| {}), + descriptor: Default::default(), + hook_on_add: |mut world, hook_context| { + world.commands().queue(move |world: &mut World| { + let entity = hook_context.entity; + if let Some(mut observe) = world.get_mut::(entity) { + if observe.descriptor.events.is_empty() { + return; + } + if observe.error_handler.is_none() { + observe.error_handler = Some(default_error_handler()); + } + world.register_observer(entity); + } + }); + }, + error_handler: None, + runner, + despawned_watched_entities: 0, + last_trigger_id: 0, } } @@ -358,6 +309,21 @@ impl Component for Observer { hook(world, context); }) } + fn on_remove() -> Option { + Some(|mut world, HookContext { entity, .. }| { + let descriptor = core::mem::take( + &mut world + .entity_mut(entity) + .get_mut::() + .unwrap() + .as_mut() + .descriptor, + ); + world.commands().queue(move |world: &mut World| { + world.unregister_observer(entity, descriptor); + }); + }) + } } fn observer_system_runner>( @@ -373,12 +339,8 @@ fn observer_system_runner>( .get_entity(observer_trigger.observer) .debug_checked_unwrap() }; - // SAFETY: Observer was triggered so must have an `ObserverState` - let mut state = unsafe { - observer_cell - .get_mut::() - .debug_checked_unwrap() - }; + // SAFETY: Observer was triggered so must have an `Observer` + let mut state = unsafe { observer_cell.get_mut::().debug_checked_unwrap() }; // TODO: Move this check into the observer cache to avoid dynamic dispatch let last_trigger = world.last_trigger_id(); @@ -386,15 +348,8 @@ fn observer_system_runner>( return; } state.last_trigger_id = last_trigger; - // SAFETY: Observer was triggered so must have an `Observer` component. - let error_handler = unsafe { - observer_cell - .get::() - .debug_checked_unwrap() - .error_handler - .debug_checked_unwrap() - }; + let error_handler = unsafe { state.error_handler.debug_checked_unwrap() }; let trigger: Trigger = Trigger::new( // SAFETY: Caller ensures `ptr` is castable to `&mut T` @@ -402,12 +357,12 @@ fn observer_system_runner>( propagate, observer_trigger, ); + // SAFETY: // - observer was triggered so must have an `Observer` component. // - observer cannot be dropped or mutated until after the system pointer is already dropped. let system: *mut dyn ObserverSystem = unsafe { - let mut observe = observer_cell.get_mut::().debug_checked_unwrap(); - let system = observe.system.downcast_mut::().unwrap(); + let system = state.system.downcast_mut::().debug_checked_unwrap(); &mut *system }; @@ -461,48 +416,26 @@ fn hook_on_add>( ) { world.commands().queue(move |world: &mut World| { let event_id = E::register_component_id(world); - let mut components = Vec::new(); + let mut components = vec![]; B::component_ids(&mut world.components_registrator(), &mut |id| { components.push(id); }); - let mut descriptor = ObserverDescriptor { - events: vec![event_id], - components, - ..Default::default() - }; + if let Some(mut observe) = world.get_mut::(entity) { + observe.descriptor.events.push(event_id); + observe.descriptor.components.extend(components); - let error_handler = default_error_handler(); - - // Initialize System - let system: *mut dyn ObserverSystem = - if let Some(mut observe) = world.get_mut::(entity) { - descriptor.merge(&observe.descriptor); - if observe.error_handler.is_none() { - observe.error_handler = Some(error_handler); - } - let system = observe.system.downcast_mut::().unwrap(); - &mut *system - } else { - return; - }; - // SAFETY: World reference is exclusive and initialize does not touch system, so references do not alias - unsafe { - (*system).initialize(world); - } - - { - let mut entity = world.entity_mut(entity); - if let crate::world::Entry::Vacant(entry) = entity.entry::() { - entry.insert(ObserverState { - descriptor, - runner: observer_system_runner::, - ..Default::default() - }); + if observe.error_handler.is_none() { + observe.error_handler = Some(default_error_handler()); } + let system: *mut dyn ObserverSystem = observe.system.downcast_mut::().unwrap(); + // SAFETY: World reference is exclusive and initialize does not touch system, so references do not alias + unsafe { + (*system).initialize(world); + } + world.register_observer(entity); } }); } - #[cfg(test)] mod tests { use super::*; diff --git a/release-content/migration-guides/merge_observerState_observer_single_component.md b/release-content/migration-guides/merge_observerState_observer_single_component.md new file mode 100644 index 0000000000..f55962619a --- /dev/null +++ b/release-content/migration-guides/merge_observerState_observer_single_component.md @@ -0,0 +1,17 @@ +--- +title: Integrate `ObserverState` component into `Observer`. +pull_requests: [18728] +--- + +`ObserverState` and `Observer` have been merged into a single component. +now you can use `Observer::with_dynamic_runner` to build custom Observe. + +```rust +let observe = unsafe { + Observer::with_dynamic_runner(|mut world, trigger, ptr, propagate| { + // do something + }) + .with_event(event_a) +}; +world.spawn(observe); +```