Trigger::entity() only returns valid entities

[SarthakSingh31]

Objective

Fixes #14236

Solution

Made Trigger::entity() panic on Entity::PLACEHOLDER and added a Trigger::get_entity()

---

Migration Guide

Anyone using Trigger::entity() should switch to Trigger::get_entity() unless they know that the event will always be triggered on an entity.

[DasLixou]

This has itched me when reading the release docs. But isn't Entity::PLACEHOLDER allowed to exist? The docs don't deny it.
What about a real Option<Entity> from the beginning?

[alice-i-cecile]

I'm nervous about all of our options here. Panicking is Very Bad, returning dummy entities is a silly footgun, and returning an option that always has to be unwrapped is frustrating.

In the spirit of #14275 / #12660, I'd prefer to store / return an Option here over adding a panic, but I'm interested in @janhohenheim / @benfrankel / @james7132's opinions too.

[janhohenheim]

Ping @MiniaczQ who I know has thoughts about this

[janhohenheim]

This message does not help a user, as they will mostly not interact with Entity::PLACEHOLDER and probably got this panic while trying to get the entity of a globally triggered observer. Add a hint to this like "Did you try [...]? There is no associated entity in that case."

This comment is also valid for the # Panics documentation, which should mention under which circumstances an underlying Entity would be a placeholder.

[janhohenheim]

Suggested change

	/// Returns the entity that triggered the observer, if it's not a placeholder entity.
	/// Returns the entity that triggered the observer, if it's not [`Entity::PLACEHOLDER`].

[janhohenheim]

Suggested change

	.get_entity()
	.map(|entity| query.get(entity).is_ok())
	.unwrap_or_default()
	.get_entity()
	.is_some_and(|entity| query.get(entity).is_ok())

[janhohenheim]

Suggested change

	info!("Attack hit {}", name);
	info!("Attack hit {name}");

[janhohenheim]

@alice-i-cecile my own preference in a vacuum would be to make entity() return an Option and force unwraps.

The general consensus there is that we should not break pre-established naming conventions, so respecting that, the recommendation becomes to only add get_entity and remove or deprecate entity
The general consensus then is that forcing Options is bad because we do not have good error handling built in. Good error handling is possible, but it currently requires the user to use anyhow, then create their own error logging function that works with anyhow::Error, and then pipe their function into it.
As such, respecting the general vibe from the discussion in #14275, my recommendation ultimately becomes allowing both APIs and changing get_entity to return a Result instead of an Option.

Note that the API as presented here is a bit ambiguous in terms of what triggered a None or panic. It could either be because a global trigger was triggered, or because a trigger was manually targeted at an entity that happens to be the placeholder entity for some reason. I think differentiating these cases is out of scope for this PR, but using a Result as a return type would allow us to be more precise in the future by extending the Error enum.

[benfrankel]

Agreed with @janhohenheim, in that entity() -> Result would be ideal, but entity() -> Entity vs. get_entity() -> Result is the consistent option for now. I have an extension trait in my own code that provides get_entity() and I unwrap it or warn + return with r!(trigger.get_entity()) from tiny_bail, and I'm happy with this personally.

[MiniaczQ]

No strong opinion here.
Definitely need a method that returns Option<Entity>, whether it's entity() or get_entity() depends solely on decision in #14275 / #12660

[DasLixou]

I wonder if we could always show get_entity returning an Option<Entity> but then having a generic thing on the observer which indicates that this should only run when an entity was targeted and then enable an entity() function.

[NthTensor]

My 2C here is that this is a silly problem that we have created for ourselves, and which we can solve by changing the problem.

Why can't we have a Trigger<T> and TriggerEntity<T>, and let the user statically indicate whether they need an entity or not. Then trigger() will only be dispatched to the former, and trigger_targets() will be dispatched to both. I suggest this because, in my experience so far, there do not seem to be many cases where the entity is really optional; either you want the entity, or you don't.

A slightly less controversial option might be to make Trigger::entity() return Option<Entity> and TriggerEntity::entity() return Entity. I think this is what @DasLixou was proposing, and I like it.

[benfrankel]

Relevant: #14272 and #14843.

This PR shouldn't be blocked on that issue, though.

[DasLixou]

Yes that's what I proposed, I also tried prototyping it but the logic behind observers were kinda weird, so many different hashmaps of hashmaps where I was unsure if I need entity or option there.. so that's for someone else :>

[alice-i-cecile]

@SarthakSingh31 are you planning on completing this PR?

[NthTensor]

Should be re-evaluated in the context of #15698. This could just be Trigger<Event, (), Entity>.

I will also say, the use-case for target-less observers seems small.

[MiniaczQ]

I will also say, the use-case for target-less observers seems small.

I use "targetless" observers for state transition events.
Targetless cost us nothing as it's just targeted at Entity::PLACEHOLDER, the point is just to have a slightly better API than comparing to this entity.