Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make Trigger::entity() panic on Entity::PLACEHOLDER and add Trigger::get_entity() #14236

Open
MiniaczQ opened this issue Jul 9, 2024 · 1 comment · May be fixed by #14558
Open

Make Trigger::entity() panic on Entity::PLACEHOLDER and add Trigger::get_entity() #14236

MiniaczQ opened this issue Jul 9, 2024 · 1 comment · May be fixed by #14558
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! X-Contentious There are nontrivial implications that should be thought through

Comments

@MiniaczQ
Copy link
Contributor

MiniaczQ commented Jul 9, 2024

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

Trying to perform commands on trigger entity can lead to errors later than expected (command execution instead of call site).

What solution would you like?

While Entity::PLACEHOLDER works great for the internals, the users should be presented with the standard two methods:

  • entity() - get a valid entity or panic
  • get_entity() - get a valid entity or none

This requires a breaking change on entity() which currently never panics, but can return Entity::PLACEHOLDER.

What alternative(s) have you considered?

Check for placeholder by oneself.
@MiniaczQ MiniaczQ added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Jul 9, 2024
@janhohenheim
Copy link
Member

In the vein of #14268, maybe there should only be a get_entity() and no entity()

@benfrankel benfrankel mentioned this issue Jul 10, 2024
Open
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Contentious There are nontrivial implications that should be thought through D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! and removed C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Jul 14, 2024
@SarthakSingh31 SarthakSingh31 linked a pull request Jul 31, 2024 that will close this issue
Open
@alice-i-cecile alice-i-cecile mentioned this issue Oct 20, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Trigger::entity() only returns valid entities SarthakSingh31/bevy
3 participants
@alice-i-cecile @janhohenheim @MiniaczQ