Open alice-i-cecile opened 1 week ago
This was raised by @MiniaczQ here, who now regrets the end decision.
Related to https://github.com/bevyengine/bevy/issues/14236.
We could alternatively canonicalize PLACEHOLDER
as a null value for Entity
.
If it's niche optimized, I'd rather use Option<Entity>
We could alternatively canonicalize
PLACEHOLDER
as a null value forEntity
.
I'd like to move away from this wherever possible.
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.
Sweet, I'm on board with that too.
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 useEntity::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 anOption<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 https://github.com/bevyengine/bevy/pull/10839