Observers should not use Entity::PLACEHOLDER

[alice-i-cecile]

What problem does this solve or what need does it fill?

As described in the docs for Trigger::entity, observers can be triggered by a specific entity, or by no specific entity at all.

Rather than returning and storing an Option here, we use Entity::PLACEHOLDER.

This is unidiomatic and unclear, and makes it very hard to robustly handle this case.
In Rust, Option or other enums is the correct way to handle this, not special-cased values.

This is especially true since Option<Entity> is niched and doesn't even take up any additional memory.

What solution would you like?

Replace all uses of Entity::PLACEHOLDER in observers code with an Option<Entity>.

What alternative(s) have you considered?

We could use a custom enum here instead, but I think that an option is sufficiently clear.

Additional context

This was added as part of #10839

[alice-i-cecile]

This was raised by @MiniaczQ here, who now regrets the end decision.

[alice-i-cecile]

Related to #14236.

[iiYese]

We could alternatively canonicalize PLACEHOLDER as a null value for Entity.

[MiniaczQ]

If it's niche optimized, I'd rather use Option<Entity>

[alice-i-cecile]

We could alternatively canonicalize PLACEHOLDER as a null value for Entity.

I'd like to move away from this wherever possible.

[cart]

I don't think Option<Entity> is the right approach here. Events (or whatever we choose to call them) should be designed with a specific target type in mind (no target, entity target, component target, etc).

Right after we landed observers I implemented "static event targets", which removes the need for Option / PLACEHOLDER , makes it statically impossible for users to try to access a non-existent entity for a "global event", and general prevents expressing the "wrong" thing.

https://github.com/cart/bevy/tree/static-event-targets

[alice-i-cecile]

Sweet, I'm on board with that too.