Skip to content

Query::get_multiple and get_multiple_mut #3333

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

Closed
wants to merge 50 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
3e8288a
Method skeletons
alice-i-cecile Dec 13, 2021
6e7600b
Preliminary implementation
alice-i-cecile Dec 13, 2021
6b81770
Removed World API
alice-i-cecile Dec 15, 2021
ad9200c
Added doc tests
alice-i-cecile Dec 15, 2021
3707686
Improved API design
alice-i-cecile Dec 15, 2021
2a10fc2
Improved docs
alice-i-cecile Dec 15, 2021
36df4e5
Remove PR noise
alice-i-cecile Dec 15, 2021
b7a1cbe
Typo fix
alice-i-cecile Dec 15, 2021
499f804
Typo fix
alice-i-cecile Dec 15, 2021
1a5895c
Minor docs fix
alice-i-cecile Dec 15, 2021
b13d970
Add get_pair and get_pair_mut
alice-i-cecile Dec 15, 2021
bfa1d84
Fix bugs in doc tests
alice-i-cecile Dec 15, 2021
d6173f1
Add `Query::from_state` helper method
alice-i-cecile Dec 15, 2021
687b4fc
Better error-handling for get_pair
alice-i-cecile Dec 15, 2021
04288ba
Allow arbitrary iterators for get_multiple_mut
alice-i-cecile Dec 15, 2021
c42bd70
Close code blocks so they compile...
alice-i-cecile Dec 15, 2021
3bc5252
Fixed broken tests
alice-i-cecile Dec 15, 2021
9cd5c23
Improve performance by returning an iter of Results
alice-i-cecile Dec 15, 2021
aa5a23b
Avoid needless allocation in get_multiple_mut
alice-i-cecile Dec 16, 2021
5a4daf7
Fix inappropriate lifetime constraint on get_unchecked
alice-i-cecile Dec 16, 2021
6ad1922
Allocate HashSet size correctly
alice-i-cecile Dec 16, 2021
d1e262a
Use unwrap_or
alice-i-cecile Jan 7, 2022
183910c
Fix CI
alice-i-cecile Feb 14, 2022
d815de2
Initial conversion to const_generics
alice-i-cecile Feb 14, 2022
b2e499e
Fix off-by-one error
alice-i-cecile Feb 14, 2022
1a7f1d3
Remove references to get_pair in docs
alice-i-cecile Feb 14, 2022
69fb1c9
Fix doc links
alice-i-cecile Feb 14, 2022
81d6a6c
Update docs
alice-i-cecile Feb 16, 2022
94ca870
Update docs
alice-i-cecile Feb 16, 2022
a191c1e
Remove dead import
alice-i-cecile Feb 16, 2022
724d6f3
Fix iterator
alice-i-cecile Feb 16, 2022
ccf903d
Remove QueryState -> Query helper method
alice-i-cecile Feb 16, 2022
4e01623
Revert "Remove QueryState -> Query helper method"
alice-i-cecile Feb 16, 2022
c6e3241
Improve docs for `Query::from_state`
alice-i-cecile Feb 16, 2022
189974f
Compile-fail test for Query::from_state
alice-i-cecile Feb 16, 2022
ab74fd3
Fix lifetimes per Boxy
alice-i-cecile Mar 21, 2022
043abb1
Fix lifetimes per Boxy
alice-i-cecile Mar 21, 2022
7cb4330
Iterator should require a mutable reference
alice-i-cecile Mar 21, 2022
b264ffc
Mostly fix liftime errors
alice-i-cecile Mar 21, 2022
be7ec0d
Fix remaining lifetime error
alice-i-cecile Mar 21, 2022
1670877
Change return types
alice-i-cecile Mar 21, 2022
10dd581
Record which entity caused QueryEntityError errors
alice-i-cecile Mar 21, 2022
783b4f4
Remove Query::from_state method
alice-i-cecile Mar 21, 2022
a0d6632
Doc improvements
alice-i-cecile Mar 21, 2022
9c74b6d
Move methods onto QueryState
alice-i-cecile Mar 22, 2022
aff9c65
Add infallible variants
alice-i-cecile Mar 22, 2022
de5bda7
i did not realise querystate was so unbelievably MESSED Up
BoxyUwU Mar 22, 2022
dfc8258
Progress!
alice-i-cecile Mar 22, 2022
52d57c1
Return a result of an array, rather than an array of results
alice-i-cecile Mar 22, 2022
bee959e
More comments, more safety
alice-i-cecile Mar 22, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 140 additions & 3 deletions crates/bevy_ecs/src/query/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,141 @@ where
}
}

/// Returns the read-only query results for the provided Array of [`Entity`]s.
///
/// These values follow the order of your input array.
/// In case of a nonexisting entity,
/// a [`QueryEntityError`] is returned instead.
///
/// # Example
/// ```rust
/// # use bevy_ecs::prelude::*;
/// #[derive(Component, PartialEq, Debug)]
/// struct A(u64);
///
/// let mut world = World::new();
/// let entity_1 = world.spawn().insert(A(1)).id();
/// let entity_2 = world.spawn().insert(A(2)).id();
/// let entity_3 = world.spawn().insert(A(3)).id();
///
/// let query_state = world.query::<&A>();
/// let mut a_iterator = a_query.get_multiple([entity_3, entity_2, entity_1]).map(|i|i.unwrap());
/// assert_eq!(*a_iterator.next().unwrap(), A(3));
/// assert_eq!(*a_iterator.next().unwrap(), A(2));
/// assert_eq!(*a_iterator.next().unwrap(), A(1));
/// ```
#[inline]
pub fn get_multiple<'w, 's, const N: usize>(
&'s self,
world: &'w World,
entities: [Entity; N],
) -> Result<[<Q::ReadOnlyFetch as Fetch<'w, 's>>::Item; N], QueryEntityError> {
let array_of_results = entities.map(move |e| self.get(world, e));

// If any of the entities were missing, return the first error
for result in array_of_results {
result?;
}

// At this point, we're guaranteed that all results are okay
Ok(array_of_results.map(move |result| result.unwrap()))
}

/// Returns the read-only query items for the provided Array of [`Entity`]s.
///
/// # Panics
///
/// Panics if any entities do not exist.
#[inline]
pub fn multiple<'w, 's, const N: usize>(
&'s self,
world: &'w World,
entities: [Entity; N],
) -> [<Q::ReadOnlyFetch as Fetch<'w, 's>>::Item; N] {
self.get_multiple(world, entities).unwrap()
}

/// Returns the query results for the provided Array of [`Entity`]s.
///
/// These values follow the order of your input array.
/// In case of nonunique or nonexisting entities or a mismatched component,
/// a [`QueryEntityError`] is returned instead.
///
/// If you absolutely cannot afford the overhead of verifying uniqueness in this way,
/// you can (carefully) call the unsafe [`get_unchecked`](Self::get_unchecked) method repeatedly instead.
///
/// # Example
/// ```rust
/// # use bevy_ecs::prelude::*;
/// #[derive(Component, PartialEq, Debug)]
/// struct A(u64);
///
/// let mut world = World::new();
/// let entity_1 = world.spawn().insert(A(1)).id();
/// let entity_2 = world.spawn().insert(A(2)).id();
/// let entity_3 = world.spawn().insert(A(3)).id();
///
/// let query_state = world.query::<&mut A>();
/// let mut a_iterator = a_query.get_multiple_mut([entity_1, entity_3]).map(|i|i.unwrap());
/// let mut a_1 = a_iterator.next().unwrap();
/// let mut a_3 = a_iterator.next().unwrap();
///
/// *a_1 = A(11);
/// *a_3 = A(33);
///
/// // Manually drop references so we can access the `World` again
/// std::mem::drop(a_iterator);
/// std::mem::drop(a_query);
///
/// assert_eq!(*world.get::<A>(entity_1).unwrap(), A(11));
/// assert_eq!(*world.get::<A>(entity_2).unwrap(), A(2));
/// assert_eq!(*world.get::<A>(entity_3).unwrap(), A(33));
/// ```
#[inline]
pub fn get_multiple_mut<'w, 's, const N: usize>(
&'s mut self,
world: &'w mut World,
entities: [Entity; N],
) -> Result<[<Q::Fetch as Fetch<'w, 's>>::Item; N], QueryEntityError> {
// Brute force verification of uniqueness
for entity_i in entities {
for entity_j in entities {
if entity_i == entity_j {
return Err(QueryEntityError::AliasedMutability(entity_i));
}
}
}

// SAFE: the entities are checked for uniqueness above
// No other references to the query can be live, as this method takes &mut self
// The World cannot be modified in other ways as we take &mut World
unsafe {
let array_of_results = entities.map(move |e| self.get_unchecked(world, e));

// If any of the entities were missing, return the first error
for result in array_of_results {
result?;
}

// At this point, we're guaranteed that all results are okay
Ok(array_of_results.map(move |result| result.unwrap()))
}
}

/// Returns the query items for the provided Array of [`Entity`]s.
///
/// # Panics
///
/// Panics if any entities do not exist, or any entities are repeated.
#[inline]
pub fn multiple_mut<'w, 's, const N: usize>(
&'s mut self,
world: &'w mut World,
entities: [Entity; N],
) -> [<Q::Fetch as Fetch<'w, 's>>::Item; N] {
self.get_multiple_mut(world, entities).unwrap()
}

/// Gets the query result for the given [`World`] and [`Entity`].
///
/// # Safety
Expand Down Expand Up @@ -228,7 +363,7 @@ where
let location = world
.entities
.get(entity)
.ok_or(QueryEntityError::NoSuchEntity)?;
.ok_or(QueryEntityError::NoSuchEntity(entity))?;
if !self
.matched_archetypes
.contains(location.archetype_id.index())
Expand Down Expand Up @@ -733,10 +868,12 @@ where
}

/// An error that occurs when retrieving a specific [`Entity`]'s query result.
#[derive(Error, Debug)]
#[derive(Error, Debug, Clone, Copy)]
pub enum QueryEntityError {
#[error("The given entity does not have the requested component.")]
QueryDoesNotMatch,
#[error("The requested entity does not exist.")]
NoSuchEntity,
NoSuchEntity(Entity),
#[error("The same entity was accessed mutably more than once.")]
AliasedMutability(Entity),
}
63 changes: 60 additions & 3 deletions crates/bevy_ecs/src/system/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,9 @@ use thiserror::Error;
/// If you have an [`Entity`] ID, you can use the [`get`](Self::get) or
/// [`get_mut`](Self::get_mut) methods to access the query result for that particular entity.
///
/// If you require access to the data of multiple entities at once,
/// you can use the [`get_multiple`](Self::get_multiple) or [`get_multiple_mut`](Self::get_multiple_mut) methods.
///
/// ## Getting a single query result
///
/// While it's possible to get a single result from a query by using `iter.next()`, a more
Expand Down Expand Up @@ -551,7 +554,7 @@ where
};
}

/// Returns the query result for the given [`Entity`].
/// Returns the read-only query result for the given [`Entity`].
///
/// In case of a nonexisting entity or mismatched component, a [`QueryEntityError`] is
/// returned instead.
Expand Down Expand Up @@ -584,7 +587,7 @@ where
/// ```
#[inline]
pub fn get(
&'s self,
&self,
entity: Entity,
) -> Result<<Q::ReadOnlyFetch as Fetch<'w, 's>>::Item, QueryEntityError> {
// SAFE: system runs without conflicts with other systems.
Expand Down Expand Up @@ -640,6 +643,60 @@ where
}
}

/// Returns the read-only query results for the provided Array of [`Entity`]s.
///
/// These values follow the order of your input array.
/// In case of a nonexisting entity,
/// a [`QueryEntityError`] is returned instead.
#[inline]
pub fn get_multiple<const N: usize>(
&self,
entities: [Entity; N],
) -> Result<[<Q::ReadOnlyFetch as Fetch<'_, 's>>::Item; N], QueryEntityError> {
self.state.get_multiple(self.world, entities)
}

/// Returns the read-only query items for the provided Array of [`Entity`]s.
///
/// # Panics
///
/// Panics if any entities do not exist.
#[inline]
pub fn multiple<const N: usize>(
&self,
entities: [Entity; N],
) -> [<Q::ReadOnlyFetch as Fetch<'_, 's>>::Item; N] {
self.state.multiple(self.world, entities)
}

/// Returns the query results for the provided Array of [`Entity`]s.
///
/// These values follow the order of your input array.
/// In case of nonunique or nonexisting entities or a mismatched component,
/// a [`QueryEntityError`] is returned instead.
///
/// If you absolutely cannot afford the overhead of verifying uniqueness in this way,
/// you can (carefully) call the unsafe [`get_unchecked`](Self::get_unchecked) method repeatedly instead.
#[inline]
pub fn get_multiple_mut<const N: usize>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imo this shouldn't use an iterator or hash set. To avoid allocations and hashing, it should do a brute force in place entity collision check. For small numbers of entities (which the const generic basically enforces) this will be much much faster. I expect 99% of use cases to use 2 or 3 entities (one comparison and three comparisons, respectively). And it should return a fixed size array:

if let Ok([c1, c2, c3]) = query.get_multiple_mut([e1, e2, e3]) {
}

Copy link
Member

@cart cart Feb 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick and dirty impl that ignores error handling:

    #[inline]
    pub fn get_multiple<const N: usize>(
        &'s self,
        entities: [Entity; N],
    ) -> Result<[<Q::ReadOnlyFetch as Fetch<'w, 's>>::Item; N], QueryEntityError> {
        for i in 0..entities.len() {
            for j in (i + 1)..entities.len() {
                if entities[i] == entities[j] {
                    panic!("cannot call on matching entities")
                }
            }
        }
        Ok(entities.map(|e| unsafe {
            self.state
                .get_unchecked_manual::<Q::ReadOnlyFetch>(
                    self.world,
                    e,
                    self.last_change_tick,
                    self.change_tick,
                )
                .unwrap()
        }))
    }

&mut self,
entities: [Entity; N],
) -> Result<[<Q::Fetch as Fetch<'_, 's>>::Item; N], QueryEntityError> {
self.state.get_multiple_mut(self.world, entities)
}

/// Returns the query items for the provided Array of [`Entity`]s.
///
/// # Panics
/// Panics if any entities do not exist, or any entities are repeated.
#[inline]
pub fn multiple_mut<const N: usize>(
&mut self,
entities: [Entity; N],
) -> [<Q::Fetch as Fetch<'_, 's>>::Item; N] {
self.state.multiple_mut(self.world, entities)
}

/// Returns the query result for the given [`Entity`].
///
/// In case of a nonexisting entity or mismatched component, a [`QueryEntityError`] is
Expand All @@ -651,7 +708,7 @@ where
/// this call does not result in multiple mutable references to the same component
#[inline]
pub unsafe fn get_unchecked(
&'s self,
&self,
entity: Entity,
) -> Result<<Q::Fetch as Fetch<'w, 's>>::Item, QueryEntityError> {
// SEMI-SAFE: system runs without conflicts with other systems.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
use bevy_ecs::prelude::*;

#[derive(Component, Eq, PartialEq, Debug)]
struct A(usize);

fn main() {
let mut world = World::default();
world.spawn().insert(A(1));

let first_query_state: QueryState<&mut A> = world.query();
let second_query_state: QueryState<&mut A> = world.query();

let mut first_query = Query::from_state(&mut world, &first_query_state);
// This should fail to compile, as another query is already active
let mut second_query = Query::from_state(&mut world, &second_query_state);

// This is a clear violation of no-aliased mutability
assert_eq!(*first_query.single_mut(), *second_query.single_mut());
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
error[E0499]: cannot borrow `world` as mutable more than once at a time
--> tests/ui/query_state_uniqueness_safety.rs:15:46
|
13 | let mut first_query = Query::from_state(&mut world, &first_query_state);
| ---------- first mutable borrow occurs here
14 | // This should fail to compile, as another query is already active
15 | let mut second_query = Query::from_state(&mut world, &second_query_state);
| ^^^^^^^^^^ second mutable borrow occurs here
...
18 | assert_eq!(*first_query.single_mut(), *second_query.single_mut());
| ------------------------ first borrow later used here