Skip to content

Event Split: Event, EntityEvent, and BufferedEvent #19647

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

Merged
merged 29 commits into from
Jun 15, 2025

Conversation

Jondolf
Copy link
Contributor

@Jondolf Jondolf commented Jun 14, 2025

Objective

Closes #19564.

The current Event trait looks like this:

pub trait Event: Send + Sync + 'static {
    type Traversal: Traversal<Self>;
    const AUTO_PROPAGATE: bool = false;
    
    fn register_component_id(world: &mut World) -> ComponentId { ... }
    fn component_id(world: &World) -> Option<ComponentId> { ... }
}

The Event trait is used by both buffered events (EventReader/EventWriter) and observer events. If they are observer events, they can optionally be targeted at specific Entitys or ComponentIds, and can even be propagated to other entities.

However, there has long been a desire to split the trait semantically for a variety of reasons, see #14843, #14272, and #16031 for discussion. Some reasons include:

  • It's very uncommon to use a single event type as both a buffered event and targeted observer event. They are used differently and tend to have distinct semantics.
  • A common footgun is using buffered events with observers or event readers with observer events, as there is no type-level error that prevents this kind of misuse.
  • Remove entity placeholder from observers #19440 made Trigger::target return an Option<Entity>. This seriously hurts ergonomics for the general case of entity observers, as you need to .unwrap() each time. If we could statically determine whether the event is expected to have an entity target, this would be unnecessary.

There's really two main ways that we can categorize events: push vs. pull (i.e. "observer event" vs. "buffered event") and global vs. targeted:

Push Pull
Global Global observer EventReader/EventWriter
Targeted Entity observer -

There are many ways to approach this, each with their tradeoffs. Ultimately, we kind of want to split events both ways:

  • A type-level distinction between observer events and buffered events, to prevent people from using the wrong kind of event in APIs
  • A statically designated entity target for observer events to avoid accidentally using untargeted events for targeted APIs

This PR achieves these goals by splitting event traits into Event, EntityEvent, and BufferedEvent, with Event being the shared trait implemented by all events.

Event, EntityEvent, and BufferedEvent

Event is now a very simple trait shared by all events.

pub trait Event: Send + Sync + 'static {
    // Required for observer APIs
    fn register_component_id(world: &mut World) -> ComponentId { ... }
    fn component_id(world: &World) -> Option<ComponentId> { ... }
}

You can call trigger for any event, and use a global observer for listening to the event.

#[derive(Event)]
struct Speak {
    message: String,
}

// ...

app.add_observer(|trigger: On<Speak>| {
    println!("{}", trigger.message);
});

// ...

commands.trigger(Speak {
    message: "Y'all like these reworked events?".to_string(),
});

To allow an event to be targeted at entities and even propagated further, you can additionally implement the EntityEvent trait:

pub trait EntityEvent: Event {
    type Traversal: Traversal<Self>;
    const AUTO_PROPAGATE: bool = false;
}

This lets you call trigger_targets, and to use targeted observer APIs like EntityCommands::observe:

#[derive(Event, EntityEvent)]
#[entity_event(traversal = &'static ChildOf, auto_propagate)]
struct Damage {
    amount: f32,
}

// ...

let enemy = commands.spawn((Enemy, Health(100.0))).id();

// Spawn some armor as a child of the enemy entity.
// When the armor takes damage, it will bubble the event up to the enemy.
let armor_piece = commands
    .spawn((ArmorPiece, Health(25.0), ChildOf(enemy)))
    .observe(|trigger: On<Damage>, mut query: Query<&mut Health>| {
        // Note: `On::target` only exists because this is an `EntityEvent`.
        let mut health = query.get(trigger.target()).unwrap();
        health.0 -= trigger.amount();
    });

commands.trigger_targets(Damage { amount: 10.0 }, armor_piece);

Note

You can still also trigger an EntityEvent without targets using trigger. We probably could make this an either-or thing, but I'm not sure that's actually desirable.

To allow an event to be used with the buffered API, you can implement BufferedEvent:

pub trait BufferedEvent: Event {}

The event can then be used with EventReader/EventWriter:

#[derive(Event, BufferedEvent)]
struct Message(String);

fn write_hello(mut writer: EventWriter<Message>) {
    writer.write(Message("I hope these examples are alright".to_string()));
}

fn read_messages(mut reader: EventReader<Message>) {
    // Process all buffered events of type `Message`.
    for Message(message) in reader.read() {
        println!("{message}");
    }
}

In summary:

  • Need a basic event you can trigger and observe? Derive Event!
  • Need the event to be targeted at an entity? Derive EntityEvent!
  • Need the event to be buffered and support the EventReader/EventWriter API? Derive BufferedEvent!

Alternatives

I'll now cover some of the alternative approaches I have considered and briefly explored. I made this section collapsible since it ended up being quite long :P

Expand this to see alternatives

1. Unified Event Trait

One option is not to have three separate traits (Event, EntityEvent, BufferedEvent), and to instead just use associated constants on Event to determine whether an event supports targeting and buffering or not:

pub trait Event: Send + Sync + 'static {
    type Traversal: Traversal<Self>;
    const AUTO_PROPAGATE: bool = false;
    const TARGETED: bool = false;
    const BUFFERED: bool = false;
    
    fn register_component_id(world: &mut World) -> ComponentId { ... }
    fn component_id(world: &World) -> Option<ComponentId> { ... }
}

Methods can then use bounds like where E: Event<TARGETED = true> or where E: Event<BUFFERED = true> to limit APIs to specific kinds of events.

This would keep everything under one Event trait, but I don't think it's necessarily a good idea. It makes APIs harder to read, and docs can't easily refer to specific types of events. You can also create weird invariants: what if you specify TARGETED = false, but have Traversal and/or AUTO_PROPAGATE enabled?

2. Event and Trigger

Another option is to only split the traits between buffered events and observer events, since that is the main thing people have been asking for, and they have the largest API difference.

If we did this, I think we would need to make the terms clearly separate. We can't really use Event and BufferedEvent as the names, since it would be strange that BufferedEvent doesn't implement Event. Something like ObserverEvent and BufferedEvent could work, but it'd be more verbose.

For this approach, I would instead keep Event for the current EventReader/EventWriter API, and call the observer event a Trigger, since the "trigger" terminology is already used in the observer context within Bevy (both as a noun and a verb). This is also what a long bikeshed on Discord seemed to land on at the end of last year.

// For `EventReader`/`EventWriter`
pub trait Event: Send + Sync + 'static {}

// For observers
pub trait Trigger: Send + Sync + 'static {
    type Traversal: Traversal<Self>;
    const AUTO_PROPAGATE: bool = false;
    const TARGETED: bool = false;
    
    fn register_component_id(world: &mut World) -> ComponentId { ... }
    fn component_id(world: &World) -> Option<ComponentId> { ... }
}

The problem is that "event" is just a really good term for something that "happens". Observers are rapidly becoming the more prominent API, so it'd be weird to give them the Trigger name and leave the good Event name for the less common API.

So, even though a split like this seems neat on the surface, I think it ultimately wouldn't really work. We want to keep the Event name for observer events, and there is no good alternative for the buffered variant. (Message was suggested, but saying stuff like "sends a collision message" is weird.)

3. GlobalEvent + TargetedEvent

What if instead of focusing on the buffered vs. observed split, we only make a distinction between global and targeted events?

// A shared event trait to allow global observers to work
pub trait Event: Send + Sync + 'static {
    fn register_component_id(world: &mut World) -> ComponentId { ... }
    fn component_id(world: &World) -> Option<ComponentId> { ... }
}

// For buffered events and non-targeted observer events
pub trait GlobalEvent: Event {}

// For targeted observer events
pub trait TargetedEvent: Event {
    type Traversal: Traversal<Self>;
    const AUTO_PROPAGATE: bool = false;
}

This is actually the first approach I implemented, and it has the neat characteristic that you can only use non-targeted APIs like trigger with a GlobalEvent and targeted APIs like trigger_targets with a TargetedEvent. You have full control over whether the entity should or should not have a target, as they are fully distinct at the type-level.

However, there's a few problems:

  • There is no type-level indication of whether a GlobalEvent supports buffered events or just non-targeted observer events
  • An Event on its own does literally nothing, it's just a shared trait required to make global observers accept both non-targeted and targeted events
  • If an event is both a GlobalEvent and TargetedEvent, global observers again have ambiguity on whether an event has a target or not, undermining some of the benefits
  • The names are not ideal

4. Event and EntityEvent

We can fix some of the problems of Alternative 3 by accepting that targeted events can also be used in non-targeted contexts, and simply having the Event and EntityEvent traits:

// For buffered events and non-targeted observer events
pub trait Event: Send + Sync + 'static {
    fn register_component_id(world: &mut World) -> ComponentId { ... }
    fn component_id(world: &World) -> Option<ComponentId> { ... }
}

// For targeted observer events
pub trait EntityEvent: Event {
    type Traversal: Traversal<Self>;
    const AUTO_PROPAGATE: bool = false;
}

This is essentially identical to this PR, just without a dedicated BufferedEvent. The remaining major "problem" is that there is still zero type-level indication of whether an Event event actually supports the buffered API. This leads us to the solution proposed in this PR, using Event, EntityEvent, and BufferedEvent.

Conclusion

The Event + EntityEvent + BufferedEvent split proposed in this PR aims to solve all the common problems with Bevy's current event model while keeping the "weirdness" factor minimal. It splits in terms of both the push vs. pull and global vs. targeted aspects, while maintaining a shared concept for an "event".

Why I Like This

  • The term "event" remains as a single concept for all the different kinds of events in Bevy.
  • Despite all event types being "events", they use fundamentally different APIs. Instead of assuming that you can use an event type with any pattern (when only one is typically supported), you explicitly opt in to each one with dedicated traits.
  • Using separate traits for each type of event helps with documentation and clearer function signatures.
  • I can safely make assumptions on expected usage.
    • If I see that an event is an EntityEvent, I can assume that I can use observe on it and get targeted events.
    • If I see that an event is a BufferedEvent, I can assume that I can use EventReader to read events.
    • If I see both EntityEvent and BufferedEvent, I can assume that both APIs are supported.

In summary: This allows for a unified concept for events, while limiting the different ways to use them with opt-in traits. No more guess-work involved when using APIs.

Problems?

  • Because BufferedEvent implements Event (for more consistent semantics etc.), you can still use all buffered events for non-targeted observers. I think this is fine/good. The important part is that if you see that an event implements BufferedEvent, you know that the EventReader/EventWriter API should be supported. Whether it also supports other APIs is secondary.
  • I currently only support trigger_targets for an EntityEvent. However, you can technically target components too, without targeting any entities. I consider that such a niche and advanced use case that it's not a huge problem to only support it for EntityEvents, but we could also split trigger_targets into trigger_entities and trigger_components if we wanted to (or implement components as entities :P).
  • You can still trigger an EntityEvent without targets. I consider this correct, since Event implements the non-targeted behavior, and it'd be weird if implementing another trait removed behavior. However, it does mean that global observers for entity events can technically return Entity::PLACEHOLDER again (since I got rid of the Option<Entity> added in Remove entity placeholder from observers #19440 for ergonomics). I think that's enough of an edge case that it's not a huge problem, but it is worth keeping in mind.
  • Deriving both EntityEvent and BufferedEvent for the same type currently duplicates the Event implementation, so you instead need to manually implement one of them. Changed to always requiring Event to be derived.

Related Work

There are plans to implement multi-event support for observers, especially for UI contexts. Cart's example API looked like this:

// Truncated for brevity
trigger: Trigger<(
    OnAdd<Pressed>,
    OnRemove<Pressed>,
    OnAdd<InteractionDisabled>,
    OnRemove<InteractionDisabled>,
    OnInsert<Hovered>,
)>,

I believe this shouldn't be in conflict with this PR. If anything, this PR might help achieve the multi-event pattern for entity observers with fewer footguns: by statically enforcing that all of these events are EntityEvents in the context of EntityCommands::observe, we can avoid misuse or weird cases where some events inside the trigger are targeted while others are not.

@Jondolf Jondolf added A-ECS Entities, components, systems, and events M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide M-Needs-Release-Note Work that should be called out in the blog due to impact X-Contentious There are nontrivial implications that should be thought through D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 14, 2025
@Jondolf Jondolf added this to the 0.17 milestone Jun 14, 2025
@Jondolf Jondolf requested a review from alice-i-cecile June 14, 2025 20:53
@Jondolf Jondolf added C-Docs An addition or correction to our documentation C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jun 14, 2025
@alice-i-cecile
Copy link
Member

#[event(traversal = &'static ChildOf, auto_propagate)]

The syntax for this should probably be changed, since it's no longer coming from the Event trait.

@Jondolf
Copy link
Contributor Author

Jondolf commented Jun 14, 2025

Changed #[event(...)] to #[entity_event(...)]

@Jondolf
Copy link
Contributor Author

Jondolf commented Jun 15, 2025

I tried if we could make the derive macros only implement Event if there isn't already a conflicting derive, but I don't think Rust makes it possible to know which other derives exist from within the derive macro. So I think we either need to:

  1. Accept that if you want to implement both EntityEvent and BufferedEvent, you need to implement at least one of them manually
  2. Don't implement Event automatically in the derive macros, and require users to write all the traits manually each time, like @Bleachfuel suggested

I'm personally mildly in favor of 1. Cases where you need an event to be both an EntityEvent and BufferedEvent are extremely rare (I think Pointer<E> is the only instance of this in Bevy), and the manual implementations are extremely simple anyway, since you don't need to implement any methods. The ergonomic cost of having to manually derive Event each time feels a bit annoying to me.

But I'm fine with either option if y'all have a preference :)

@alice-i-cecile
Copy link
Member

2 is less surprising, and I prefer it because it makes the mental model clearer here. It also works more gracefully. Let's go with that for now; another easy derive on events is fine by me.

@BigWingBeat
Copy link
Contributor

I'd like to share my perspective as someone who has only been using Bevy and hasn't participated in any of the engine design talk. I fully agree that this needs to be changed, I love observers but overloading the Event trait always struck me as needlessly confusing, and counter to Rust's (and Bevy's) compile-time-correctness design philosophy. However, I am thoroughly unconvinced by the argument that was made for this PR's approach to solving this.

I agree with the arguments made for why the current overloaded-trait impl is bad. Namely, that buffered events and observers are two semantically distinct things, and being able to use them interchangeably is a footgun. The most intuitive solution to this is what was listed as number 2 in the Alternatives section, which is to just use two different traits for the two different APIs.

The actually proposed solution, by comparison, makes the traits meaningfully more complicated, seemingly just for the sake of re-introducing the footgun of interchangeability. This is kind-of acknowledged in the Problems section, but no actual justification is given beyond simply stating

I think this is fine/good.

As for the aforementioned alternative solution number 2, the given counterarguments all just amount to "maybe the trait names won't be optimal", which to me is entirely trivial when the benefit is a simpler and more statically correct API.

In my mind, buffered events and observers are two entirely distinct concepts, and the only semantic overlap between them is that they are both "reactive" in a way. If a given "conceptual" event really does need to be expressed with both buffered events and observers, I think it would be entirely fine to (have to) split it across two different concrete types for the two different APIs. Using a single type as both a buffered event and an observer is simply too niche to be worth supporting, when the tradeoff is a tangibly worse API for what is the far more common usage pattern.

@BigWingBeat
Copy link
Contributor

Additionally, (as in, I forgot to mention before posting), I think the arguments in favour of the proposed solution are pretty weak, too. They mention being able to assume that a type is used in a certain way, based on the implemented traits. You may note that assumptions are fallible, while compile-time static typing is not. I entirely disagree with the claim that there will be "no more guess-work". If it's possible to use an API in surprising ways, it certainly will be used in surprising ways.

Copy link
Contributor

@rdrpenguin04 rdrpenguin04 left a comment

Choose a reason for hiding this comment

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

From an end-user perspective, as far as I can see, this looks very easy to use and very intuitive.

@Jondolf
Copy link
Contributor Author

Jondolf commented Jun 15, 2025

@BigWingBeat So I would largely agree with your points. Alternative 2 is what I was proposing for a long time on the Discord, and I too am strongly of the opinion that "buffered events" and "observer events" are basically totally different tools for different use cases, and there isn't much point in unifying them. I'll "briefly" respond to some of your points and then go over some other thoughts :)

I put the responses in this collapsible thingy again since my messages are looong lol

In my mind, buffered events and observers are two entirely distinct concepts, and the only semantic overlap between them is that they are both "reactive" in a way.

The semantic overlap is that they are both "something that happens", and can be read/observed by app logic. Fundamentally, they are both events, they are just used and consumed very differently (pull vs. push). This is why the naming is so difficult here; we basically need to come up with two near-synonymous terms for two totally different APIs when "event" would be a really good name for both.

The actually proposed solution, by comparison, makes the traits meaningfully more complicated, seemingly just for the sake of re-introducing the footgun of interchangeability. This is kind-of acknowledged in the Problems section, but no actual justification is given beyond simply stating "I think this is fine/good."

The motivation there is that we get to keep a single "event" concept that defaults to a global observer event, but you can opt in to additional targeting and buffering, which (with this PR's semantics) are considered additional behaviors on top of the base Event. As long as we have a shared Event trait, it would be weird if another trait like BufferedEvent didn't have the capabilities of the base Event, so we need to allow usage with global observers there.

Like you mention, this does re-introduce some footguns, since you can use a BufferedEvent with the global observer API. But that's really the only big problem there. You cannot use an EntityEvent with the buffered API unless BufferedEvent is also implemented, and you cannot use a BufferedEvent with targeted observer APIs (the most common footgun ime) unless EntityEvent is also implemented.

Still, I agree that this is a problem, and it would be clearer if you simply couldn't use a BufferedEvent even with non-targeted observers by default.

I think the arguments in favour of the proposed solution are pretty weak, too. They mention being able to assume that a type is used in a certain way, based on the implemented traits. You may note that assumptions are fallible, while compile-time static typing is not. I entirely disagree with the claim that there will be "no more guess-work". If it's possible to use an API in surprising ways, it certainly will be used in surprising ways.

Yeah, I would agree here, and "no more guess-work" was a bit of an overstatement. My argument there is mainly that there is absolutely no reason to implement BufferedEvent if you don't actually support using EventReader/EventWriter for the event, so for a user, seeing it implemented on a type should be a pretty clear indication that the event most likely supports those APIs.

But again, it would be more reassuring if this assumption was actually upheld at the type-level, and you weren't allowed to use the types in the wrong contexts to begin with.


I implemented the approach in this PR instead of Approach 2 mainly because:

  • "Event" is just a good name for something that "happens", and makes sense for both observer events and buffered events, which implies a shared trait.
  • Fundamentally, both observer events and buffered events are events, they are just used and consumed differently, which lends itself to the "implement additional traits to opt in to additional behavior" model while keeping some shared semantics.
  • Cart really seemed to want to keep the "event" name for at least observer events, and I would agree with him there. But I have not seen a compelling alternative name for buffered events. Message and FeedReader/FeedWriter was suggested in the past, but I really don't want to call collision events "collision messages". And also "a message" is often sent to specific receivers, which implies targeting.
  • Assuming we do use the term "event" for both buffered events and observer events, but don't want them to share a trait, what would we call them? For clearly distinct names I proposed Event and Trigger (and even made Rename Trigger to On #19596 to free up the Trigger name for this), but again this would mean that observer events wouldn't be called events. We could also use Event and BufferedEvent (where BufferedEvent does not implement Event) or ObserverEvent and BufferedEvent, but those don't feel quite ideal either, not sure.
  • I believe Cart also mentioned an Event + BufferedEvent + EntityEvent split as a possible option.

So to fix the problems you pointed out, we would need something similar to Approach 2, the Event + Trigger split, but we would need to make a call on the naming there. I would like to hear @cart's thoughts on this topic.

Additionally, there's the question of how we want to handle targeting for observer events if we go the route of fully statically splitting event types. Do we:

  • Use a single observer event trait like Trigger with an associated TARGETED bool constant to determine whether targeting is supported. This means that you can only define an event with or without a target, not sometimes use it with a target and sometimes without.
  • Use a single observer event trait like Trigger with an associated Target type that can take something like () (non-targeted), Entity (targeted), or Option<Entity> (targeted or non-targeted).
  • Use separate traits like GlobalTrigger and EntityTrigger for non-targeted and targeted events, respectively. We would also need a shared Trigger trait at least internally to not duplicate lots of internals. If both traits are implemented, you can sometimes use the event with a target and sometimes without.

So yeah... lot's of complexity here :P

@IQuick143
Copy link
Contributor

I'd maybe add my two uneducated cents regarding naming:
Perhaps buffered events could not be called events.

As you mentioned, Event is a word that describes "something happening", but if I go a little philosophical, BufferedEvents aren't that much of a "is happening" thing.
"Things" in ECS "happen" all the time, it's when something mutates some state somewhere in the ECS. It thus makes perfect sense to me that whenever an observer sees such a change, sees an event occur, it would be called an event.
Meanwhile BufferedEvents are not directly linked to something happening, at best they're emitted by a system that did something/noticed something has happened. They seem more like reports or announcements. "I have changed the number of enemy units, someone do something about it if you care" or "A new level is being instantiated, everyone (referring to systems) please do the appropriate steps."
I heard someone, though I don't remember their name, suggesting calling them "Message"s instead, which would make sense at least to my mental model. It would also fit with the terminology and usage APIs:
Messages get Written/sent and Read.
Events happen, they don't get written or sent, but you can Observe them happening and react to them.

Regarding Option events, as a consumer of them, I'd be most happy if I didn't have to unwrap the target on events when not needed. I think that having a type-level classification here makes sense because the type should define what semantics it has, notably what semantics does the target of the event have. An "EntityExploded" event would have fairly obvious semantics regarding what the target did, but undefined semantics if there was a None.

Sorry if this is either an already-discussed territory or if I'm going off the rails with this, I'm not the biggest Observer user (let alone knowing how they work internally) as I haven't had time to experiment with them more. But the conflation between the message-passing Events I'm used to and Observer events was definitely a source of initial confusion for me; so even the PR as is counts as a major improvement by me.

@Jondolf
Copy link
Contributor Author

Jondolf commented Jun 15, 2025

My physics engine POV may be in play here, but I just really can't see us using a term other than "event" for buffered events. In the context of a physics engine, there might be contact events, sensor events, movement events, and so on, and all physics engines that have these sorts of buffered/streamed events just use the term "event" (Box2D, PhysX, Rapier, Avian...). It is simply a good term for what it is, and going against the grain here would not be a good idea in my opinion.

I really dislike the commonly proposed Message name in particular because:

  • It's a very misleading analogy to me. When I think of a "message" in common language, it is typically targeted at someone either explicitly or implicitly, whether it is a DM or a reply in some thread. This possible targeting implies that it is an observer event, not necessarily a non-targeted buffered event.
  • I really, really don't want to call collision events "collision messages" :( I would still just call them collision events and say that e.g. "CollisionStarted is a Message for the collision started event", which kinda defeats the whole point of keeping the names separate.

So from my personal POV, the naming options for buffered events and observer events are

  1. What this PR does
  2. Keep calling both event types "events", but clearly disambiguate them. For example BufferedEvent and ObserverEvent, or BufferedEvent and GlobalEvent/EntityEvent.
  3. Keep buffered events as "events", but use a different term for the more instantaneous "observer events", like Trigger, Signal, or Observation.

(Signal would be nice, but overlaps too much and in the wrong ways with the reactivity concept... so Trigger would be my preference for option 3)

@rendaoer
Copy link
Contributor

If 3, then changing Trigger to On seems strange🤔

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Good! Really nice writing, and thoughtful API design. I'm still not fully sold on the existence of the Event trait, but this is 100% a step in the right direction, and doesn't break any existing functionality while preserving Cart's desire for a soft-unification of the ideas. I'm going to merge this and we can iterate further during this cycle: things like bikeshedding names or tweaking behavior can be done in a much more reviewable fashion in followup.

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 15, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 15, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 15, 2025
@BigWingBeat
Copy link
Contributor

I totally understand your perspective, I suppose it comes down to a difference in priorities and design philosophy.

"Event" is just a good name for something that "happens", and makes sense for both observer events and buffered events, which implies a shared trait.

This to me seems like the core of the disagreement. The idea that two things being conceptually similar necessarily implies that the similarity be reflected in the type system, simply doesn't make sense to me. Buffered events and observers are definitely conceptually similar, but, as has been conceded several times, the practical implementations are indeed very different. I would argue that the only thing you need to adequately reflect the conceptual similarity is good documentation and code organization, even if type-system-wise the traits are entirely distinct.

Additionally, the majority of the discussion is (still) focused on what names the traits should have, which is something I personally value significantly less than the type-system-wise "shape" of the API. If good naming conventions are really so important to Bevy's design philosophy that people think it's worth having a worse API for, that's a fundamental difference in perspective that I don't think can be reconciled between us. I'm not going to continue arguing over this if the decision has already been made, but I will maintain my stance that this is an inferior solution.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 15, 2025
@Jondolf
Copy link
Contributor Author

Jondolf commented Jun 15, 2025

Yup, I would agree that this is an inferior solution to Approach 2 (the Event + Trigger split) in some ways. I was (and to some extent, still am) advocating for that sort of split too!

But I also think that having a good naming convention here is extremely important. If we can't even name the concepts used in our APIs in a clear and consistent way, then the whole system and its teachability kinda falls apart imo. Event + Trigger is still my top suggestion there, but it seemed like Cart (at the time when I proposed it) was against it, so I kinda left that be for now.

if the decision has already been made

The decision for this PR has been made, but we can totally revisit this :)

Even though there are some disagreements on the details, people seem to generally agree that this is at least better than the status quo, so it's good to move forward for now and we can iterate on the design further in follow-ups.

Merged via the queue into bevyengine:main with commit 38c3423 Jun 15, 2025
32 checks passed
@Jondolf Jondolf deleted the split-event-types branch June 15, 2025 17:10
@andrewzhurov
Copy link

Nice split for added compile-time correctness (event.target() available only on an EntityEvent).

Presently though, it falls a bit short, as we can .trigger_targets(SomeEvent, vec![])
In which case current_target is None and event.target() returns a placeholder (e.g., when events is handled by a global observer).
Same with .trigger(MeantToBeTargetedEvent).

Is this really fine for a user or compile-time error is better? 🤔
(imo, legit argument for the latter here #19440)

But then, how that extra step correctness could look like?
My thinking, if we have a generic Event that represents 'what happened' (imo nice), seems we need to be able to parameterize it with any extra behavior. So there's one derive that decides what flavor this event is.

#[derive(Event)]
struct SomeEvent; // which can be used with `.trigger()` and its kind

#[derive(Event)]
#[event(targeted)] // will derive EntityEvent
struct SomeTargetedEvent; // which can be used with `.trigger_targets()` and it's kind, but not `.trigger()`

Good, bad, ugly? Let me know!
Here's the modified Event's derive.


Not familiar with pain points that led to introduction of BufferedEvent

"Event" is just a good name for something that "happens", and makes sense for both observer events and buffered events, which implies a shared trait.

Why not have all events bufferable?

@alice-i-cecile
Copy link
Member

I agree with most of your critiques: we're planning to revisit this split within the 0.17 cycle. Our current plan can be found at https://hackmd.io/@bevy/rk4S92hmlg, but drop by the Observers Overhaul working group on Discord too for more real-time discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Docs An addition or correction to our documentation C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide M-Needs-Release-Note Work that should be called out in the blog due to impact S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Statically designate event targets
8 participants