Skip to content

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.
@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-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Sep 6, 2023
@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
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-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