Skip to content

ObserverTrigger: Add wrapper type around ComponentId #19755

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 37 additions & 9 deletions crates/bevy_ecs/src/event/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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_type`](Event::event_type).
fn register_event_type(world: &mut World) -> EventKey {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that these names would be clearer and more consistent as register_event_key / event_key.

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,
Expand All @@ -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_type`](Event::register_event_type).
fn event_type(world: &World) -> Option<EventKey> {
world
.component_id::<EventWrapperComponent<Self>>()
.map(EventKey)
}
}

Expand Down Expand Up @@ -421,3 +424,28 @@ pub(crate) struct EventInstance<E: BufferedEvent> {
pub event_id: EventId<E>,
pub event: E,
}

/// # Warning
///
/// This struct should only be instantiated internally to this crate.
///
/// [`EventKey`]s are created in:
/// - `crate::observer::trigger_dynamic_ref_with_caller`
/// - `crate::observer::trigger_dynamic_ref_with_caller`
/// - `crate::observer::observer_multiple_events`
/// - `crate::observer::observer_dynamic_trigger`
/// - `crate::observer::runner::hook_on_add`
Comment on lines +428 to +437
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// # Warning
///
/// This struct should only be instantiated internally to this crate.
///
/// [`EventKey`]s are created in:
/// - `crate::observer::trigger_dynamic_ref_with_caller`
/// - `crate::observer::trigger_dynamic_ref_with_caller`
/// - `crate::observer::observer_multiple_events`
/// - `crate::observer::observer_dynamic_trigger`
/// - `crate::observer::runner::hook_on_add`
/// 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

We don't need to give a warning about non-public construction: Rust makes that impossible with correct usage of the visibility system. But we should give some docs about what this type is for and how to get it.

#[derive(Debug, Copy, Clone, Hash, Ord, PartialOrd, Eq, PartialEq)]
pub struct EventKey(pub(crate) ComponentId);

impl EventKey {
/// Returns id of the underlying [`Event`].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Returns id of the underlying [`Event`].
/// Returns the internal [`ComponentId`].

/// Used internally in:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This list will be really hard to maintain. Better to leave this off, and allow users to use their IDEs go-to-uses instead.

/// - [`crate::event::register_event`]
/// - [`crate::event::run_updates`]
/// - [`crate::event::deregister_events`]
#[inline]
pub(crate) fn component_id(&self) -> ComponentId {
self.0
}
}
2 changes: 1 addition & 1 deletion crates/bevy_ecs/src/event/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
15 changes: 9 additions & 6 deletions crates/bevy_ecs/src/event/registry.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
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_type: EventKey,
// event_type:
// 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
Expand Down Expand Up @@ -51,7 +52,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_type: EventKey(component_id),
previously_updated: false,
update: |ptr| {
// SAFETY: The resource was initialized with the type Events<T>.
Expand All @@ -66,7 +67,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_type.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
Expand All @@ -87,7 +90,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_type.component_id() != component_id);
world.remove_resource::<Events<T>>();
}
}
3 changes: 2 additions & 1 deletion crates/bevy_ecs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down
22 changes: 11 additions & 11 deletions crates/bevy_ecs/src/lifecycle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ use crate::{
entity::Entity,
event::{
BufferedEvent, EntityEvent, Event, EventCursor, EventId, EventIterator,
EventIteratorWithId, Events,
EventIteratorWithId, EventKey, Events,
},
query::FilteredAccessSet,
relationship::RelationshipHookMode,
Expand Down Expand Up @@ -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`.
Expand Down
Loading
Loading