-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Conversation
The syntax for this should probably be changed, since it's no longer coming from the |
Changed |
I tried if we could make the derive macros only implement
I'm personally mildly in favor of 1. Cases where you need an event to be both an But I'm fine with either option if y'all have a preference :) |
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. |
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
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. |
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. |
There was a problem hiding this 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.
@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
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 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 Like you mention, this does re-introduce some footguns, since you can use a Still, I agree that this is a problem, and it would be clearer if you simply couldn't use a
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 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:
So to fix the problems you pointed out, we would need something similar to Approach 2, the 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:
So yeah... lot's of complexity here :P |
I'd maybe add my two uneducated cents regarding naming: 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. 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. |
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
So from my personal POV, the naming options for buffered events and observer events are
( |
If 3, then changing Trigger to On seems strange🤔 |
There was a problem hiding this 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.
I totally understand your perspective, I suppose it comes down to a difference in priorities and design philosophy.
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. |
Yup, I would agree that this is an inferior solution to Approach 2 (the 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.
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. |
Nice split for added compile-time correctness ( Presently though, it falls a bit short, as we can Is this really fine for a user or compile-time error is better? 🤔 But then, how that extra step correctness could look like? #[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! Not familiar with pain points that led to introduction of BufferedEvent
Why not have all events bufferable? |
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. |
Objective
Closes #19564.
The current
Event
trait looks like this: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 specificEntity
s orComponentId
s, 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:
Trigger::target
return anOption<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:
EventReader
/EventWriter
There are many ways to approach this, each with their tradeoffs. Ultimately, we kind of want to split events both ways:
This PR achieves these goals by splitting event traits into
Event
,EntityEvent
, andBufferedEvent
, withEvent
being the shared trait implemented by all events.Event
,EntityEvent
, andBufferedEvent
Event
is now a very simple trait shared by all events.You can call
trigger
for any event, and use a global observer for listening to the event.To allow an event to be targeted at entities and even propagated further, you can additionally implement the
EntityEvent
trait:This lets you call
trigger_targets
, and to use targeted observer APIs likeEntityCommands::observe
:Note
You can still also trigger an
EntityEvent
without targets usingtrigger
. 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
:The event can then be used with
EventReader
/EventWriter
:In summary:
Event
!EntityEvent
!EventReader
/EventWriter
API? DeriveBufferedEvent
!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
TraitOne option is not to have three separate traits (
Event
,EntityEvent
,BufferedEvent
), and to instead just use associated constants onEvent
to determine whether an event supports targeting and buffering or not:Methods can then use bounds like
where E: Event<TARGETED = true>
orwhere 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 specifyTARGETED = false
, but haveTraversal
and/orAUTO_PROPAGATE
enabled?2.
Event
andTrigger
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
andBufferedEvent
as the names, since it would be strange thatBufferedEvent
doesn't implementEvent
. Something likeObserverEvent
andBufferedEvent
could work, but it'd be more verbose.For this approach, I would instead keep
Event
for the currentEventReader
/EventWriter
API, and call the observer event aTrigger
, 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.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 goodEvent
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?
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 aGlobalEvent
and targeted APIs liketrigger_targets
with aTargetedEvent
. 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:
GlobalEvent
supports buffered events or just non-targeted observer eventsEvent
on its own does literally nothing, it's just a shared trait required to make global observers accept both non-targeted and targeted eventsGlobalEvent
andTargetedEvent
, global observers again have ambiguity on whether an event has a target or not, undermining some of the benefits4.
Event
andEntityEvent
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
andEntityEvent
traits: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 anEvent
event actually supports the buffered API. This leads us to the solution proposed in this PR, usingEvent
,EntityEvent
, andBufferedEvent
.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
EntityEvent
, I can assume that I can useobserve
on it and get targeted events.BufferedEvent
, I can assume that I can useEventReader
to read events.EntityEvent
andBufferedEvent
, 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?
BufferedEvent
implementsEvent
(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 implementsBufferedEvent
, you know that theEventReader
/EventWriter
API should be supported. Whether it also supports other APIs is secondary.trigger_targets
for anEntityEvent
. 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 forEntityEvent
s, but we could also splittrigger_targets
intotrigger_entities
andtrigger_components
if we wanted to (or implement components as entities :P).EntityEvent
without targets. I consider this correct, sinceEvent
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 returnEntity::PLACEHOLDER
again (since I got rid of theOption<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 bothChanged to always requiringEntityEvent
andBufferedEvent
for the same type currently duplicates theEvent
implementation, so you instead need to manually implement one of them.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:
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
EntityEvent
s in the context ofEntityCommands::observe
, we can avoid misuse or weird cases where some events inside the trigger are targeted while others are not.