Query::get_multiple and get_multiple_mut #3333

alice-i-cecile · 2021-12-15T04:33:22Z

Objective

The inability to have multiple active mutable borrows into a query is a common source of borrow-checker pain for users.
This is a pointless restriction if and only if we can guarantee that the entities they are accessing are unique.
This could already by bypassed with get_unchecked, but that is an extremely unsafe API.
Closes Safe method for multiple mutable references to components from Query #2042.

Solution

Implement Query::get_multiple and get_multiple_mut, to provide access to the data of more than one entity at once.
get_multiple_mut enforces uniqueness (to ensure soundness), but get_multiple does not (to reduce overhead).
No equivalent methods were made on World, as multiple EntityMut cannot exist at once: the ability to add and remove components to entities with it make that design fundamentally unsound.
Even when working in an exclusive world context, users can still create a Query and then use these methods.
An iterator is returned, rather than a Vec or other collection in order to avoid allocations and ensure maximal flexibility.

Breaking

Technically, this is a breaking change, as I've added a AliasedMutability variant to the QueryEntityError enum.

crates/bevy_ecs/src/system/query.rs

nakedible

Community review! Looks good to me, but strained myself to get at least something to comment 😉

crates/bevy_ecs/src/system/query.rs

nakedible · 2022-01-07T06:16:25Z

Also, probably just stating the obvious here, but adding an error to QueryEntityError would not be a breaking change if it would be marked as #[non_exhaustive].

alice-i-cecile · 2022-01-07T07:37:27Z

We have accepted that every release will be extremely breaking, at least for now ;) I'm reasonably happy with causing compiler errors to draw attention to new behavior in this case.

yilinwei · 2022-01-12T10:42:24Z

For the lack of variadics, what about using and returning const generics? So [Entity; N] -> [&mut T; N]?

Getting multiple EntityMut is fundamentally impossible due to the ability to add and remove components. Fixing this is out of scope for this PR.

Co-authored-by: MrGVSV <49806985+MrGVSV@users.noreply.github.com>

cart · 2022-02-23T23:36:13Z

crates/bevy_ecs/src/system/query.rs

+ /// assert_eq!(*a_iterator.next().unwrap(), A(1));
+ /// ```
+ #[inline]
+ pub fn get_multiple<const N: usize>(


I think these should be implemented on QueryState with appropriate variants, then wired up to Query

Yep... I don't love this whole API duplication, but I think that's a reasonable call.

I think it is necessary if we want "self driving" queries with direct world access (and I think we do).

BoxyUwU

You should add a compile fail test making sure that you cant make a GetMultipleMut iterator and then call methods on Query and then use the results from GetMultipleMut to get an aliased mutable ref

crates/bevy_ecs/src/system/query.rs

BoxyUwU · 2022-03-21T16:26:13Z

I don't really think that we should be adding Query::from_state in this PR, its an entirely unrelated API addition. It should be possible to get a Query from World already:

let mut world = World::default();
let mut system_state = SystemState::<Query<&mut Foo>>::new(&mut world); 
let mut query = system_state.get_mut(&mut world);

Co-authored-by: Boxy <supbscripter@gmail.com>

BoxyUwU · 2022-03-21T17:16:12Z

Lol those errors are not super helpful... can be fixed by replacing impl Iterator with GetMultipleMut in fn get_multiple_mut and by doing Query::<'q, 's, Q, F>::get_unchecked in GetMultipleMut::next. changing Query::get from fn get(&'s self to fn get(&self seems to fix the error for fn get_multiple

Full credit to Boxy

i did not realise querystate was so unbelievably MESSED Up - Boxy

alice-i-cecile · 2022-03-22T22:53:43Z

Closing in favor of the less messy #4298.

@BoxyUwU

…4298) # Objective - The inability to have multiple active mutable borrows into a query is a common source of borrow-checker pain for users. - This is a pointless restriction if and only if we can guarantee that the entities they are accessing are unique. - This could already by bypassed with get_unchecked, but that is an extremely unsafe API. - Closes #2042. ## Solution - Add `get_multiple`, `get_multiple_mut` and their unchecked equivalents (`multiple` and `multiple_mut`) to `Query` and `QueryState`. - Improve the `QueryEntityError` type to provide more useful error information. ## Changelog - Added `get_multiple`, `get_multiple_mut` and their unchecked equivalents (`multiple` and `multiple_mut`) to Query and QueryState. ## Migration Guide - The `QueryEntityError` enum now has a `AliasedMutability variant, and returns the offending entity. ## Context This is a fresh attempt at #3333; rebasing was behaving very badly and it was important to rebase on top of the recent query soundness fixes. Many thanks to all the reviewers in that thread, especially @BoxyUwU for the help with lifetimes. ## To-do - [x] Add compile fail tests - [x] Successfully deduplicate code - [x] Decide what to do about failing doc tests - [x] Get some reviews for lifetime soundness

@BoxyUwU

…evyengine#4298) # Objective - The inability to have multiple active mutable borrows into a query is a common source of borrow-checker pain for users. - This is a pointless restriction if and only if we can guarantee that the entities they are accessing are unique. - This could already by bypassed with get_unchecked, but that is an extremely unsafe API. - Closes bevyengine#2042. ## Solution - Add `get_multiple`, `get_multiple_mut` and their unchecked equivalents (`multiple` and `multiple_mut`) to `Query` and `QueryState`. - Improve the `QueryEntityError` type to provide more useful error information. ## Changelog - Added `get_multiple`, `get_multiple_mut` and their unchecked equivalents (`multiple` and `multiple_mut`) to Query and QueryState. ## Migration Guide - The `QueryEntityError` enum now has a `AliasedMutability variant, and returns the offending entity. ## Context This is a fresh attempt at bevyengine#3333; rebasing was behaving very badly and it was important to rebase on top of the recent query soundness fixes. Many thanks to all the reviewers in that thread, especially @BoxyUwU for the help with lifetimes. ## To-do - [x] Add compile fail tests - [x] Successfully deduplicate code - [x] Decide what to do about failing doc tests - [x] Get some reviews for lifetime soundness

@BoxyUwU

…evyengine#4298) # Objective - The inability to have multiple active mutable borrows into a query is a common source of borrow-checker pain for users. - This is a pointless restriction if and only if we can guarantee that the entities they are accessing are unique. - This could already by bypassed with get_unchecked, but that is an extremely unsafe API. - Closes bevyengine#2042. ## Solution - Add `get_multiple`, `get_multiple_mut` and their unchecked equivalents (`multiple` and `multiple_mut`) to `Query` and `QueryState`. - Improve the `QueryEntityError` type to provide more useful error information. ## Changelog - Added `get_multiple`, `get_multiple_mut` and their unchecked equivalents (`multiple` and `multiple_mut`) to Query and QueryState. ## Migration Guide - The `QueryEntityError` enum now has a `AliasedMutability variant, and returns the offending entity. ## Context This is a fresh attempt at bevyengine#3333; rebasing was behaving very badly and it was important to rebase on top of the recent query soundness fixes. Many thanks to all the reviewers in that thread, especially @BoxyUwU for the help with lifetimes. ## To-do - [x] Add compile fail tests - [x] Successfully deduplicate code - [x] Decide what to do about failing doc tests - [x] Get some reviews for lifetime soundness

alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Dec 15, 2021

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Dec 15, 2021

alice-i-cecile added S-Needs-Review and removed S-Needs-Triage This issue needs to be labelled labels Dec 15, 2021

MrGVSV reviewed Dec 15, 2021

View reviewed changes

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 force-pushed the get_multiple branch 3 times, most recently from fdccab5 to 1b6e83d Compare December 15, 2021 20:20

alice-i-cecile mentioned this pull request Dec 15, 2021

Provide size hint information about the contents of Queries #3342

Closed

alice-i-cecile added the S-Needs-Migration-Guide label Dec 15, 2021

cart removed the S-Needs-Review label Dec 16, 2021

alice-i-cecile force-pushed the get_multiple branch from c65edc6 to 8b448cb Compare December 16, 2021 02:56

Mathspy reviewed Dec 16, 2021

View reviewed changes

crates/bevy_ecs/src/system/query.rs Outdated Show resolved Hide resolved

nakedible suggested changes Jan 6, 2022

View reviewed changes

crates/bevy_ecs/src/system/query.rs Outdated Show resolved Hide resolved

crates/bevy_ecs/src/system/query.rs Show resolved Hide resolved

alice-i-cecile added M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide and removed S-Needs-Migration-Guide labels Jan 20, 2022

nakedible approved these changes Feb 13, 2022

View reviewed changes

alice-i-cecile and others added 9 commits February 14, 2022 15:37

Method skeletons

3e8288a

Preliminary implementation

6e7600b

Removed World API

6b81770

Getting multiple EntityMut is fundamentally impossible due to the ability to add and remove components. Fixing this is out of scope for this PR.

Added doc tests

ad9200c

Improved API design

3707686

Improved docs

2a10fc2

Remove PR noise

36df4e5

Typo fix

b7a1cbe

Co-authored-by: MrGVSV <49806985+MrGVSV@users.noreply.github.com>

Typo fix

499f804

Co-authored-by: MrGVSV <49806985+MrGVSV@users.noreply.github.com>

cart reviewed Feb 23, 2022

View reviewed changes

alice-i-cecile removed 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 Feb 24, 2022

BoxyUwU suggested changes Mar 21, 2022

View reviewed changes

crates/bevy_ecs/src/system/query.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

crates/bevy_ecs/src/system/query.rs Outdated Show resolved Hide resolved

alice-i-cecile and others added 3 commits March 21, 2022 12:45

Fix lifetimes per Boxy

ab74fd3

Co-authored-by: Boxy <supbscripter@gmail.com>

Fix lifetimes per Boxy

043abb1

Co-authored-by: Boxy <supbscripter@gmail.com>

Iterator should require a mutable reference

7cb4330

Co-authored-by: Boxy <supbscripter@gmail.com>

Mostly fix liftime errors

b264ffc

Full credit to Boxy

cart added this to the Bevy 0.7 milestone Mar 21, 2022

alice-i-cecile added 7 commits March 21, 2022 19:00

Fix remaining lifetime error

be7ec0d

Change return types

1670877

Record which entity caused QueryEntityError errors

10dd581

Remove Query::from_state method

783b4f4

Doc improvements

a0d6632

Move methods onto QueryState

9c74b6d

Add infallible variants

aff9c65

alice-i-cecile marked this pull request as draft March 22, 2022 00:42

BoxyUwU and others added 4 commits March 22, 2022 01:58

i did not realise querystate was so unbelievably MESSED Up

de5bda7

Progress!

dfc8258

i did not realise querystate was so unbelievably MESSED Up - Boxy

Return a result of an array, rather than an array of results

52d57c1

More comments, more safety

bee959e

This was referenced Mar 22, 2022

Use type aliases for Query item types in Query and QueryState methods #4297

Closed

[Merged by Bors] - Add get_multiple and get_multiple_mut APIs for Query and QueryState #4298

Closed

alice-i-cecile removed this from the Bevy 0.7 milestone Mar 22, 2022

alice-i-cecile closed this Mar 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Query::get_multiple and get_multiple_mut #3333

Query::get_multiple and get_multiple_mut #3333

alice-i-cecile commented Dec 15, 2021 •

edited

Loading

nakedible left a comment

nakedible commented Jan 7, 2022

alice-i-cecile commented Jan 7, 2022

yilinwei commented Jan 12, 2022

cart Feb 23, 2022

alice-i-cecile Feb 23, 2022

cart Feb 24, 2022

BoxyUwU left a comment

BoxyUwU commented Mar 21, 2022

BoxyUwU commented Mar 21, 2022 •

edited

Loading

alice-i-cecile commented Mar 22, 2022

Query::get_multiple and get_multiple_mut #3333

Query::get_multiple and get_multiple_mut #3333

Conversation

alice-i-cecile commented Dec 15, 2021 • edited Loading

Objective

Solution

Breaking

nakedible left a comment

Choose a reason for hiding this comment

nakedible commented Jan 7, 2022

alice-i-cecile commented Jan 7, 2022

yilinwei commented Jan 12, 2022

cart Feb 23, 2022

Choose a reason for hiding this comment

alice-i-cecile Feb 23, 2022

Choose a reason for hiding this comment

cart Feb 24, 2022

Choose a reason for hiding this comment

BoxyUwU left a comment

Choose a reason for hiding this comment

BoxyUwU commented Mar 21, 2022

BoxyUwU commented Mar 21, 2022 • edited Loading

alice-i-cecile commented Mar 22, 2022

alice-i-cecile commented Dec 15, 2021 •

edited

Loading

BoxyUwU commented Mar 21, 2022 •

edited

Loading