feat: allow for Trigger to provide the Entity on which an event w…

[emfax]

…as observed.

Trigger in an observer system has provided the target method which gives the entity on which an event was triggered. This may be different to the entity that is being observed. This change introduces a current_target method on Trigger, which returns the id of the entity being observed.

In some cases, code that uses the result of Trigger::target may now actually need Trigger::target.

Objective

• Describe the objective or issue this PR addresses.
• If you're fixing a specific issue, say "Fixes #X".

Solution

• Describe the solution used to achieve the objective above.

Testing

• Did you test these changes? If so, how?
• Are there any parts that need more testing?
• How can other people (reviewers) test your changes? Is there anything specific they need to know?
• If relevant, what platforms did you test these changes on, and are there any important ones you can't test?

---

Showcase

This section is optional. If this PR does not include a visual change or does not add a new feature, you can delete this section.

• Help others understand the result of this PR by showcasing your awesome work!
• If this PR adds a new feature or public API, consider adding a brief pseudo-code snippet of it in action
• If this PR includes a visual change, consider adding a screenshot, GIF, or video
  • If you want, you could even include a before/after comparison!
• If the Migration Guide adequately covers the changes, you can delete this section

While a showcase should aim to be brief and digestible, you can use a toggleable section to save space on longer showcases:

Click to view showcase

println!("My super cool code.");

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

[hukasu]

How is this different fromTrigger::observer?

[emfax]

Trigger::observer returns the Entity of the observer. Even if you call observe on an EntityCommands, the observer is spawned as a separate entity. So Trigger::observer returns the Entity of the observer entity.

Trigger::current_target returns the Entity of the entity being observed. For example, calling EntityCommands::observe will create an observer that watches that particular entity (even though the observer itself is spawned as a separate entity). When the observer is triggered, current_target returns the Entity of the entity that is being watched.

Trigger::current_target is also different to Trigger::target since events can bubble up the hierarchy. The target might be an entity deeper in the hierarchy.

[hukasu]

so when bubbling you would have access to the current entity, and the entity that started the bubbling?

[emfax]

Yes. That’s right

[SteveAlexander]

I've been using Relationships, event bubbling, and triggers / observers. I found the current Trigger::target and Trigger::observer to be confusing -- is the target the entity that the event was sent with, or the entity that is being observed? Turns out sometimes I want to know both the originally targeted entity and the related entity with the observer that the event was bubbled to.

So, this change is helpful to me.

May I suggest some different naming? I find "current_target" and "target" a bit confusing.

My suggestion is as follows:

Trigger::target = the entity that was triggered
Trigger::observer = the observer system that is currently processing the event
Trigger::observed = the entity that Trigger::observer is observing

With no event bubbling, Trigger::target == Trigger::observed.
With event bubbling, for a particular event, Trigger::target stays the same, and Trigger::observed changes for each observer the event gets bubbled to.

[chescock]

If I'm reading this right, then the value returned by target() today will be returned by current_target() after this change, and the target() method will change from returning the current target to the original target. Is that right?

If that's the case, then I think you need a migration guide, since it's a change to the behavior of the existing target() method. (And you may also need to update some target != Entity::PLACEHOLDER checks to use current_target.)

[chescock]

Given how confusing this has been, it might help to explicitly mention event propagation here. Maybe something along the lines of

    /// When [propagation](Trigger::propagate) is used,
    /// an observer may be invoked multiple times for a single event.
    /// [`Trigger::target`] will be the same for each invocation,
    /// while [`Trigger::current_target`] will be different.

[emfax]

Today, target returns the entity from which the event originated, so the result of target doesn’t actually change.

[chescock]

Today, target returns the entity from which the event originated, so the result of target doesn’t actually change.

... okay, I must be reading this wrong somehow. Let me try to walk through it slowly.

Before the change, fn trigger_observers_with_data() had mut target, which was different on every call to Observers::invoke because of the target = traverse_to; assignment. After the change, it has target, which is the same on every call, and mut current_target, which is different every time. Is that right?

So within fn trigger_observers_with_data(), current_target is equal to the old target and changes every time, while target is a new variable holding the initial target. Is that right?

That calls Observers::invoke, which constructs an ObserverTrigger. The variable names all still match, so current_target is equal to the old target and target is a new variable. The two get(&target) calls were changed to get(&current_target) to preserve the meaning. Is that right?

Where am I going wrong? It sure looks like fn current_target() returns the value that used to be returned by fn target(), and fn target() is a new value with the initial target.

[SteveAlexander]

Before the change, fn trigger_observers_with_data() had mut target, which was different on every call to Observers::invoke because of the target = traverse_to; assignment. After the change, it has target, which is the same on every call, and mut current_target, which is different every time. Is that right?

I think this is correct.

Said another way: currently, the value returned by fn target() is different as the event gets bubbled to different observers.

Where am I going wrong? It sure looks like fn current_target() returns the value that used to be returned by fn target(), and fn target() is a new value with the initial target.

Yes. My game uses fn target() to get the entity that's being observed, which may be different than the entity that was originally sent the event. This is due to event bubbling.

emfax wrote:

Today, target returns the entity from which the event originated, so the result of target doesn’t actually change.

I do not think this is correct. I am relying on "target" changing in my game code today. If it helps, I can post an example showing that it does in fact change, as event bubbling proceeds.

[emfax]

@SteveAlexander You're correct about what target() returns.

I was so sure that I was correct, I've been trying to figure this out for an hour. I was using the target field which derefs from the Pointer event I was testing with. Dammit.

So yes, the result of target() would change. Where should the migration guide go?

[SteveAlexander]

@emfax thanks. I can certainly recognise the situation of spending an hour chasing a mirage!

What do you think about the naming of target and current_target? Personally, I don't find it intuitive which one should be considered "current". Should it be the one that is receiving the event? Or the one that is the original target of the event?

I could make a case to myself that either would be the "current" one, and so I find it hard to remember which one I want. I think perhaps the confusion you spoke to in your comment above also suggests that the naming isn't as clear as it could be.

If the events were named differently, I think I'd find it easier to remember. For example, original_target and target -- "original" seems to me less ambiguous than "current". Or maybe the other names I suggested in my comment above.

[SteveAlexander]

I checked what javascript does, as that's an existing example of an event bubbling API. And it's consistent with the name and meanings for "target" and "current target" in this PR.

event.target – is the “target” element that initiated the event, it doesn’t change through the bubbling process.
event.currentTarget (=this) – the current element that handles the event (the one that has the handler on it)

(from https://javascript.info/bubbling-and-capturing)

[hukasu]

#[test]
fn current_target() {
    struct Bubbling;

    impl Event for Bubbling {
        const AUTO_PROPAGATE: bool = true;
        type Traversal = &'static ChildOf;
    }

    #[derive(Debug, Default, PartialEq, Eq, Resource)]
    struct BubblingStorage {
        observer: Vec<Entity>,
        target: Vec<Entity>,
    }

    fn bubble(trigger: Trigger<Bubbling>, mut storage: ResMut<BubblingStorage>) {
        storage.observer.push(trigger.observer());
        storage.target.push(trigger.target());
    }

    let mut world = World::new();

    world.init_resource::<BubblingStorage>();

    let root = world.spawn_empty().id();
    let a = world.spawn(ChildOf(root)).id();
    let b = world.spawn(ChildOf(a)).id();
    let c = world.spawn(ChildOf(b)).id();
    let d = world.spawn(ChildOf(c)).id();

    let observer = world
        .spawn(
            Observer::new(bubble)
                .with_entity(root)
                .with_entity(a)
                .with_entity(b)
                .with_entity(c)
                .with_entity(d),
        )
        .id();

    world.trigger_targets(Bubbling, d);

    let expected = BubblingStorage {
        observer: vec![observer, observer, observer, observer, observer],
        target: vec![d, c, b, a, root],
    };

    assert_eq!(&expected, world.get_resource::<BubblingStorage>().unwrap());
}

this passes with the current implementation
on your branch add a current_target to the BubblingStorage, add to the bubble observer system, and adjust the expected to what you are trying

[github-actions]

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes.

[chescock]

So yes, the result of target() would change. Where should the migration guide go?

I think if I add the "needs migration guide" label, then a bot will add a comment with instructions... yup, there is goes!

(If you wind up changing the names so that it no longer needs a migration guide, leave a comment and someone with triage access can remove the label.)

[emfax]

@SteveAlexander

I checked what javascript does, as that's an existing example of an event bubbling API. And it's consistent with the name and meanings for "target" and "current target" in this PR.

Yeah, I decided to use similar names to web here.

[alice-i-cecile]

@emfax, I quite like this work. Could you take a crack at reviewing #19609, and then rebase this on top of that?

I think that we should go with a current_target / original_target split inside of ObserverTrigger, but keep the nice entity name as the method for On to get the current_target.

[alice-i-cecile]

I've opened a PR that accomplishes the same thing plus a bit, so I'm going to close this out and finish up the work there.

[emfax]

@alice-i-cecile I’ve just seen this flurry of activity here.

Sorry I missed it. I would’ve got that stuff done.