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

Safe method for multiple mutable references to components from Query #2042

Closed
engiwengi opened this issue Apr 29, 2021 · 2 comments
Closed
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!

Comments

@engiwengi
Copy link

What problem does this solve or what need does it fill?

It is sometimes useful/easier to have mutable access to the same component of many different entities at once.

What solution would you like?

Two new methods for Query, a default and one for a component. Example:

fn get_many_mut<'a, Q: WorldQuery, F: WorldQuery>(
    entities: impl IntoIterator<Item = Entity>,
    query: &'a mut Query<Q, F>,
) -> Result<Vec<<Q::Fetch as Fetch<'a>>::Item>, MultiEntityQueryErr>
where
    F::Fetch: FilterFetch,
{
    let mut unique = HashSet::default();
    let mut iter = entities.into_iter();
    if iter.by_ref().all(|x| unique.insert(x)) {
        let mut result = Vec::with_capacity(unique.len());

        for entity in iter {
            match unsafe { query.get_unchecked(entity) } {
                Ok(item) => {
                    result.push(item);
                }
                Err(err) => {
                    return Err(MultiEntityQueryErr::QueryEntityError(err));
                }
            }
        }

        Ok(result)
    } else {
        Err(MultiEntityQueryErr::DuplicateEntities)
    }
}

Just a quick implementation I've already written, not necessarily the best. Originally I used const generic arrays to avoid heap allocation too - but depends on use case.

What alternative(s) have you considered?

Have users implement it themselves using the existing unsafe Query methods, like above, and worry their use of unsafe will cause issues.

@engiwengi engiwengi added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Apr 29, 2021
@alice-i-cecile
Copy link
Member

This functionality is definitely valuable; see #2010 for another version of how this might look, and check out the Relations RFC that inspired it.

As a stop-gap, #1763 provides a limited but very commonly useful version of this functionality.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events and removed S-Needs-Triage This issue needs to be labelled labels Apr 29, 2021
@alice-i-cecile alice-i-cecile added the S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! label Dec 12, 2021
@alice-i-cecile
Copy link
Member

See #2276 for more thoughts on the API design here.

@bors bors bot closed this as completed in 5095481 Mar 30, 2022
aevyrie pushed a commit to aevyrie/bevy that referenced this issue Jun 7, 2022
…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
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
…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
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-Feature A new feature, making something new possible S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!
Projects
None yet
2 participants