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

Implement QueryData for &Archetype and EntityLocation #12398

Merged
merged 6 commits into from
Mar 29, 2024

Conversation

james7132
Copy link
Member

@james7132 james7132 commented Mar 10, 2024

Objective

Fixes #12392, fixes #12393, and fixes #11387. Implement QueryData for Archetype and EntityLocation.

Solution

Add impls for both of the types.


Changelog

Added: &Archetype now implements QueryData
Added: EntityLocation now implements QueryData

@james7132 james7132 added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Mar 10, 2024
@james7132 james7132 requested a review from JoJoJet March 10, 2024 04:19
@alice-i-cecile
Copy link
Member

@urben1680 feel free to live a review here if you're comfortable.

@urben1680
Copy link
Contributor

urben1680 commented Mar 10, 2024

I would like the two types being mentioned in the Query::transmute_lens docs that these can be added and removed as wished, just as Entity.

Might switch it to dense to not break existing dense queries.

Would the implementation as it is right now here, besides the IS_DENSE value, not be what a dense WorldQuery look like? Why was false picked?

@james7132
Copy link
Member Author

james7132 commented Mar 10, 2024

Would the implementation as it is right now here, besides the IS_DENSE value, not be what a dense WorldQuery look like? Why was false picked?

set_archetype is never called on dense fetches, as it's table oriented instead. The Option in the fetch would always be None, and hence the debug_checked_unwrap would always panic or become undefined behavior.

If we were instead to take the entity, look up its location, then use its location to look up its archetype, it would work with both, but that would likely be slower in isolation than just returning back the assigned archetype.

I'm leaning towards making it dense, as making it force archetypal iteration will likely be slower in any compound query.

@james7132 james7132 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 Mar 23, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 25, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 25, 2024
@james7132 james7132 added this pull request to the merge queue Mar 29, 2024
Merged via the queue into bevyengine:main with commit 741803d Mar 29, 2024
27 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-Usability A targeted quality-of-life change that makes Bevy easier to use 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
4 participants