Observer trigger refactor (#19935)

# Objective

- The usage of ComponentId is quite confusing: events are not
components. By newtyping this, we can prevent stupid mistakes, avoid
leaking internal details and make the code clearer for users and engine
devs reading it.
- Adopts #19755

---------

Co-authored-by: oscar-benderstone <oscarbenderstone@gmail.com>
Co-authored-by: Oscar Bender-Stone <88625129+oscar-benderstone@users.noreply.github.com>

Author: Zeophlite

crates/bevy_ecs/src/event/base.rs

@@ -94,23 +94,23 @@ use core::{
     note = "consider annotating `{Self}` with `#[derive(Event)]`"
 )]
 pub trait Event: Send + Sync + 'static {
-    /// Generates the [`ComponentId`] for this event type.
+    /// Generates the [`EventKey`] for this event type.
     ///
     /// If this type has already been registered,
-    /// this will return the existing [`ComponentId`].
+    /// this will return the existing [`EventKey`].
     ///
     /// This is used by various dynamically typed observer APIs,
     /// such as [`World::trigger_targets_dynamic`].
     ///
     /// # Warning
     ///
     /// This method should not be overridden by implementers,
-    /// and should always correspond to the implementation of [`component_id`](Event::component_id).
-    fn register_component_id(world: &mut World) -> ComponentId {
-        world.register_component::<EventWrapperComponent<Self>>()
+    /// and should always correspond to the implementation of [`event_key`](Event::event_key).
+    fn register_event_key(world: &mut World) -> EventKey {
+        EventKey(world.register_component::<EventWrapperComponent<Self>>())
     }
 
-    /// Fetches the [`ComponentId`] for this event type,
+    /// Fetches the [`EventKey`] for this event type,
     /// if it has already been generated.
     ///
     /// This is used by various dynamically typed observer APIs,
@@ -119,9 +119,12 @@ pub trait Event: Send + Sync + 'static {
     /// # Warning
     ///
     /// This method should not be overridden by implementers,
-    /// and should always correspond to the implementation of [`register_component_id`](Event::register_component_id).
-    fn component_id(world: &World) -> Option<ComponentId> {
-        world.component_id::<EventWrapperComponent<Self>>()
+    /// and should always correspond to the implementation of
+    /// [`register_event_key`](Event::register_event_key).
+    fn event_key(world: &World) -> Option<EventKey> {
+        world
+            .component_id::<EventWrapperComponent<Self>>()
+            .map(EventKey)
     }
 }
 
@@ -421,3 +424,19 @@ pub(crate) struct EventInstance<E: BufferedEvent> {
     pub event_id: EventId<E>,
     pub event: E,
 }
+
+/// A unique identifier for an [`Event`], used by [observers].
+///
+/// You can look up the key for your event by calling the [`Event::event_key`] method.
+///
+/// [observers]: crate::observer
+#[derive(Debug, Copy, Clone, Hash, Ord, PartialOrd, Eq, PartialEq)]
+pub struct EventKey(pub(crate) ComponentId);
+
+impl EventKey {
+    /// Returns the internal [`ComponentId`].
+    #[inline]
+    pub(crate) fn component_id(&self) -> ComponentId {
+        self.0
+    }
+}

crates/bevy_ecs/src/event/mod.rs

@@ -11,7 +11,7 @@ mod update;
 mod writer;
 
 pub(crate) use base::EventInstance;
-pub use base::{BufferedEvent, EntityEvent, Event, EventId};
+pub use base::{BufferedEvent, EntityEvent, Event, EventId, EventKey};
 pub use bevy_ecs_macros::{BufferedEvent, EntityEvent, Event};
 pub use collections::{Events, SendBatchIds};
 pub use event_cursor::EventCursor;

crates/bevy_ecs/src/event/registry.rs

@@ -1,15 +1,15 @@
 use alloc::vec::Vec;
 use bevy_ecs::{
     change_detection::{DetectChangesMut, MutUntyped},
-    component::{ComponentId, Tick},
-    event::{BufferedEvent, Events},
+    component::Tick,
+    event::{BufferedEvent, EventKey, Events},
     resource::Resource,
     world::World,
 };
 
 #[doc(hidden)]
 struct RegisteredEvent {
-    component_id: ComponentId,
+    event_key: EventKey,
     // Required to flush the secondary buffer and drop events even if left unchanged.
     previously_updated: bool,
     // SAFETY: The component ID and the function must be used to fetch the Events<T> resource
@@ -51,7 +51,7 @@ impl EventRegistry {
         let component_id = world.init_resource::<Events<T>>();
         let mut registry = world.get_resource_or_init::<Self>();
         registry.event_updates.push(RegisteredEvent {
-            component_id,
+            event_key: EventKey(component_id),
             previously_updated: false,
             update: |ptr| {
                 // SAFETY: The resource was initialized with the type Events<T>.
@@ -66,7 +66,9 @@ impl EventRegistry {
     pub fn run_updates(&mut self, world: &mut World, last_change_tick: Tick) {
         for registered_event in &mut self.event_updates {
             // Bypass the type ID -> Component ID lookup with the cached component ID.
-            if let Some(events) = world.get_resource_mut_by_id(registered_event.component_id) {
+            if let Some(events) =
+                world.get_resource_mut_by_id(registered_event.event_key.component_id())
+            {
                 let has_changed = events.has_changed_since(last_change_tick);
                 if registered_event.previously_updated || has_changed {
                     // SAFETY: The update function pointer is called with the resource
@@ -87,7 +89,7 @@ impl EventRegistry {
         let mut registry = world.get_resource_or_init::<Self>();
         registry
             .event_updates
-            .retain(|e| e.component_id != component_id);
+            .retain(|e| e.event_key.component_id() != component_id);
         world.remove_resource::<Events<T>>();
     }
 }

crates/bevy_ecs/src/lib.rs

@@ -79,7 +79,8 @@ pub mod prelude {
         entity::{ContainsEntity, Entity, EntityMapper},
         error::{BevyError, Result},
         event::{
-            BufferedEvent, EntityEvent, Event, EventMutator, EventReader, EventWriter, Events,
+            BufferedEvent, EntityEvent, Event, EventKey, EventMutator, EventReader, EventWriter,
+            Events,
         },
         hierarchy::{ChildOf, ChildSpawner, ChildSpawnerCommands, Children},
         lifecycle::{

crates/bevy_ecs/src/lifecycle.rs

@@ -55,7 +55,7 @@ use crate::{
     entity::Entity,
     event::{
         BufferedEvent, EntityEvent, Event, EventCursor, EventId, EventIterator,
-        EventIteratorWithId, Events,
+        EventIteratorWithId, EventKey, Events,
     },
     query::FilteredAccessSet,
     relationship::RelationshipHookMode,
@@ -314,16 +314,16 @@ impl ComponentHooks {
     }
 }
 
-/// [`ComponentId`] for [`Add`]
-pub const ADD: ComponentId = ComponentId::new(0);
-/// [`ComponentId`] for [`Insert`]
-pub const INSERT: ComponentId = ComponentId::new(1);
-/// [`ComponentId`] for [`Replace`]
-pub const REPLACE: ComponentId = ComponentId::new(2);
-/// [`ComponentId`] for [`Remove`]
-pub const REMOVE: ComponentId = ComponentId::new(3);
-/// [`ComponentId`] for [`Despawn`]
-pub const DESPAWN: ComponentId = ComponentId::new(4);
+/// [`EventKey`] for [`Add`]
+pub const ADD: EventKey = EventKey(ComponentId::new(0));
+/// [`EventKey`] for [`Insert`]
+pub const INSERT: EventKey = EventKey(ComponentId::new(1));
+/// [`EventKey`] for [`Replace`]
+pub const REPLACE: EventKey = EventKey(ComponentId::new(2));
+/// [`EventKey`] for [`Remove`]
+pub const REMOVE: EventKey = EventKey(ComponentId::new(3));
+/// [`EventKey`] for [`Despawn`]
+pub const DESPAWN: EventKey = EventKey(ComponentId::new(4));
 
 /// Trigger emitted when a component is inserted onto an entity that does not already have that
 /// component. Runs before `Insert`.

crates/bevy_ecs/src/observer/centralized_storage.rs

@@ -37,44 +37,44 @@ pub struct Observers {
     remove: CachedObservers,
     despawn: CachedObservers,
     // Map from trigger type to set of observers listening to that trigger
-    cache: HashMap<ComponentId, CachedObservers>,
+    cache: HashMap<EventKey, CachedObservers>,
 }
 
 impl Observers {
-    pub(crate) fn get_observers_mut(&mut self, event_type: ComponentId) -> &mut CachedObservers {
+    pub(crate) fn get_observers_mut(&mut self, event_key: EventKey) -> &mut CachedObservers {
         use crate::lifecycle::*;
 
-        match event_type {
+        match event_key {
             ADD => &mut self.add,
             INSERT => &mut self.insert,
             REPLACE => &mut self.replace,
             REMOVE => &mut self.remove,
             DESPAWN => &mut self.despawn,
-            _ => self.cache.entry(event_type).or_default(),
+            _ => self.cache.entry(event_key).or_default(),
         }
     }
 
-    /// Attempts to get the observers for the given `event_type`.
+    /// Attempts to get the observers for the given `event_key`.
     ///
     /// When accessing the observers for lifecycle events, such as [`Add`], [`Insert`], [`Replace`], [`Remove`], and [`Despawn`],
-    /// use the [`ComponentId`] constants from the [`lifecycle`](crate::lifecycle) module.
-    pub fn try_get_observers(&self, event_type: ComponentId) -> Option<&CachedObservers> {
+    /// use the [`EventKey`] constants from the [`lifecycle`](crate::lifecycle) module.
+    pub fn try_get_observers(&self, event_key: EventKey) -> Option<&CachedObservers> {
         use crate::lifecycle::*;
 
-        match event_type {
+        match event_key {
             ADD => Some(&self.add),
             INSERT => Some(&self.insert),
             REPLACE => Some(&self.replace),
             REMOVE => Some(&self.remove),
             DESPAWN => Some(&self.despawn),
-            _ => self.cache.get(&event_type),
+            _ => self.cache.get(&event_key),
         }
     }
 
-    /// This will run the observers of the given `event_type`, targeting the given `entity` and `components`.
+    /// This will run the observers of the given `event_key`, targeting the given `entity` and `components`.
     pub(crate) fn invoke<T>(
         mut world: DeferredWorld,
-        event_type: ComponentId,
+        event_key: EventKey,
         current_target: Option<Entity>,
         original_target: Option<Entity>,
         components: impl Iterator<Item = ComponentId> + Clone,
@@ -88,7 +88,7 @@ impl Observers {
             // SAFETY: There are no outstanding world references
             world.increment_trigger_id();
             let observers = world.observers();
-            let Some(observers) = observers.try_get_observers(event_type) else {
+            let Some(observers) = observers.try_get_observers(event_key) else {
                 return;
             };
             // SAFETY: The only outstanding reference to world is `observers`
@@ -102,7 +102,7 @@ impl Observers {
                 world.reborrow(),
                 ObserverTrigger {
                     observer,
-                    event_type,
+                    event_key,
                     components: components.clone().collect(),
                     current_target,
                     original_target,
@@ -145,10 +145,10 @@ impl Observers {
         });
     }
 
-    pub(crate) fn is_archetype_cached(event_type: ComponentId) -> Option<ArchetypeFlags> {
+    pub(crate) fn is_archetype_cached(event_key: EventKey) -> Option<ArchetypeFlags> {
         use crate::lifecycle::*;
 
-        match event_type {
+        match event_key {
             ADD => Some(ArchetypeFlags::ON_ADD_OBSERVER),
             INSERT => Some(ArchetypeFlags::ON_INSERT_OBSERVER),
             REPLACE => Some(ArchetypeFlags::ON_REPLACE_OBSERVER),

crates/bevy_ecs/src/observer/distributed_storage.rs

@@ -289,9 +289,9 @@ impl Observer {
     /// Observe the given `event`. This will cause the [`Observer`] to run whenever an event with the given [`ComponentId`]
     /// is triggered.
     /// # Safety
-    /// The type of the `event` [`ComponentId`] _must_ match the actual value
+    /// The type of the `event` [`EventKey`] _must_ match the actual value
     /// of the event passed into the observer system.
-    pub unsafe fn with_event(mut self, event: ComponentId) -> Self {
+    pub unsafe fn with_event(mut self, event: EventKey) -> Self {
         self.descriptor.events.push(event);
         self
     }
@@ -350,7 +350,7 @@ impl Component for Observer {
 #[derive(Default, Clone)]
 pub struct ObserverDescriptor {
     /// The events the observer is watching.
-    pub(super) events: Vec<ComponentId>,
+    pub(super) events: Vec<EventKey>,
 
     /// The components the observer is watching.
     pub(super) components: Vec<ComponentId>,
@@ -362,9 +362,9 @@ pub struct ObserverDescriptor {
 impl ObserverDescriptor {
     /// Add the given `events` to the descriptor.
     /// # Safety
-    /// The type of each [`ComponentId`] in `events` _must_ match the actual value
+    /// The type of each [`EventKey`] in `events` _must_ match the actual value
     /// of the event passed into the observer.
-    pub unsafe fn with_events(mut self, events: Vec<ComponentId>) -> Self {
+    pub unsafe fn with_events(mut self, events: Vec<EventKey>) -> Self {
         self.events = events;
         self
     }
@@ -382,7 +382,7 @@ impl ObserverDescriptor {
     }
 
     /// Returns the `events` that the observer is watching.
-    pub fn events(&self) -> &[ComponentId] {
+    pub fn events(&self) -> &[EventKey] {
         &self.events
     }
 
@@ -410,13 +410,13 @@ fn hook_on_add<E: Event, B: Bundle, S: ObserverSystem<E, B>>(
     HookContext { entity, .. }: HookContext,
 ) {
     world.commands().queue(move |world: &mut World| {
-        let event_id = E::register_component_id(world);
+        let event_key = E::register_event_key(world);
         let mut components = alloc::vec![];
         B::component_ids(&mut world.components_registrator(), &mut |id| {
             components.push(id);
         });
         if let Some(mut observer) = world.get_mut::<Observer>(entity) {
-            observer.descriptor.events.push(event_id);
+            observer.descriptor.events.push(event_key);
             observer.descriptor.components.extend(components);
 
             let system: &mut dyn Any = observer.system.as_mut();

crates/bevy_ecs/src/observer/entity_cloning.rs

@@ -43,10 +43,10 @@ fn component_clone_observed_by(_source: &SourceComponent, ctx: &mut ComponentClo
                 .get_mut::<Observer>(observer_entity)
                 .expect("Source observer entity must have Observer");
             observer_state.descriptor.entities.push(target);
-            let event_types = observer_state.descriptor.events.clone();
+            let event_keys = observer_state.descriptor.events.clone();
             let components = observer_state.descriptor.components.clone();
-            for event_type in event_types {
-                let observers = world.observers.get_observers_mut(event_type);
+            for event_key in event_keys {
+                let observers = world.observers.get_observers_mut(event_key);
                 if components.is_empty() {
                     if let Some(map) = observers.entity_observers.get(&source).cloned() {
                         observers.entity_observers.insert(target, map);