Refactor EntityEvent to support ContainsEntity, unlocking the use of kinded entities with observers

[Zeenobit]

This is related to #21384

Objective

The goal of this change is to enable EntityEvent to work with any Entity-like type which implements ContainsEntity.

Solution

• Change return type of EntityEvent::event_target from Entity to &impl ContainsEntity
• Change all appropriate call sites to use event_target().entity() instead of event_target()
• Refactor EntityEvent::event_target_mut into EntityEvent::set_event_target

I'm not fully happy with this solution yet, but I'm opening the PR to discuss the options from here.

Mainly the issue revolves around set_event_target. To make this work, we need the underlying type to be constructible from an entity. This may not always be safe from user's perspective (it wouldn't be with Instance<T> for example).

Based on my understanding of the system, an event's target is only mutated in the case of event propagation, which makes sense.

To work around this, I propose that we flag EntityEvents as immutable or not; or "not propagatable" or not (I'm open to other terminology! :P).

This would allow us to implement set_event_target as unreachable()! and throw an error instead if the user tries to propagate such an event. We could even maybe enforce this compile time but it'll require additional complexity (mostly in form of different permutations of trigger_* method family).

Testing

• Added test_derive_entity_event to cover all permutations

[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.

[alice-i-cecile]

&impl ContainsEntity

I'm reluctant to do this as it will break object safety. That will be a pretty widespread problem when trying to fix this though.

[Zeenobit]

I'm reluctant to do this as it will break object safety. That will be a pretty widespread problem when trying to fix this though.

It won't be needed with @cart 's suggestion. I'll revise the CL.

How do we feel about immutable entity events though?

[alice-i-cecile]

How do we feel about immutable entity events though?

Generally open to it, but haven't thought about this hard enough to be convinced that they're the only solution :)

[Zeenobit]

I've updated the PR with the simplified implementation. Great idea! :)

Generally open to it, but haven't thought about this hard enough to be convinced that they're the only solution :)

I'm open to suggestions!

From my usage of the engine so far, I have yet to find a use case for propagating events, which as I understand, is the only reason entity events are mutable.

So from my perspective, I could easily envision all entity events being immutable by default, and only mutable if #[entity_event(propagate)] is specified. Attempting to propagate an event not flagged with #[entity_event(propagate)] seems like a degenerate case to me, and so we could easily treat it as an error.

If we do that, we could then implement set_event_target with unreachable!() for immutable events, since it would never be called. Additionally, I would go as far as marking set_event_target as unsafe. In my mind, this function should never be called externally.

[cart]

On the topic of immutable entity events, I'm on board for that, but I think we'd do that at the trait level and break it out as SetEntityEventTarget (where specifying propagate in the EntityEvent derive would provide that impl).

From my usage of the engine so far, I have yet to find a use case for propagating events, which as I understand, is the only reason entity events are mutable.

Making them mutable would also theoretically enable a bulk API (ex: commands.trigger_targets(MyEvent { entity: Entity::PLACEHOLDER }, [e1, e2, e3])). But that feels a bit dirty to me / I'm pretty fine with losing it.

[Zeenobit]

I've added SetEntityEventTarget as discussed. Much cleaner implementation than what I had in mind! :)

For me, the only remaining issue is the EntityCommand::trigger call.

Should I split that into a separate PR or should I continue working in this PR?

Any opinions on how to refactor trigger?

In my opinion, the current implementation goes against Rust nomenclature. Typically in Rust we have functions like verb which take some data as input, or verb_with which take a constructor Fn.
We only have trigger(f: impl FnOnce(Entity) -> E).

To fix this properly, I think trigger should just be trigger(e: E) and we add trigger_with(f: impl FnOnce(Entity) -> E); but that would be a breaking change (which this PR already is? Maybe? 😅). I'm also open to other ideas on the naming.

[Zeenobit]

I looked into refactoring trigger function, but then I realized the current implementation already does support what I initially asked for. It just requires a different call, because this actually doesn't make a lot of sense:

world.entity_mut(entity).trigger(MyEntityEvent { entity });

This is because we have to guarantee the entity event matches the referenced entity.

Instead, we need to use:

world.trigger(MyEntityEvent { entity });

And this seems to work just fine.

So I've updated the tests to use that instead, which makes the PR ready for review!
AFAIK this doesn't have any breaking changes anymore either. Does it still need a migration guide?

(PS: Sorry about lack of commit message in the last commit, I forgot to set it 😅 )

[alice-i-cecile]

Very nice: that's a very minimal change set. Can you please add a migration guide? We've lost a method on EntityEvent.

[Victoronz]

While I agree with the general direction, I see a flaw here:
IIUC, this PR allows deriving EntityEvent on types that are ContainsEntity. However, to support mutable use, and with it propagation, it also requires an into::<Entity>() call.
Many, (currently most) "kinded entities" do not want to be freely constructible from Entity!
A large boon of newtypes is that they can carry some meaning that the inner type doesn't. By requiring a public boundary, we lose that!

ContainsEntity/EntityEquivalent types should themselves be what the user mutates and passes around. To do that compatibly with EntityEvent without locking us out of mutation, we'd need to have the trait methods take entity wrappers themselves.
And doing that begs the infectious question of "if we take entity wrappers here, why not over there?"
I am all for that, but I think it should have a dedicated discussion first!

Lastly, considering the prior implications, ContainsEntity as a generic should not be implicitly converted to Entity, unless that type implements EntityEquivalent. This way we keep the more semantically distant ContainsEntity -> Entity conversions on the user side, which protects the internal entity semantics and guards against generic compile time hits.

[cart]

And doing that begs the infectious question of "if we take entity wrappers here, why not over there?"

Yeah I find this bit concerning. I don't really want to be in the business of "ContainsEntity everywhere" yet, both for "api complexity" reasons and compile time reasons (doing that everywhere would prevent us from pre-compiling a large number of apis).

From my perspective the "public facing entity contract" is Entity right now.

[Zeenobit]

IIUC, this PR allows deriving EntityEvent on types that are ContainsEntity. However, to support mutable use, and with it propagation, it also requires an into::() call.
Many, (currently most) "kinded entities" do not want to be freely constructible from Entity!

That's why we pulled out the mutating functions out of EntityEvent. I don't see a way to support mutable use with entitoids (I really don't want us locked in the idea that the only entity newtypes are kinded entities) unless the type specifically allows mutation or construction from an Entity.

And doing that begs the infectious question of "if we take entity wrappers here, why not over there?"
I am all for that, but I think it should have a dedicated discussion first!

If it were up to me, a lot of Bevy API would use impl ContainsEntity rather than Entity. :P But I agree that's a separate discussion. I can open it if there is interest.