Event Rearchitecture

[cart]

Note: This is a draft PR. It needs a quality pass and a bunch of doc changes. Don't waste bandwidth by commenting on the doc stuff

There is general consensus that our terminology for Events, "entity events", Observers, and BufferedEvents needs clarity. Additionally, many of us also agree that the current Observer system would benefit from additional static-ness: currently it is assumed that you can use events in pretty much any context, and they all go through the exact same code path.

Alice put forth a proposal to Overhaul Observers, and we have already partially implemented it for 0.17. I think it does a great job of outlining many of the issues at play, and it solves them reasonably well. But I also think the proposed solution isn't yet ideal. Given that it is already partially implemented for 0.17, it is a breaking change, and given that we have already broken the Observer API a number of times, I think we need to sort this out before the next release.

This is a big changeset, but it is largely just a reframing of what is already there. I haven't fundamentally changed the behaviors. I've just refined and constrained in a way that allows us to do what we are currently doing in a clearer, simpler, and more performant way.

First, I'll give some quick notes on Alice's proposal (which you all should read if you haven't yet!):

Notes on Alice's Proposal

• I like the move toward a more static API
• I think we've gone too far down the "separate terminology" path. The proposal introduces a zoo of apis, terms, and "subterms". I think we need to simplify our concepts and names to make this all easier to talk about and use in practice.
• BroadcastEvent feels like the wrong name. EntityEvent is also "broadcast" in the exact same way
• BufferedEvent is a completely different system than EntityEvent and BroadcastEvent. This muddles concepts too much. It needs its own standalone, single-word concept name.
• "Universal observers": I think this should be fully context driven, rather than needing encoding in the API.
• I agree we can't get rid of buffered events, and that merging them with "broadcast events" isn't helpful
• I'm not quite sure how we'd make the proposed PropagateEvent subtrait work transparently. This can't be "layered on top" as a trait. It needs to be baked in at more fundamental level.

• I don't like app.add_broadcast_observers(), app.add_universal_observers(), Observer::entity_observer, Observer::broadcast, etc. The On event should statically determine whether an observer is an "entity observer" or a "broadcast" Observer. This would already be encoded in the type system and is therefore something we can do on the developer's behalf. Likewise, any observer being registered at a top level is inherently not a specific entity observer. All of these variants serve to make users guess and poke around in a way that is unnecessary. I want simple one word concept names, single constructors, etc.

Proposed Principals

• Static-ness:
  • Events should only be usable in the context they were defined to be used.
  • When triggered, Observers should only have access to fields and behaviors that are relevant:
    • Dont return Option or PLACEHOLDER: the field or function shouldn't exist
    • Entity events that don't support propagation shouldn't expose that functionality
• Don't do unnecessary work at runtime
  • Event triggers shouldn't branch through every potential event code path
  • Don't clone potentially large lists of event context unnecessarily (Ex: we currently clone the component list for every observer invocation)
• Minimize codegen
  • Don't recompile things redundantly.
  • Don't compile unnecessary code paths.
• Clear and Simple
  • Minimize the number of concept names floating around, and lock each concept down heavily to a specific context
  • I'm convinced at this point that "buffered events" and "observer events" sharing concept names is wrong. We need two clean and clear terms, and I'm willing to give "buffered events" a slightly worse name if it means "observer events" can be nicer.
  • Don't throw the concept name "Event" out ... it is a very good name. Instead, constrain it to one specific thing.
  • Minimize our API surface
  • Events contain all context, including what used to be the "target". This lets people define the "target" name that makes the most sense for the context, and lets the documentation fully describe the context of that "target".

Concepts

• Event (the thing you "observe")
  • Rationale: "Event" is the clear choice for this concept. An "event" feels like something that happens in real time. "Event observers" are things that observe events when they occur (are triggered). Additionally, this is the concept that "propagates", and "event propagation" is a term people understand.
  • Trigger: (the verb that "causes" events to happen for targets). Events are Triggered. This can include additional context/ data that is passed to observers / informs the trigger behavior. Events have exactly one Trigger. If you want a different trigger behavior, define a new event. This makes the system more static, more predictable, and easier to understand and document. world.trigger_ref_with makes it possible to pass in mutable reference to your own Trigger data, making it possible to customize the input trigger data and read out the final trigger data.
  • Observer (the thing that "observes" events): An event's Trigger determines which observers will run.
  • Event Types: You can build any "type" of event. The concept of a "target" has been removed. Instead, define a Trigger that expects a specific kind of event (ex: E: EntityEvent).
    • EntityEvent We add a new EntityEvent trait, which defines an event.entity() accessor. This is used by the Trigger impls : EntityTrigger, PropagateEntityTrigger, and EntityComponentsTrigger.
• Message (the buffered thing you "read" and "write")
  • Message is a solid metaphor for what this is ... it is data that is written and then at some later point read by someone / something else. I expect existing consumers of "buffered events" to lament this name change, as "event" feels nicer. But having a separate name is within everyone's best interest.
  • MessageReader (the thing that reads messages)
  • MessageWriter (the thing that writes messages)

The Changes

• Event trait changes
  • Event is now used exclusively by Observers
  • Added Event::Trigger, which defines what trigger implementation this event will use
• Added the Trigger trait
  • All of the shared / hard-coded observer trigger logic has been broken out into individual context-specific Trigger traits.
• "Trigger Targets" have been removed.
  • Instead, Events, in combination with their Trigger impl, decide how they will be triggered. In general, this means that Events now include their "targets" as fields on the event.
  • APIs like trigger_targets have been replaced by trigger, which can now be used for any Event
• EntityEvent trait changes
  • Propagation config has been removed from the EntityEvent trait. It now lives on the Trigger trait (specifically the PropagateEntityTrigger trait).
  • EntityEvent now provides entity / entity_mut accessors for the Event it is implemented for
  • EntityEvent defaults to having no propagation (uses the simpler EntityTrigger)
  • #[entity_event(propagate)] enables the "default" propagation logic (uses ChildOf). The existing #[entity_event(traversal = X)] has been renamed to #[entity_event(propagate = X)
  • Deriving EntityEvent requires either a single MyEvent(Entity), the entity field name (MyEvent { entity: Entity}), or MyEvent { #[event_entity] custom: Entity }
• Animation event changes
  • Animation events now have their own AnimationEvent trait, which sets the AnimationEventTrigger. This allows developers to pass in events that dont include the Entity field (as this is set by the system). The custom trigger also opens the doors to cheaply passing in additional animation system context, accessible through On
• EntityComponentsTrigger
  • The built in Add/Remove/etc lifecycle events now use the EntityComponentsTrigger, which passes in the components as additional state. This significantly cuts down on clones, as it does a borrow rather than cloning the list into each observer execution.
  • Each event now has an entity field.
• Style changes
  • Prefer the event name for variables: explode: On<Explode> not event: On<Explode>
  • Prefer using the direct field name for the entity on entity events, rather than event.entity(). This allows us to use more specific names where appropriate, provides better / more contextual docs, and coaches developers to think of On<MyEvent> as the event itself.

Take a look at the changes to the examples and the built-in events to see what this looks like in practice.

Downsides

• Moving the "target" into the event adds some new constraints:
  • Triggering the same event for multiple entities requires multiple trigger calls. For "expensive" events (ex: lots of data attached to the event), this will be more awkward. Your options become:
    • Create multiple instances of the event, cloning the expensive data
    • Use trigger_ref, and mutate the event on each call to change the target.
    • Move the "expensive" shared data into the Trigger, and use `trigger_ref_with``
    • We could build a new EntityEvent method that abstracts over the "event mutation" behavior and provides something like the old trigger_target behavior.
    • Use a different EntityTargetTrigger (not currently provided by bevy, but we could), which brings back the old behavior. This would be used with trigger_with to replicate the old pattern: world.trigger_with(MyEvent, [e1, e2].into()) (or we could make the into() implicit)
  • Bubbling the event involves mutating the event to set the entity. This means that trigger_ref will result in the event's EntityEvent::entity() being the final bubbled entity instead of the initial entity.
  • Some APIs (trivially) benefit from the "target entity" being separate from the event. Specifically, this new API requires changes to the "Animation Event" system in AnimationPlayer. I think this is actually a good change set, as it allows us to:
    • Cheaply expose more animation state as part of a new AnimationEventTrigger impl
    • Move that "implict" entity target provided by the AnimationPlayer into the AnimationEventTrigger
    • Encode the "animation event trigger-ness" of the event into the type itself (by requiring #[event(trigger = AnimationEventTrigger)])
    • By not implementing Default for AnimationEventTrigger, we can block animation events from being fired manually by the user.

Draft TODO

• Fill in documentation and update existing docs
• Benchmark: I expect this impl to be significantly faster. There might also be tangible binary size improvements, as I've removed a lot of redundant codegen.
• Update release notes and migration guides

Next Steps

• The BufferedEvent -> Message rename was not included to keep the size down.

Fixes #19648

[cart]

Gotta resolve some conflicts with main.

[Jondolf]

(not related to any specific code, just opening a thread)

I expect existing consumers of "buffered events" to lament this name change, as "event" feels nicer.

As one such consumer, I'll just voice my concerns on the "message" name.

• Collision events are called "events" practically everywhere that supports them. Unity, Unreal Engine, Box2D, Rapier, Avian, Bepu (manually via callbacks), and so on. These tend to be buffered, or sometimes only supported via callbacks/hooks, but I have not seen a term other than "event" for the actual "thing" you are reading. Should we then diverge from the standard naming used everywhere and call them "collision messages"? I'm not sure if I'm comfortable with that.
• Related to the above, Avian currently has a CollisionEventsEnabled component. This enables both buffered and observer events for a collider, with the intent that both are fundamentally events, and it is up to the user how they want to consume them, either with a "per-entity" observer API or a buffered API for more performant batched event processing. If we split these concepts so that buffered collision events are no longer "events", this suddenly becomes more awkward, as we need to either split the component into CollisionEventsEnabled and CollisionMessagesEnabled (I can see a lot of people reaching for the former and being confused why they're not getting buffered events), or by renaming to something like ContactReportingEnabled, which removes the event/message concept name and is a lot more ambiguous.
• To me, a "message" in everyday life is way more targeted than an event is. You don't simply send a message into the void and have arbitrary consumers read it; they are more commonly scoped to a specific target, whether that is a DM on Discord, a thread on Reddit, or a response to someone's tweet. Messages are targeted either implicitly or explicitly, so to me the semantics are quite different from what I'd expect for (most) non-observer events.

[SteveAlexander]

In support of “message”, this is the term used by Erlang (and Elixir) to describe a data structure that is sent from one actor to another via an asynchronous buffer.

[alice-i-cecile]

To me, a "message" in everyday life is way more targeted than an event is.

This is my opinion too. I don't particularly care though: both events and messages are good names, and as long as they have distinct names I'm happy enough.

[viridia]

In my mind, an "event" represents an instantaneous happening, whereas a "message" is something that is stored and forwarded like an email. There's an entire sub-industry built around "message queueing", in which messages are routed and processed in ways that are fundamentally buffered.

[cart]

Because you are treating both behaviors as the "same" currently, I think you can largely dodge this terminology change:

1. Continue using the CollisionEventsEnabled component.
2. Continue referring to the events produced by Avian as "events".
3. Derive both EntityEvent and Message for your events
4. Consider it as "we are also sending copies of these Collision Events as Messages" / "this Message is a Collision Event"

Aka I don't think you need to have a separate set of OnCollisionStartEvent / OnCollisionStartMessage types.

To me, a "message" in everyday life is way more targeted than an event is.

This is a reasonable point. Currently buffered EventWriters write their events without knowing who will consume them. However they do write to a specific place (the buffer). Its like someone is writing a message on a public bulletin board (or in a public forum). So perhaps MessageWriter is a better name than MessageSender (which more tightly confines the context to a specific set of receivers).

[viridia]

So perhaps MessageWriter is a better name than MessageSender

This would be consistent with queuing systems: one tasks writes to a MessageQueue, another reads from it.

[alice-i-cecile]

I think we need to sort this out before the next release.

Fully agree here :) Changes to internals can wait, but I would really like to nail down the fundamental user facing API and terminology. Really glad you're taking a look here.

I'm aligned with you on the core principles: less duplication, more staticness, more efficiency! I'll leave more detailed thoughts in review comments to allow them to be collapsed and threaded.

[viridia]

@cart Since you are working on improving the docs, could you also address #19648 ? It should be just a cut & paste.

[SkiFire13]

This safety comment is not entirely correct. The caller can only guarantee that trigger_ptr is castable to &mut E::Trigger<'a> for some lifetime 'a chosen by the caller to trigger_with and similar methods. Here however we're casting it to a &mut E::Trigger<'w> for some arbitrary lifetime 'w. This is fine only as long as nothing about the lifetime 'w that 's different from 'a is ever exposed to safe code, effectively forcing the safe code to be universally quantifed over a set of lifetimes that must include the original 'a.

Practically speaking, a requirement (but I'm not sure if this is sufficient!) is that On must never expose anything with the 'w lifetime except E::Trigger<'w> (even &'w mut E::Trigger<'w> or &'w E::Trigger<'w> would be unsound!) and nothing else must be exposed with the same 'w lifetime as a On<'w, E, B>. This is not documented anywhere and seems very easy to accidentally break, especially with how On is defined.

[cart]

Hmm isn't this just constraining the lifetime to a smaller scope though? We are pretending that the 'a in Trigger<'a> is actually the lifetime of the references to Trigger<'a> (which we call 'w). In that context, the lifetime of 'a must be greater than or equal to 'w, for any 'w right?

[cart]

@tychedelia I'm moving your comment here to support threaded conversation.

@tychedelia's original comment:

I'm convinced at this point that "buffered events" and "observer events" sharing concept names is wrong. We need two clean and clear terms, and I'm willing to give "buffered events" a slightly worse name if it means "observer events" can be nicer.

Overall I prefer the direction of this PR to the conceptual muddle we were wading into, but I'm not totally sure I understand why sharing concept names is wrong here? Event/message tends to be pretty exchangeable in practice. For example, the Apache Kafka docs note:

An event records the fact that "something happened" in the world or in your business. It is also called record or message in the documentation.

This maybe depends a bit on your background, and I do think that "event" being targeted is intuitive in UI development in a way that message is not. But being targeted is also a property of buffered message queues, for example event sourcing patterns where you are observing a sequence of OnAdd, OnChange, OnRemove messages targeting certain database rows, etc. Indeed, pull based systems are often used to back more ergonomic push based APIs, see Spring cloud streams, SQS Lambda triggers, etc.

So I think the naming here is absolutely correct if they indeed should be separate, but at least to this reader it doesn't exactly resolve the ambiguity for me. Buffering vs pushing still feels like an implementation detail to me.

Maybe the argument just really is that in our game engine specifically, observers have a kind of special status? Whereas messages are more just a general utility you could absolutely re-implement yourself. I think I'm convinced of that.

[cart]

Event/message tends to be pretty exchangeable in practice. For example, the Apache Kafka [...]

I agree that in the industry "message" and "event" are often interchangeable concepts.

So I think the naming here is absolutely correct if they indeed should be separate, but at least to this reader it doesn't exactly resolve the ambiguity for me. Buffering vs pushing still feels like an implementation detail to me.

This is about contracts, teaching, predictability, and expectation management. It is not an implementation detail. Events and Messages differ on many fronts from a user perspective:

1. Events are consumed one-by-one in Observers, which exist outside of a schedule. Messages are consumed by iterating over many of them in normal systems, which exist in one or more places inside a schedule.
2. Event handlers are run for developers. Message consumers are responsible for dispatching handler logic themselves.
3. Events are handled immediately. Messages are handled at some later moment in time (or not at all).
4. Events need additional configuration to make them work (Event::Trigger). Messages do not.
5. Sending/Handling Events incurs a degree of overhead. Sending/Handling Messages is about as fast as it can be.

Perhaps most importantly: this is about predictability / UX. Lets say someone is consuming a ProcessingFinished event from some 3rd party library. If events can either be "buffered" or "observed", the consumer has no way to know how to consume the ProcessingFinished event. Unless the author was aware of this issue, decided to be careful / documented this, and the consumer knew to read said docs, then the consumer just needs guess. Is their Observer not running because ProcessingFinished never happened, or because the event was never "triggered" and instead just written to the buffered queue?

That is not a good situation to be in. The hard conceptual line here makes building and consuming APIs much clearer. Developers can unambiguously define "things that happen" in their APIs and developers can know exactly how to consume those things.

Maybe the argument just really is that in our game engine specifically, observers have a kind of special status

Yes, but I wouldn't frame it that way. Observers (and the Events they run on) are functionally a completely different system with a different API surface, different behaviors, different timing, different performance, etc.

[tychedelia]

I didn't mean implementation detail in the casually dismissive sense. Agree that differences are extremely important for consumers. But they can and often do carry the same kind of information, i.e. their differences are in their implementation and not necessarily in their content.

I think this is most obvious with something like RemovedComponents. On the one hand, the Remove observer is much easier to work with and having the equivalent (#2148) change detection filter would be quite nice.

But it also makes sense to me that, just like a buffered message queue of removed component ids, we probably could/should also have an AddedComponents message, etc. Especially in a world where components are more easily introspectible, it totally would make sense to me that you may want to performantly consume a queue of all change events to put into some sink (telemetry, etc). In which case, Remove should also probably be Message.

I agree that when looking at the docs, consumers should easily be able to tell how they are supposed to consume an event. I also like the naming of Message a lot better than two traits both sharing the event nomenclature. But if many things end up being both Event/Message I think there's still potential source for confusion. Because they both are events in the vernacular sense.

So ig I'm not really objecting to anything in particular, just noting that I think the venn diagram potentially has a lot more significant overlap that undermines these being cleanly severable concepts.

[cart]

But it also makes sense to me that, just like a buffered message queue of removed component ids, we probably could/should also have an AddedComponents message

Perhaps, although we'd have to weigh this functionality against the performance cost. I think theres a strong argument to remove RemovedComponents.

we probably could/should also have an AddedComponents message

I'm thinking we might want a plugin that adds an observer for an arbitrary event type and pipes it into a MessageWriter. Then we don't need to pay this cost by default.

It might even be worth removing the Message trait entirely to allow anything to be sent as a message.

[viridia]

The core widgets demo uses RemovedComponents (as does feathers), and it's pretty ugly - I would love a better alternative.

One potential approach would be some kind of unified message stream for all mutations (add, remove, replace, etc.). Because this would be expensive, it would have to be strictly opt-in, perhaps via a marker component or query filter. This sounds very much like some of the things Sander has done.

This starts to get into the whole discussion around reactivity, which is likely off-topic for this PR. Ideally, you'd want some way to handle all changes to components, assets, and resources in a uniform way, so that you could then have arbitrary reactive functions that are able to depend upon heterogenous inputs.

[tychedelia]

It might even be worth removing the Message trait entirely to allow anything to be sent as a message.

This is also where my head goes because it allows observers to own the event concept without having a separate also-kinda-event trait. This is also what I meant by messages not being "special" in the engine like observers are. You could roll your own with no trait bound and whatever storage you want in like a dozen lines of code unlike observers.

I'm thinking we might want a plugin that adds an observer for an arbitrary event type and pipes it into a MessageWriter. Then we don't need to pay this cost by default.

I'm definitely not a fan of RemovedComponents and it feels like an awkward gap in the API (although the semantics of a Removed filter are somewhat unclear to me).

But I will note this is kinda how change data capture evolved in SQL. First it was built via database triggers and then most engines moved to reading directly the WAL or some equivalent for performance reasons.

I'd much prefer API consistency above hypothetical performance concerns, though. Until observers are shown to have a serious deficiency for this use case.

Tbh I think you've pretty convinced me of the above. The ability to (optionally) express message producers as observers sounds powerful and flexible.