Move `EntityEvent::event_target` to `EntityTargetTrigger`

[mrchantey]

The Event Rearchitecture PR creates a powerful Trigger abstraction pattern which allows the trigger to store what it needs to do its job, but event_target is currently stored on the Event payload.

To me this seems a usability regression from 0.16 and introduces maintenance overhead:

• Every EntityEvent must specify an entity field which introduces macro magic, creates work for the developer and disallows fieldless events.
• It requires knowing the target at construction, we lose helper methods like commands.spawn(Foo).trigger(Bar)
• The Trigger depending on and mutating Event internals seems at odds with other trigger patterns like PropagateEntityTrigger::original_event_target and AnimationEventTrigger::animation_player

Solution

The Event Rearchitecture PR mentions this EntityTargetTrigger pattern as an alternative approach. An 'entity event' becomes a regular event with a specific trigger, and the event_target is assigned on the trigger not the payload.

// we could still use `EntityEvent` as an alias for this
#[derive(Event)]
#[event(trigger = EntityTargetTrigger)]
struct MyEvent;

impl EntityWorldMut<'_> {
  fn trigger(&mut self, mut ev: E) -> &mut Self {
    // the target is assigned here, 0.16 style
    let mut trigger = EntityTargetTrigger::new(self.id());
    self.trigger_ref(&mut ev, &mut trigger,  MaybeLocation::caller());
    self
  }

Alternatives

• We could encourage users to store an Entity::PLACEHOLDER on the Event and then assign it in EntityCommands::trigger but we've already fought hard to remove patterns like this, see #16029.
• Currently this is not required to be upstreamed. I've implemented EventTargetTrigger for my own crates and just dont use EntityEvent, so we could point people to a third party crate.

If we were to go ahead with the refactor I'd be happy to open a PR and ensure its ready for 0.17, otherwise perhaps this issue could serve as a place to clarify the reasoning so people can understand the tradeoff.

[mrchantey]

Cart's clarification of the resoning from discord:

Yup I built the system with the "entities stored on trigger" use case in mind (such as your proposed EntityTargetTrigger), which would allow:

commands.trigger_with(event, [e1, e2])

This assumes Into<E::Trigger>, for the passed in Trigger, and From<[Entity; N]> for EntityTargetTrigger and likely no Default impl for EntityTargetTrigger, to prevent the footgun of commands.trigger(event) being called without entities.

I'm not convinced that we should support that pattern officially (even as an opt-in trigger type), as I think the "entity inside event" pattern is globally better, for the reasons stated in the PR. I think mixing and matching the two paradigms would cause confusion.

Alice 🌹 We can definitely add trigger on EntityCommands easily It naively has ergonomic implications, as it would require doing something like Entity::PLACEHOLDER on the event.

I think the cleaner solve would be something like:

commands.entity(e1).trigger(|entity| Explode { entity }) // alternatively if From<Entity> is implemented commands.entity(e1).trigger(Explode::from) // alternatively if it is Explode(Entity) commands.entity(e1).trigger(Explode)

Being able to process the other data on the payload at the same time is convenient. Being able to name the entity field whatever you want, and allowing the author of the event to document it with additional context has made so many things better. I think storing the entity on the event is considerably more legible, and easier to learn.

I made this decision with eyes wide open. I knew what I was sacrificing and I considered it to be "worth it". (note that Into<E::Trigger> is not currently present in that API) I'm on board for the trigger(&mut self, impl FnOnce(Entity) -> E) API above, rather than deprecating it entirely

....

I'd also like to point out that I suspect the builder-style entity construction pattern to fall out of favor once bsn! lands. Instead, I think moving these types of "trigger on spawn" events to entity lifecycle observers

// option 1: observers
bsn! {
  ILiveOnlyToExplode
  on(|add: On<Add, ILiveOnlyToExplode>, mut commands: Commands| {
    commands.trigger(Explode { entity: add.entity });
  })
}

// option 2: hooks
#[derive(Component)]
#[component(on_add = Self::on_add)]
struct ILiveOnlyToExplode;

impl ILiveOnlyToExplode {
  fn on_add(mut world: DeferredWorld, context: HookContext) {
    world.trigger(Explode { entity: context.entity })
  }
}

[mrchantey]

Reopening because despite confirmation that EntityEvent will not be changed, the rationale for EntityTargetEvent is still unresolved and would be closed by #21799