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.

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

Add non-existent entity behavior to Has doc #11025

Merged
merged 1 commit into from
Dec 19, 2023

Conversation

nicopap
Copy link
Contributor

@nicopap nicopap commented Dec 19, 2023

Objective

Has<T> in some niche cases may behave in an unexpected way.

Specifically, when using Query::get on a Has<T> with a despawned entity.

Solution

Add precision about cases wehre Query::get could return an Err.

@nicopap nicopap added C-Docs An addition or correction to our documentation A-ECS Entities, components, systems, and events labels Dec 19, 2023
@Nilirad
Copy link
Contributor

Nilirad commented Dec 19, 2023

I hope I don't sound too rude, but to me, at least, it is obvious that Query::get is fallible, because it returns a result. If I mindlessly unwrap it I should not expect to not encounter errors.

Also, I don't understand how this is specific to Has, as it seems to involve Query::get in general.

@nicopap
Copy link
Contributor Author

nicopap commented Dec 19, 2023

This PR is a follow up on this discord discussion: https://discord.com/channels/691052431525675048/730525730601041940/1185535439461941298.

So we are at least 2 (me and @NiseVoid) who would overlook it.

The reasoning was "if this matches every existing entities, it's safe to unwrap". Unlike say Query<&Transform> which only match entities with a Transform component, Query<Has<Transform>> matches every entities. But the reasoning is flawed for the reasons mentioned in the new doc paragraph.

@matiqo15 matiqo15 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Dec 19, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 19, 2023
Merged via the queue into bevyengine:main with commit d64e148 Dec 19, 2023
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Docs An addition or correction to our documentation S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants