From be35cba801ab8f4aab7b1263c797a81bcd21f05e Mon Sep 17 00:00:00 2001 From: Zachary Harrold Date: Wed, 11 Sep 2024 11:14:28 +1000 Subject: [PATCH] Removed Type Parameters from `Observer` (#15151) # Objective - Remove any ambiguity around how multiple `Observer` components work on a single `Entity` by completely removing the concept. - Fixes #15122 ## Solution - Removed type parameters from `Observer`, relying on a function pointer to provide type information into the relevant aspects of running an observer. ## Testing - Ran CI locally. - Checked `observers.rs` example continued to function as expected. ## Notes This communicates to users of observers that only a single `Observer` can be inserted onto an entity at a time within the established type system. This has been achieved by erasing the type information from the stored `ObserverSystem` and retrieving it again using a function pointer. This has the downside of increasing the size of the `Observer` component and increases the complexity of the observer runner. However, this complexity was already present, and is in my opinion a worthwhile tradeoff for the clearer user experience. The other notable benefit is users no longer need to use the `ObserverState` component to filter for `Observer` entities, and can instead use `Observer` directly. Technically this is a breaking change, since the type signature for `Observer` has changed. However, it was so cumbersome to use that I don't believe there are any instances in the wild of users directly naming `Observer` types, instead relying on `ObserverState`, and the methods provided by `App` and `World`. As can be seen in the diff, this change had very little knock-on effects across Bevy. ## Migration Guide If you filtered for observers using `Observer`, instead filter for an `Observer`. --------- Co-authored-by: Alice Cecile --- crates/bevy_ecs/src/observer/runner.rs | 141 ++++++++++++++----------- 1 file changed, 81 insertions(+), 60 deletions(-) diff --git a/crates/bevy_ecs/src/observer/runner.rs b/crates/bevy_ecs/src/observer/runner.rs index 1706ead914..2b5c8f5077 100644 --- a/crates/bevy_ecs/src/observer/runner.rs +++ b/crates/bevy_ecs/src/observer/runner.rs @@ -1,5 +1,7 @@ +use std::any::Any; + use crate::{ - component::{ComponentHooks, ComponentId, StorageType}, + component::{ComponentHook, ComponentHooks, ComponentId, StorageType}, observer::{ObserverDescriptor, ObserverTrigger}, prelude::*, query::DebugCheckedUnwrap, @@ -257,22 +259,23 @@ pub type ObserverRunner = fn(DeferredWorld, ObserverTrigger, PtrMut, propagate: /// 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. [`ObserverState`] can be used to filter for [`Observer`] [`Entity`]s, e.g. -/// [`With`]. +/// serves as the "source of truth" of the observer. /// /// [`SystemParam`]: crate::system::SystemParam -pub struct Observer { - system: BoxedObserverSystem, +pub struct Observer { + system: Box, descriptor: ObserverDescriptor, + hook_on_add: ComponentHook, } -impl Observer { +impl Observer { /// Creates a new [`Observer`], which defaults to a "global" observer. This means it will run whenever the event `E` is triggered /// for _any_ entity (or no entity). - pub fn new(system: impl IntoObserverSystem) -> Self { + pub fn new>(system: I) -> Self { Self { system: Box::new(IntoObserverSystem::into_system(system)), descriptor: Default::default(), + hook_on_add: hook_on_add::, } } @@ -308,54 +311,20 @@ impl Observer { } } -impl Component for Observer { +impl Component for Observer { const STORAGE_TYPE: StorageType = StorageType::SparseSet; fn register_component_hooks(hooks: &mut ComponentHooks) { - hooks.on_add(|mut world, entity, _| { - world.commands().add(move |world: &mut World| { - let event_type = world.init_component::(); - let mut components = Vec::new(); - B::component_ids(&mut world.components, &mut world.storages, &mut |id| { - components.push(id); - }); - let mut descriptor = ObserverDescriptor { - events: vec![event_type], - components, - ..Default::default() - }; - - // Initialize System - let system: *mut dyn ObserverSystem = - if let Some(mut observe) = world.get_mut::(entity) { - descriptor.merge(&observe.descriptor); - &mut *observe.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() - }); - } - } - }); + hooks.on_add(|world, entity, _id| { + let Some(observe) = world.get::(entity) else { + return; + }; + let hook = observe.hook_on_add; + hook(world, entity, _id); }); } } -/// Equivalent to [`BoxedSystem`](crate::system::BoxedSystem) for [`ObserverSystem`]. -pub type BoxedObserverSystem = Box>; - -fn observer_system_runner( +fn observer_system_runner>( mut world: DeferredWorld, observer_trigger: ObserverTrigger, ptr: PtrMut, @@ -395,23 +364,75 @@ fn observer_system_runner( // This transmute is obviously not ideal, but it is safe. Ideally we can remove the // static constraint from ObserverSystem, but so far we have not found a way. let trigger: Trigger<'static, E, B> = unsafe { std::mem::transmute(trigger) }; - // SAFETY: Observer was triggered so must have an `Observer` component. - let system = unsafe { - &mut observer_cell - .get_mut::>() - .debug_checked_unwrap() - .system + // 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(); + &mut *system }; - system.update_archetype_component_access(world); - // SAFETY: - // - `update_archetype_component_access` was just called + // - `update_archetype_component_access` is called first // - there are no outstanding references to world except a private component // - system is an `ObserverSystem` so won't mutate world beyond the access of a `DeferredWorld` // - system is the same type erased system from above unsafe { - system.run_unsafe(trigger, world); - system.queue_deferred(world.into_deferred()); + (*system).update_archetype_component_access(world); + (*system).run_unsafe(trigger, world); + (*system).queue_deferred(world.into_deferred()); } } + +/// A [`ComponentHook`] used by [`Observer`] to handle its [`on-add`](`ComponentHooks::on_add`). +/// +/// This function exists separate from [`Observer`] to allow [`Observer`] to have its type parameters +/// erased. +/// +/// The type parameters of this function _must_ match those used to create the [`Observer`]. +/// As such, it is recommended to only use this function within the [`Observer::new`] method to +/// ensure type parameters match. +fn hook_on_add>( + mut world: DeferredWorld<'_>, + entity: Entity, + _: ComponentId, +) { + world.commands().add(move |world: &mut World| { + let event_type = world.init_component::(); + let mut components = Vec::new(); + B::component_ids(&mut world.components, &mut world.storages, &mut |id| { + components.push(id); + }); + let mut descriptor = ObserverDescriptor { + events: vec![event_type], + components, + ..Default::default() + }; + + // Initialize System + let system: *mut dyn ObserverSystem = + if let Some(mut observe) = world.get_mut::(entity) { + descriptor.merge(&observe.descriptor); + 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() + }); + } + } + }); +}