Observer EventPatterns

[ItsDoot]

Objective

On<E, B>'s B type parameter is the single most commonly confusing construct in bevy_ecs. The predominate use case is for filtering lifecycle event observers:

world.add_observer(|on: On<Add, A>| {
    // gets fired whenever component `A` is added to an entity
});

For most other use cases, like picking or custom user events, this type parameter is wholly unnecessary.

Therefore, it should be hidden away for the cases where it is not needed.

Solution

Introduce a thin layer above Event called EventPattern, and update On's bounds:

pub trait EventPattern: 'static {
    type Event: Event;
    type Components: Bundle;
}

pub struct On<'w, 't, E: EventPattern> {
    observer: Entity,
    event: &'w mut E::Event,
    trigger: &'w mut EventPatternTrigger<'t, E>,
    trigger_context: &'w TriggerContext,
}

This allows us to lift the B type parameter in On<E, B> into the E type parameter. To ensure other events like picking continue to work, we introduce a blanket implementation for all Events:

// All events are implicitly EventPatterns, with no additional components.
impl<E: Event> EventPattern for E {
    type Event = Self;
    type Components = ();
}

To facilitate lifecycle events, we first rename them as follows:

• Add -> AddEvent
• Insert -> InsertEvent
• Discard -> DiscardEvent
• Remove -> RemoveEvent
• Despawn -> DespawnEvent

Then, we introduce new generic types in place of those old identifiers that are EventPatterns:

pub struct Add<B: Bundle>(PhantomData<B>);

impl<B: Bundle> EventPattern for Add<B> {
    type Event = AddEvent;
    type Components = B;
}

pub struct Insert<B: Bundle>(PhantomData<B>);

impl<B: Bundle> EventPattern for Insert<B> {
    type Event = InsertEvent;
    type Components = B;
}

pub struct Discard<B: Bundle>(PhantomData<B>);

impl<B: Bundle> EventPattern for Discard<B> {
    type Event = DiscardEvent;
    type Components = B;
}

pub struct Remove<B: Bundle>(PhantomData<B>);

impl<B: Bundle> EventPattern for Remove<B> {
    type Event = RemoveEvent;
    type Components = B;
}

pub struct Despawn<B: Bundle>(PhantomData<B>);

impl<B: Bundle> EventPattern for Despawn<B> {
    type Event = DespawnEvent;
    type Components = B;
}

This results in a slightly modified observer setup:

world.add_observer(|on: On<Add<A>>| {
    // gets fired whenever component `A` is added to an entity
});

Testing

No new tests have been added, we have decent coverage of observer tests as is.

[james-j-obrien]

Cool approach! Looks good.

[Diddykonga]

LGTM

Abstracts the E: Event and B: Bundle from On<E, B> into On<E> where E: EventMatcher
This allows the syntax to be like Add<C>, while the actual semantics use the Event and Components associated types on the EventMatcher. Similiar-ish to an custom WorldQuery.

[chescock]

Nice, this is an elegant way to solve this! I like how a bunch of B generics disappeared from places that don't care about them.

[alice-i-cecile]

I'm curious about the answers to @chescock's questions :)

FYI, I'm planning to hold off merging this until after we cut the 0.19-rc, so we can land this together with a F: QueryFilter generic in 0.20.

[mockersf]

@ItsDoot this has some conflicts, could you resolve them?

[mockersf]

needs a few fixes for CI

[alice-i-cecile]

From my merge train notes:

Oh ho-ho: a beefy one! Getting rid of the bundle generic in observers is something I've wanted to do for years now.
Moving it to the lifecycle events makes perfect sense, and it's great to see that we could actually get something that compiles here.

Let's pull this down for a more complete review: this is literally my job as an SME-ECS.
Okay, I see: we're extending the class of things that can be used in observers to EventPattern, as a superset of Events.
Makes sense.
EventPattern is not my favorite name, but I can live with it since I don't have a better suggestion.

Solid docs for some genuinely complex machinery. The cross-links are incredibly important for putting together an understanding of what's going on.

Migration guide is good, and has the added benefit of verifying that the dynamic component case was not lost.

This has my approval.
@cart self-requested a review a few days ago though.
I'm going to ping him before merging this; there's no immediate rush, as badly as I want this.

[alice-i-cecile]

I do. I consider myself a primary keeper of the "observer context" in terms of the current implementation, its limitations, and the desired future path. Historically, changes to observers have been fraught and I'd like to keep things on the rails. Happy to give this some time after 0.19 is ready

• @cart on Discord