Skip to content

Commit

Permalink
Query::get_many should not check for duplicates (#17724)
Browse files Browse the repository at this point in the history
# Objective

Restore the behavior of `Query::get_many` prior to #15858.  

When passed duplicate `Entity`s, `get_many` is supposed to return
results for all of them, since read-only queries don't alias. However,
#15858 merged the implementation with `get_many_mut` and caused it to
return `QueryEntityError::AliasedMutability`.

## Solution

Introduce a new `Query::get_many_readonly` method that consumes the
`Query` like `get_many_inner`, but that is constrained to `D:
ReadOnlyQueryData` so that it can skip the aliasing check. Implement
`Query::get_many` in terms of that new method. Add a test, and a comment
explaining why it doesn't match the pattern of the other `&self`
methods.
  • Loading branch information
chescock authored Feb 10, 2025
1 parent 7f9588d commit c34a2c2
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 3 deletions.
12 changes: 12 additions & 0 deletions crates/bevy_ecs/src/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,18 @@ mod tests {
);
}

#[test]
fn get_many_only_mut_checks_duplicates() {
let mut world = World::new();
let id = world.spawn(A(10)).id();
let mut query_state = world.query::<&mut A>();
let mut query = query_state.query_mut(&mut world);
let result = query.get_many([id, id]);
assert_eq!(result, Ok([&A(10), &A(10)]));
let mut_result = query.get_many_mut([id, id]);
assert!(mut_result.is_err());
}

#[test]
fn multi_storage_query() {
let mut world = World::new();
Expand Down
36 changes: 33 additions & 3 deletions crates/bevy_ecs/src/system/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1135,7 +1135,9 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> {
&self,
entities: [Entity; N],
) -> Result<[ROQueryItem<'_, D>; N], QueryEntityError> {
self.as_readonly().get_many_inner(entities)
// Note that this calls `get_many_readonly` instead of `get_many_inner`
// since we don't need to check for duplicates.
self.as_readonly().get_many_readonly(entities)
}

/// Returns the read-only query items for the given array of [`Entity`].
Expand Down Expand Up @@ -1249,7 +1251,7 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> {
///
/// # See also
///
/// - [`get_many`](Self::get_many) to get read-only query items.
/// - [`get_many`](Self::get_many) to get read-only query items without checking for duplicate entities.
/// - [`many_mut`](Self::many_mut) for the panicking version.
#[inline]
pub fn get_many_mut<const N: usize>(
Expand All @@ -1267,8 +1269,10 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> {
///
/// # See also
///
/// - [`get_many`](Self::get_many) to get read-only query items.
/// - [`get_many`](Self::get_many) to get read-only query items without checking for duplicate entities.
/// - [`get_many_mut`](Self::get_many_mut) to get items using a mutable reference.
/// - [`get_many_readonly`](Self::get_many_readonly) to get read-only query items without checking for duplicate entities
/// with the actual "inner" world lifetime.
#[inline]
pub fn get_many_inner<const N: usize>(
self,
Expand All @@ -1281,6 +1285,32 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> {
}
}

/// Returns the query items for the given array of [`Entity`].
/// This consumes the [`Query`] to return results with the actual "inner" world lifetime.
///
/// The returned query items are in the same order as the input.
/// In case of a nonexisting entity or mismatched component, a [`QueryEntityError`] is returned instead.
///
/// # See also
///
/// - [`get_many`](Self::get_many) to get read-only query items without checking for duplicate entities.
/// - [`get_many_mut`](Self::get_many_mut) to get items using a mutable reference.
/// - [`get_many_inner`](Self::get_many_readonly) to get mutable query items with the actual "inner" world lifetime.
#[inline]
pub fn get_many_readonly<const N: usize>(
self,
entities: [Entity; N],
) -> Result<[D::Item<'w>; N], QueryEntityError<'w>>
where
D: ReadOnlyQueryData,
{
// SAFETY: scheduler ensures safe Query world access
unsafe {
self.state
.get_many_read_only_manual(self.world, entities, self.last_run, self.this_run)
}
}

/// Returns the query items for the given array of [`Entity`].
///
/// # Panics
Expand Down

0 comments on commit c34a2c2

Please sign in to comment.