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

Moved get_component(_unchecked_mut) from Query to QueryState #9686

Merged

Conversation

bushrat011899
Copy link
Contributor

Objective

Solution

  • Moved get_component from Query to QueryState.
  • Moved get_component_unchecked_mut from Query to QueryState.
  • Moved QueryComponentError from bevy_ecs::system to bevy_ecs::query. Minor Breaking Change.
  • Narrowed scope of unsafe blocks in Query methods.

Migration Guide

  • use bevy_ecs::system::QueryComponentError; -> use bevy_ecs::query::QueryComponentError;

Notes

I am not very familiar with unsafe Rust nor its use within Bevy, so I may have committed a Rust faux pas during the migration.

Also had to move `QueryComponentError` to match the new location of its methods.

Also performed a QA pass to narrow the scope of `unsafe` blocks within `Query` methods.
crates/bevy_ecs/src/query/state.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/query.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/query.rs Outdated Show resolved Hide resolved
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Sep 4, 2023
@Shatur
Copy link
Contributor

Shatur commented Sep 4, 2023

Does it makes sense to move component and component_mut too?

@bushrat011899
Copy link
Contributor Author

Does it makes sense to move component and component_mut too?

It might do, I started with get_component and get_component_unchecked_mut because they were the last two methods that had sizeable implementations. There are several other functions which may want to be reworked as well:

  • par_iter: This doesn't require any additional information beyond get_component_unchecked_mut.
  • par_iter_mut: See par_iter.
  • many: Currently calls Query::get_many; should instead call QueryState::get_many_read_only_manual.
  • many_mut: See many.
  • get_component_mut: See many.
  • single: See many.
  • single_mut: See many.
  • contains: See par_iter.

Additionally, neither Query nor QueryState have the following methods, but probably should for completeness:

  • component
  • component_mut

If we also migrate the above to QueryState, then all of Query's methods simply forward to QueryState. Is that a desirable state for Query and QueryState to be in?

@Shatur
Copy link
Contributor

Shatur commented Sep 4, 2023

Additionally, neither Query nor QueryState have the following methods, but probably should for completeness:

Query have these methods since #9659. This is why I asking.

@bushrat011899
Copy link
Contributor Author

Additionally, neither Query nor QueryState have the following methods, but probably should for completeness:

Query have these methods since #9659. This is why I asking.

I missed that merge from my branch, apologies! Below would be the complete list of candidates to migrate over:

  • par_iter: This doesn't require any additional information beyond get_component_unchecked_mut.
  • par_iter_mut: See par_iter.
  • many: Currently calls Query::get_many; should instead call QueryState::get_many_read_only_manual.
  • many_mut: See many.
  • get_component_mut: See many.
  • single: See many.
  • single_mut: See many.
  • contains: See par_iter.
  • component: See many.
  • component_mut: See many.

I'm going to update my PR with the requested changes from @tguichaoua, then I'll migrate the above over just to demonstrate what that change would look like.

Added to `QueryState`:
* `component`
* `component_unchecked_mut`
* `many_read_only_manual`
* `many_unchecked_manual`
@Selene-Amanita Selene-Amanita self-requested a review September 5, 2023 01:11
@alice-i-cecile
Copy link
Member

If we also migrate the above to QueryState, then all of Query's methods simply forward to QueryState. Is that a desirable state for Query and QueryState to be in?

Yes; I much prefer that! Unless there's a very good (and commented) reason not to, these APIs should be mirrors of each other. Due to the fact that every Query stores a QueryState, method forwarding like this is the best way to achieve that.

@alice-i-cecile alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Sep 6, 2023
crates/bevy_ecs/src/system/query.rs Outdated Show resolved Hide resolved
@alice-i-cecile alice-i-cecile 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 Sep 10, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 11, 2023
Merged via the queue into bevyengine:main with commit 4c6b6fc Sep 11, 2023
22 checks passed
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
…evyengine#9686)

# Objective

- Fixes bevyengine#9683

## Solution

- Moved `get_component` from `Query` to `QueryState`.
- Moved `get_component_unchecked_mut` from `Query` to `QueryState`.
- Moved `QueryComponentError` from `bevy_ecs::system` to
`bevy_ecs::query`. Minor Breaking Change.
- Narrowed scope of `unsafe` blocks in `Query` methods.

---

## Migration Guide

- `use bevy_ecs::system::QueryComponentError;` -> `use
bevy_ecs::query::QueryComponentError;`

## Notes

I am not very familiar with unsafe Rust nor its use within Bevy, so I
may have committed a Rust faux pas during the migration.

---------

Co-authored-by: Zac Harrold <[email protected]>
Co-authored-by: Tristan Guichaoua <[email protected]>
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-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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.

Add QueryState::(get_)component(_mut)
7 participants