Skip to content

Moved get_component(_unchecked_mut) from Query to QueryState#9686

Merged
alice-i-cecile merged 18 commits intobevyengine:mainfrom
bushrat011899:QueryStateComponentAccess
Sep 11, 2023
Merged

Moved get_component(_unchecked_mut) from Query to QueryState#9686
alice-i-cecile merged 18 commits intobevyengine:mainfrom
bushrat011899:QueryStateComponentAccess

Conversation

@bushrat011899
Copy link
Copy Markdown
Contributor

@bushrat011899 bushrat011899 commented Sep 4, 2023

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.

@ZacHarroldC5
Moved get_component(_unchecked_mut) from Query to QueryState
394c506 
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.
tguichaoua
tguichaoua suggested changes Sep 4, 2023
View reviewed changes
@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
Copy Markdown
Contributor

Shatur commented Sep 4, 2023

Does it makes sense to move component and component_mut too?

@bushrat011899
Copy link
Copy Markdown
Contributor Author

bushrat011899 commented Sep 4, 2023

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
Copy Markdown
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
Copy Markdown
Contributor Author

bushrat011899 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.

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.

@ZacHarroldC5
Formatting Changes
dd0a5ea
@ZacHarroldC5
Method Parity Between Query and QueryState
15f2a61 
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
Copy Markdown
Member

alice-i-cecile commented Sep 5, 2023

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 reviewed Sep 5, 2023
View reviewed changes
alice-i-cecile
alice-i-cecile reviewed Sep 5, 2023
View reviewed changes
@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
joseph-gio
joseph-gio reviewed Sep 6, 2023
View reviewed changes
tguichaoua
tguichaoua reviewed Sep 7, 2023
View reviewed changes
joseph-gio
joseph-gio reviewed Sep 7, 2023
View reviewed changes
joseph-gio
joseph-gio reviewed Sep 8, 2023
View reviewed changes
joseph-gio
joseph-gio suggested changes Sep 8, 2023
View reviewed changes
@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
mnmaita
mnmaita reviewed Sep 10, 2023
View reviewed changes
@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
@alice-i-cecile alice-i-cecile mentioned this pull request Sep 12, 2023
Merged
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
@bushrat011899 @ZacHarroldC5 @tguichaoua
Moved get_component(_unchecked_mut) from Query to QueryState (b…
2c4f992 
…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

Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@Shatur Shatur Awaiting requested review from Shatur

@Selene-Amanita Selene-Amanita Awaiting requested review from Selene-Amanita

+3 more reviewers

@joseph-gio joseph-gio joseph-gio approved these changes

@mnmaita mnmaita mnmaita left review comments

@tguichaoua tguichaoua tguichaoua approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add QueryState::(get_)component(_mut)

7 participants

@bushrat011899 @Shatur @alice-i-cecile @joseph-gio @tguichaoua @mnmaita @ZacHarroldC5