Remove `Event: Component` trait bound #17333

alice-i-cecile · 2025-01-12T21:52:56Z

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.

P.S. This prompted a larger hackmd design from me about the core principles of "make everything entities and components".

Context

Dynamic events (which observers support IIRC) require a mapping from TypeId to "some arbitrary stable ID". We decided to use ComponentId there when implementing this, as the code / concept already exists. The easiest way to get the observers API compiling as a result was to just add a Component trait bound.

Solution

Add a dead simple way to get a ComponentId for any Rust types.
Generate the ComponentId for events (inside of the observers API) using this new method.
Relax the Component trait bound on Event to Send + Sync + 'static.

Unlike I initially thought, we cannot (and should not) relax the Component trait bound on register_component, as other work is done to make sure the component is properly set up (like registering hooks).

This change does not:

restrict our ability to implement dynamic buffered events (the main improvement over Make events no longer components #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)

Future work

Use generate_component_id inside of our existing methods to improve clarity and reduce duplication.
Consider renaming ComponentId to something that isn't component-specific.

Testing

Code compiles, and alien_cake_addict works. If something goes wrong here everything will break.

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.

alice-i-cecile · 2025-01-12T21:54:09Z

crates/bevy_ecs/src/component.rs

+    ///
+    /// For read-only access, see [`Components::component_id`].
+    pub fn generate_component_id<T: ?Sized + 'static>(&mut self) -> ComponentId {
+        if let Some(component_id) = self.indices.get(&TypeId::of::<T>()) {


Is .entry faster in general? I find this style a bit clearer.

It would save the second lookup, but that lookup only happens once per type anyways.

alice-i-cecile · 2025-01-12T21:54:28Z

crates/bevy_ecs/src/observer/runner.rs

        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],


Renamed for consistency.

alice-i-cecile · 2025-01-12T21:54:42Z

crates/bevy_ecs/src/event/base.rs

+/// Each event type has a unique [`ComponentId`] associated with it, which is used to store the event in the world.
+/// This can be obtained using [`World::generate_component_id<Self>`].
+///
+/// Each individual event sent has an [`EventId`] associated with it, which can be used to trace the flow of an event.


Bonus docs!

james-j-obrien · 2025-01-12T22:20:35Z

crates/bevy_ecs/src/component.rs

+        if let Some(component_id) = self.indices.get(&TypeId::of::<T>()) {
+            component_id.clone()
+        } else {
+            let id = ComponentId(self.components.len());


If we aren't registering a component info won't this return the same ID multiple times since the length of self.components does not increase?

SpecificProtagonist

I like the general idea.

However, as james has pointed out this doesn't work because we'd return the same id multiple times. On top of that, if the user calls generate_component_id::<Foo> before the first time register_component::<Foo> is called, the initialization of the component descriptor will be skipped.

I see two solutions:

Add a HasComponentId trait that can optionally return a component descriptor (no specialization sadge) and remove generate_component_id; keep track of maximum free id and allocate ids of non-components from high to low so the component descriptor array is kept dense (I'd favor this version)
or have component descriptors be stored in a sparse array, with register_component doing the initialization even if the id is already allocated (but there isn't an entry in the array)

Code compiles, and alien_cake_addict works. If something goes wrong here everything will break.

The reason alien_cake_addict still works is that it doesn't use observers, and even if it did World::bootstrap still uses register_component for events.

SpecificProtagonist · 2025-01-12T22:27:58Z

crates/bevy_ecs/src/component.rs

+    ///
+    /// For read-only access, see [`Components::component_id`].
+    pub fn generate_component_id<T: ?Sized + 'static>(&mut self) -> ComponentId {
+        if let Some(component_id) = self.indices.get(&TypeId::of::<T>()) {


It would save the second lookup, but that lookup only happens once per type anyways.

chescock · 2025-01-13T02:03:08Z

On top of that, if the user calls generate_component_id::<Foo> before the first time register_component::<Foo> is called, the initialization of the component descriptor will be skipped.

I see two solutions:

Add a HasComponentId trait that can optionally return a component descriptor (no specialization sadge) and remove generate_component_id; keep track of maximum free id and allocate ids of non-components from high to low so the component descriptor array is kept dense (I'd favor this version)

or have component descriptors be stored in a sparse array, with register_component doing the initialization even if the id is already allocated (but there isn't an entry in the array)

It would be a little unfortunate to be unable to record any type information for these.
ComponentDescriptor has fields like layout and drop that are useful when working with untyped data. A variation on the second idea could be to split up ComponentInfo into things we can get from any type, and things that only apply to components. Then it's only the component-specific stuff that needs to be sparse.

A third solution is to do what Resource does and have a third TypeIdMap<ComponentId> for these that is separate from indices and resource_indices. Then a single type could be used in all three ways with potentially different configuration.

alice-i-cecile · 2025-01-13T21:21:29Z

I think I'm going to go with solution 3 here: it's the simplest incremental change. I really don't like that you can get duplicate IDs for different types, but that's a pre-existing problem.

SpecificProtagonist · 2025-01-13T22:53:14Z

but that's a pre-existing problem.

How so?

alice-i-cecile · 2025-01-14T22:09:33Z

but that's a pre-existing problem.

How so?

The resources and component indexes are incremented independently now.

alice-i-cecile · 2025-01-15T05:16:37Z

Closing in favor of #17380, which is even simpler.

…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]>

alice-i-cecile added 3 commits January 12, 2025 13:31

Add generate_component_id method

33d3bef

Relax Event: Component trait bound

827a010

Clarify docs on various ids

8805cbf

alice-i-cecile added this to the 0.16 milestone Jan 12, 2025

alice-i-cecile requested review from NthTensor, SpecificProtagonist and maniwani January 12, 2025 21:53

alice-i-cecile commented Jan 12, 2025

View reviewed changes

alice-i-cecile requested a review from james-j-obrien January 12, 2025 22:20

james-j-obrien reviewed Jan 12, 2025

View reviewed changes

SpecificProtagonist suggested changes Jan 12, 2025

View reviewed changes

alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 12, 2025

Fix doc link

3860aac

alice-i-cecile mentioned this pull request Jan 15, 2025

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

Merged

alice-i-cecile closed this Jan 15, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `Event: Component` trait bound #17333

Remove `Event: Component` trait bound #17333

alice-i-cecile commented Jan 12, 2025 •

edited

Loading

alice-i-cecile Jan 12, 2025

SpecificProtagonist Jan 12, 2025

alice-i-cecile Jan 12, 2025

alice-i-cecile Jan 12, 2025

james-j-obrien Jan 12, 2025

SpecificProtagonist left a comment •

edited

Loading

SpecificProtagonist Jan 12, 2025

chescock commented Jan 13, 2025

alice-i-cecile commented Jan 13, 2025

SpecificProtagonist commented Jan 13, 2025

alice-i-cecile commented Jan 14, 2025

alice-i-cecile commented Jan 15, 2025

Remove Event: Component trait bound #17333

Remove Event: Component trait bound #17333

Conversation

alice-i-cecile commented Jan 12, 2025 • edited Loading

Objective

Context

Solution

Future work

Testing

Migration Guide

alice-i-cecile Jan 12, 2025

Choose a reason for hiding this comment

SpecificProtagonist Jan 12, 2025

Choose a reason for hiding this comment

alice-i-cecile Jan 12, 2025

Choose a reason for hiding this comment

alice-i-cecile Jan 12, 2025

Choose a reason for hiding this comment

james-j-obrien Jan 12, 2025

Choose a reason for hiding this comment

SpecificProtagonist left a comment • edited Loading

Choose a reason for hiding this comment

SpecificProtagonist Jan 12, 2025

Choose a reason for hiding this comment

chescock commented Jan 13, 2025

alice-i-cecile commented Jan 13, 2025

SpecificProtagonist commented Jan 13, 2025

alice-i-cecile commented Jan 14, 2025

alice-i-cecile commented Jan 15, 2025

Remove `Event: Component` trait bound #17333

Remove `Event: Component` trait bound #17333

alice-i-cecile commented Jan 12, 2025 •

edited

Loading

SpecificProtagonist left a comment •

edited

Loading