Remove Event: Component trait bound using a wrapper type which impls …

…Component (#17380)

# Objective

As raised in #17317, the `Event:
Component` trait bound is confusing to users.

In general, a type `E` (like `AppExit`) which implements `Event` should
not:

- be stored as a component on an entity
- be a valid option for `Query<&AppExit>`
- require the storage type and other component metadata to be specified

Events are not components (even if they one day use some of the same
internal mechanisms), and this trait bound is confusing to users.

We're also automatically generating `Component` impls with our derive
macro, which should be avoided when possible to improve explicitness and
avoid conflicts with user impls.

Closes #17317, closes #17333

## Solution

- We only care that each unique event type gets a unique `ComponentId`
- dynamic events need their own tools for getting identifiers anyways
- This avoids complicating the internals of `ComponentId` generation.
- Clearly document why this cludge-y solution exists.

In the medium term, I think that either a) properly generalizing
`ComponentId` (and moving it into `bevy_reflect?) or b) using a
new-typed `Entity` as the key for events is more correct. This change is
stupid simple though, and removes the offending trait bound in a way
that doesn't introduce complex tech debt and does not risk changes to
the internals.

This change does not:

- restrict our ability to implement dynamic buffered events (the main
improvement over #17317)
- there's still a fair bit of work to do, but this is a step in the
right direction
- limit our ability to store event metadata on entities in the future
- make it harder for users to work with types that are both events and
components (just add the derive / trait bound)

## Migration Guide

The `Event` trait no longer requires the `Component` trait. If you were
relying on this behavior, change your trait bounds from `Event` to
`Event + Component`. If you also want your `Event` type to implement
`Component`, add a derive.

---------

Co-authored-by: Chris Russell <[email protected]>

crates/bevy_ecs/macros/src/component.rs

@@ -29,11 +29,6 @@ pub fn derive_event(input: TokenStream) -> TokenStream {
             type Traversal = ();
             const AUTO_PROPAGATE: bool = false;
         }
-
-        impl #impl_generics #bevy_ecs_path::component::Component for #struct_name #type_generics #where_clause {
-            const STORAGE_TYPE: #bevy_ecs_path::component::StorageType = #bevy_ecs_path::component::StorageType::SparseSet;
-            type Mutability = #bevy_ecs_path::component::Mutable;
-        }
     })
 }
 

crates/bevy_ecs/src/event/base.rs

@@ -1,3 +1,6 @@
+use crate as bevy_ecs;
+use crate::component::ComponentId;
+use crate::world::World;
 use crate::{component::Component, traversal::Traversal};
 #[cfg(feature = "bevy_reflect")]
 use bevy_reflect::Reflect;
@@ -19,10 +22,6 @@ use core::{
 ///
 /// This trait can be derived.
 ///
-/// Events implement the [`Component`] type (and they automatically do when they are derived). Events are (generally)
-/// not directly inserted as components. More often, the [`ComponentId`] is used to identify the event type within the
-/// context of the ECS.
-///
 /// Events must be thread-safe.
 ///
 /// [`World`]: crate::world::World
@@ -36,7 +35,7 @@ use core::{
     label = "invalid `Event`",
     note = "consider annotating `{Self}` with `#[derive(Event)]`"
 )]
-pub trait Event: Component {
+pub trait Event: Send + Sync + 'static {
     /// The component that describes which Entity to propagate this event to next, when [propagation] is enabled.
     ///
     /// [propagation]: crate::observer::Trigger::propagate
@@ -48,8 +47,53 @@ pub trait Event: Component {
     /// [triggered]: crate::system::Commands::trigger_targets
     /// [`Trigger::propagate`]: crate::observer::Trigger::propagate
     const AUTO_PROPAGATE: bool = false;
+
+    /// Generates the [`ComponentId`] for this event type.
+    ///
+    /// If this type has already been registered,
+    /// this will return the existing [`ComponentId`].
+    ///
+    /// This is used by various dynamically typed observer APIs,
+    /// such as [`World::trigger_targets_dynamic`].
+    ///
+    /// # Warning
+    ///
+    /// This method should not be overridden by implementors,
+    /// 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>>()
+    }
+
+    /// Fetches the [`ComponentId`] for this event type,
+    /// if it has already been generated.
+    ///
+    /// This is used by various dynamically typed observer APIs,
+    /// such as [`World::trigger_targets_dynamic`].
+    ///
+    /// # Warning
+    ///
+    /// This method should not be overridden by implementors,
+    /// 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>>()
+    }
 }
 
+/// An internal type that implements [`Component`] for a given [`Event`] type.
+///
+/// This exists so we can easily get access to a unique [`ComponentId`] for each [`Event`] type,
+/// without requiring that [`Event`] types implement [`Component`] directly.
+/// [`ComponentId`] is used internally as a unique identitifier for events because they are:
+///
+/// - Unique to each event type.
+/// - Can be quickly generated and looked up.
+/// - Are compatible with dynamic event types, which aren't backed by a Rust type.
+///
+/// This type is an implementation detail and should never be made public.
+// TODO: refactor events to store their metadata on distinct entities, rather than using `ComponentId`
+#[derive(Component)]
+struct EventWrapperComponent<E: Event + ?Sized>(PhantomData<E>);
+
 /// An `EventId` uniquely identifies an event stored in a specific [`World`].
 ///
 /// An `EventId` can among other things be used to trace the flow of an event from the point it was

crates/bevy_ecs/src/observer/mod.rs

@@ -521,7 +521,7 @@ impl World {
     /// those that don't will be consumed and will no longer be accessible.
     /// If you need to use the event after triggering it, use [`World::trigger_ref`] instead.
     pub fn trigger<E: Event>(&mut self, mut event: E) {
-        let event_id = self.register_component::<E>();
+        let event_id = E::register_component_id(self);
         // SAFETY: We just registered `event_id` with the type of `event`
         unsafe { self.trigger_targets_dynamic_ref(event_id, &mut event, ()) };
     }
@@ -531,7 +531,7 @@ impl World {
     /// Compared to [`World::trigger`], this method is most useful when it's necessary to check
     /// or use the event after it has been modified by observers.
     pub fn trigger_ref<E: Event>(&mut self, event: &mut E) {
-        let event_id = self.register_component::<E>();
+        let event_id = E::register_component_id(self);
         // SAFETY: We just registered `event_id` with the type of `event`
         unsafe { self.trigger_targets_dynamic_ref(event_id, event, ()) };
     }
@@ -542,7 +542,7 @@ impl World {
     /// those that don't will be consumed and will no longer be accessible.
     /// If you need to use the event after triggering it, use [`World::trigger_targets_ref`] instead.
     pub fn trigger_targets<E: Event>(&mut self, mut event: E, targets: impl TriggerTargets) {
-        let event_id = self.register_component::<E>();
+        let event_id = E::register_component_id(self);
         // SAFETY: We just registered `event_id` with the type of `event`
         unsafe { self.trigger_targets_dynamic_ref(event_id, &mut event, targets) };
     }
@@ -553,7 +553,7 @@ impl World {
     /// Compared to [`World::trigger_targets`], this method is most useful when it's necessary to check
     /// or use the event after it has been modified by observers.
     pub fn trigger_targets_ref<E: Event>(&mut self, event: &mut E, targets: impl TriggerTargets) {
-        let event_id = self.register_component::<E>();
+        let event_id = E::register_component_id(self);
         // SAFETY: We just registered `event_id` with the type of `event`
         unsafe { self.trigger_targets_dynamic_ref(event_id, event, targets) };
     }
@@ -995,7 +995,7 @@ mod tests {
     fn observer_multiple_events() {
         let mut world = World::new();
         world.init_resource::<Order>();
-        let on_remove = world.register_component::<OnRemove>();
+        let on_remove = OnRemove::register_component_id(&mut world);
         world.spawn(
             // SAFETY: OnAdd and OnRemove are both unit types, so this is safe
             unsafe {
@@ -1154,7 +1154,7 @@ mod tests {
     fn observer_dynamic_trigger() {
         let mut world = World::new();
         world.init_resource::<Order>();
-        let event_a = world.register_component::<EventA>();
+        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

crates/bevy_ecs/src/observer/runner.rs

@@ -398,13 +398,13 @@ fn hook_on_add<E: Event, B: Bundle, S: ObserverSystem<E, B>>(
     _: ComponentId,
 ) {
     world.commands().queue(move |world: &mut World| {
-        let event_type = world.register_component::<E>();
+        let event_id = E::register_component_id(world);
         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],
+            events: vec![event_id],
             components,
             ..Default::default()
         };

crates/bevy_ecs/src/world/mod.rs

@@ -147,10 +147,18 @@ impl World {
     /// This _must_ be run as part of constructing a [`World`], before it is returned to the caller.
     #[inline]
     fn bootstrap(&mut self) {
-        assert_eq!(ON_ADD, self.register_component::<OnAdd>());
-        assert_eq!(ON_INSERT, self.register_component::<OnInsert>());
-        assert_eq!(ON_REPLACE, self.register_component::<OnReplace>());
-        assert_eq!(ON_REMOVE, self.register_component::<OnRemove>());
+        // The order that we register these events is vital to ensure that the constants are correct!
+        let on_add = OnAdd::register_component_id(self);
+        assert_eq!(ON_ADD, on_add);
+
+        let on_insert = OnInsert::register_component_id(self);
+        assert_eq!(ON_INSERT, on_insert);
+
+        let on_replace = OnReplace::register_component_id(self);
+        assert_eq!(ON_REPLACE, on_replace);
+
+        let on_remove = OnRemove::register_component_id(self);
+        assert_eq!(ON_REMOVE, on_remove);
     }
     /// Creates a new empty [`World`].
     ///