From 33ed41c0dccb6ddf5668a6508b13c19a143e3a1a Mon Sep 17 00:00:00 2001 From: Chris Russell <8494645+chescock@users.noreply.github.com> Date: Sat, 8 Feb 2025 11:56:27 -0500 Subject: [PATCH 1/6] Move implementations of `Query` methods from `QueryState` to `Query`. Add missing `iter_many_unique_inner` and `single_inner` methods. --- crates/bevy_ecs/src/query/error.rs | 2 +- crates/bevy_ecs/src/query/par_iter.rs | 6 +- crates/bevy_ecs/src/query/state.rs | 618 +++----------------------- crates/bevy_ecs/src/system/query.rs | 396 ++++++++++++++--- 4 files changed, 395 insertions(+), 627 deletions(-) diff --git a/crates/bevy_ecs/src/query/error.rs b/crates/bevy_ecs/src/query/error.rs index 054da37a75747..464d0b2fb1fe0 100644 --- a/crates/bevy_ecs/src/query/error.rs +++ b/crates/bevy_ecs/src/query/error.rs @@ -17,7 +17,7 @@ pub enum QueryEntityError<'w> { NoSuchEntity(Entity, EntityDoesNotExistDetails), /// The [`Entity`] was requested mutably more than once. /// - /// See [`QueryState::get_many_mut`](crate::query::QueryState::get_many_mut) for an example. + /// See [`Query::get_many_mut`](crate::system::Query::get_many_mut) for an example. AliasedMutability(Entity), } diff --git a/crates/bevy_ecs/src/query/par_iter.rs b/crates/bevy_ecs/src/query/par_iter.rs index b3ea93fbbc514..6b034f983e3c6 100644 --- a/crates/bevy_ecs/src/query/par_iter.rs +++ b/crates/bevy_ecs/src/query/par_iter.rs @@ -89,7 +89,8 @@ impl<'w, 's, D: QueryData, F: QueryFilter> QueryParIter<'w, 's, D, F> { // at the same time. unsafe { self.state - .iter_unchecked_manual(self.world, self.last_run, self.this_run) + .query_unchecked_manual_with_ticks(self.world, self.last_run, self.this_run) + .into_iter() .fold(init, func); } } @@ -101,7 +102,8 @@ impl<'w, 's, D: QueryData, F: QueryFilter> QueryParIter<'w, 's, D, F> { // SAFETY: See the safety comment above. unsafe { self.state - .iter_unchecked_manual(self.world, self.last_run, self.this_run) + .query_unchecked_manual_with_ticks(self.world, self.last_run, self.this_run) + .into_iter() .fold(init, func); } } else { diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index cd495251e6e33..458753ace9112 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -1,21 +1,17 @@ use crate::{ archetype::{Archetype, ArchetypeComponentId, ArchetypeGeneration, ArchetypeId}, - batching::BatchingStrategy, component::{ComponentId, Tick}, entity::{Entity, EntityBorrow, EntitySet}, entity_disabling::DefaultQueryFilters, prelude::FromWorld, - query::{ - Access, DebugCheckedUnwrap, FilteredAccess, QueryCombinationIter, QueryIter, QueryParIter, - WorldQuery, - }, + query::{Access, FilteredAccess, QueryCombinationIter, QueryIter, QueryParIter, WorldQuery}, storage::{SparseSetIndex, TableId}, system::Query, world::{unsafe_world_cell::UnsafeWorldCell, World, WorldId}, }; use alloc::vec::Vec; -use core::{fmt, mem::MaybeUninit, ptr}; +use core::{fmt, ptr}; use fixedbitset::FixedBitSet; use log::warn; #[cfg(feature = "trace")] @@ -451,17 +447,15 @@ impl QueryState { /// [`Changed`]: crate::query::Changed #[inline] pub fn is_empty(&self, world: &World, last_run: Tick, this_run: Tick) -> bool { - self.validate_world(world.id()); - // SAFETY: - // - We have read-only access to the entire world. - // - The world has been validated. + // SAFETY: We have read access to the entire world, and `is_empty()` only performs read access. unsafe { - self.is_empty_unsafe_world_cell( + self.query_unchecked_manual_with_ticks( world.as_unsafe_world_cell_readonly(), last_run, this_run, ) } + .is_empty() } /// Returns `true` if the given [`Entity`] matches the query. @@ -469,41 +463,15 @@ impl QueryState { /// This is always guaranteed to run in `O(1)` time. #[inline] pub fn contains(&self, entity: Entity, world: &World, last_run: Tick, this_run: Tick) -> bool { - // SAFETY: NopFetch does not access any members while &self ensures no one has exclusive access + // SAFETY: We have read access to the entire world, and `contains()` only performs read access. unsafe { - self.as_nop() - .get_unchecked_manual( - world.as_unsafe_world_cell_readonly(), - entity, - last_run, - this_run, - ) - .is_ok() - } - } - - /// Checks if the query is empty for the given [`UnsafeWorldCell`]. - /// - /// # Safety - /// - /// - `world` must have permission to read any components required by this instance's `F` [`QueryFilter`]. - /// - `world` must match the one used to create this [`QueryState`]. - #[inline] - pub(crate) unsafe fn is_empty_unsafe_world_cell( - &self, - world: UnsafeWorldCell, - last_run: Tick, - this_run: Tick, - ) -> bool { - // SAFETY: - // - The caller ensures that `world` has permission to access any data used by the filter. - // - The caller ensures that the world matches. - unsafe { - self.as_nop() - .iter_unchecked_manual(world, last_run, this_run) - .next() - .is_none() + self.query_unchecked_manual_with_ticks( + world.as_unsafe_world_cell_readonly(), + last_run, + this_run, + ) } + .contains(entity) } /// Updates the state's internal view of the [`World`]'s archetypes. If this is not called before querying data, @@ -929,16 +897,7 @@ impl QueryState { world: &'w World, entity: Entity, ) -> Result, QueryEntityError<'w>> { - self.update_archetypes(world); - // SAFETY: query is read only - unsafe { - self.as_readonly().get_unchecked_manual( - world.as_unsafe_world_cell_readonly(), - entity, - world.last_change_tick(), - world.read_change_tick(), - ) - } + self.query(world).get_inner(entity) } /// Returns the read-only query results for the given array of [`Entity`]. @@ -979,19 +938,7 @@ impl QueryState { world: &'w World, entities: [Entity; N], ) -> Result<[ROQueryItem<'w, D>; N], QueryEntityError<'w>> { - self.update_archetypes(world); - - // SAFETY: - // - We have read-only access to the entire world. - // - `update_archetypes` validates that the `World` matches. - unsafe { - self.get_many_read_only_manual( - world.as_unsafe_world_cell_readonly(), - entities, - world.last_change_tick(), - world.read_change_tick(), - ) - } + self.query(world).get_many_inner(entities) } /// Gets the query result for the given [`World`] and [`Entity`]. @@ -1003,18 +950,7 @@ impl QueryState { world: &'w mut World, entity: Entity, ) -> Result, QueryEntityError<'w>> { - self.update_archetypes(world); - let change_tick = world.change_tick(); - let last_change_tick = world.last_change_tick(); - // SAFETY: query has unique world access - unsafe { - self.get_unchecked_manual( - world.as_unsafe_world_cell(), - entity, - last_change_tick, - change_tick, - ) - } + self.query_mut(world).get_inner(entity) } /// Returns the query results for the given array of [`Entity`]. @@ -1061,20 +997,7 @@ impl QueryState { world: &'w mut World, entities: [Entity; N], ) -> Result<[D::Item<'w>; N], QueryEntityError<'w>> { - self.update_archetypes(world); - - let change_tick = world.change_tick(); - let last_change_tick = world.last_change_tick(); - // SAFETY: method requires exclusive world access - // and world has been validated via update_archetypes - unsafe { - self.get_many_unchecked_manual( - world.as_unsafe_world_cell(), - entities, - last_change_tick, - change_tick, - ) - } + self.query_mut(world).get_many_inner(entities) } /// Gets the query result for the given [`World`] and [`Entity`]. @@ -1096,16 +1019,7 @@ impl QueryState { world: &'w World, entity: Entity, ) -> Result, QueryEntityError<'w>> { - self.validate_world(world.id()); - // SAFETY: query is read only and world is validated - unsafe { - self.as_readonly().get_unchecked_manual( - world.as_unsafe_world_cell_readonly(), - entity, - world.last_change_tick(), - world.read_change_tick(), - ) - } + self.query_manual(world).get_inner(entity) } /// Gets the query result for the given [`World`] and [`Entity`]. @@ -1122,132 +1036,7 @@ impl QueryState { world: UnsafeWorldCell<'w>, entity: Entity, ) -> Result, QueryEntityError<'w>> { - self.update_archetypes_unsafe_world_cell(world); - self.get_unchecked_manual(world, entity, world.last_change_tick(), world.change_tick()) - } - - /// Gets the query result for the given [`World`] and [`Entity`], where the last change and - /// the current change tick are given. - /// - /// This is always guaranteed to run in `O(1)` time. - /// - /// # Safety - /// - /// This does not check for mutable query correctness. To be safe, make sure mutable queries - /// have unique access to the components they query. - /// - /// This must be called on the same `World` that the `Query` was generated from: - /// use `QueryState::validate_world` to verify this. - pub(crate) unsafe fn get_unchecked_manual<'w>( - &self, - world: UnsafeWorldCell<'w>, - entity: Entity, - last_run: Tick, - this_run: Tick, - ) -> Result, QueryEntityError<'w>> { - let location = world - .entities() - .get(entity) - .ok_or(QueryEntityError::NoSuchEntity( - entity, - world.entities().entity_does_not_exist_error_details(entity), - ))?; - if !self - .matched_archetypes - .contains(location.archetype_id.index()) - { - return Err(QueryEntityError::QueryDoesNotMatch(entity, world)); - } - let archetype = world - .archetypes() - .get(location.archetype_id) - .debug_checked_unwrap(); - let mut fetch = D::init_fetch(world, &self.fetch_state, last_run, this_run); - let mut filter = F::init_fetch(world, &self.filter_state, last_run, this_run); - - let table = world - .storages() - .tables - .get(location.table_id) - .debug_checked_unwrap(); - D::set_archetype(&mut fetch, &self.fetch_state, archetype, table); - F::set_archetype(&mut filter, &self.filter_state, archetype, table); - - if F::filter_fetch(&mut filter, entity, location.table_row) { - Ok(D::fetch(&mut fetch, entity, location.table_row)) - } else { - Err(QueryEntityError::QueryDoesNotMatch(entity, world)) - } - } - - /// Gets the read-only query results for the given [`World`] and array of [`Entity`], where the last change and - /// the current change tick are given. - /// - /// # Safety - /// - /// * `world` must have permission to read all of the components returned from this call. - /// No mutable references may coexist with any of the returned references. - /// * This must be called on the same `World` that the `Query` was generated from: - /// use `QueryState::validate_world` to verify this. - pub(crate) unsafe fn get_many_read_only_manual<'w, const N: usize>( - &self, - world: UnsafeWorldCell<'w>, - entities: [Entity; N], - last_run: Tick, - this_run: Tick, - ) -> Result<[ROQueryItem<'w, D>; N], QueryEntityError<'w>> { - let mut values = [(); N].map(|_| MaybeUninit::uninit()); - - for (value, entity) in core::iter::zip(&mut values, entities) { - // SAFETY: fetch is read-only and world must be validated - let item = unsafe { - self.as_readonly() - .get_unchecked_manual(world, entity, last_run, this_run)? - }; - *value = MaybeUninit::new(item); - } - - // SAFETY: Each value has been fully initialized. - Ok(values.map(|x| unsafe { x.assume_init() })) - } - - /// Gets the query results for the given [`World`] and array of [`Entity`], where the last change and - /// the current change tick are given. - /// - /// This is always guaranteed to run in `O(1)` time. - /// - /// # Safety - /// - /// This does not check for unique access to subsets of the entity-component data. - /// To be safe, make sure mutable queries have unique access to the components they query. - /// - /// This must be called on the same `World` that the `Query` was generated from: - /// use `QueryState::validate_world` to verify this. - pub(crate) unsafe fn get_many_unchecked_manual<'w, const N: usize>( - &self, - world: UnsafeWorldCell<'w>, - entities: [Entity; N], - last_run: Tick, - this_run: Tick, - ) -> Result<[D::Item<'w>; N], QueryEntityError<'w>> { - // Verify that all entities are unique - for i in 0..N { - for j in 0..i { - if entities[i] == entities[j] { - return Err(QueryEntityError::AliasedMutability(entities[i])); - } - } - } - - let mut values = [(); N].map(|_| MaybeUninit::uninit()); - - for (value, entity) in core::iter::zip(&mut values, entities) { - let item = self.get_unchecked_manual(world, entity, last_run, this_run)?; - *value = MaybeUninit::new(item); - } - - // SAFETY: Each value has been fully initialized. - Ok(values.map(|x| x.assume_init())) + self.query_unchecked(world).get_inner(entity) } /// Returns an [`Iterator`] over the query results for the given [`World`]. @@ -1255,15 +1044,7 @@ impl QueryState { /// This can only be called for read-only queries, see [`Self::iter_mut`] for write-queries. #[inline] pub fn iter<'w, 's>(&'s mut self, world: &'w World) -> QueryIter<'w, 's, D::ReadOnly, F> { - self.update_archetypes(world); - // SAFETY: query is read only - unsafe { - self.as_readonly().iter_unchecked_manual( - world.as_unsafe_world_cell_readonly(), - world.last_change_tick(), - world.read_change_tick(), - ) - } + self.query(world).into_iter() } /// Returns an [`Iterator`] over the query results for the given [`World`]. @@ -1272,13 +1053,7 @@ impl QueryState { /// Iteration order is not guaranteed. #[inline] pub fn iter_mut<'w, 's>(&'s mut self, world: &'w mut World) -> QueryIter<'w, 's, D, F> { - self.update_archetypes(world); - let change_tick = world.change_tick(); - let last_change_tick = world.last_change_tick(); - // SAFETY: query has unique world access - unsafe { - self.iter_unchecked_manual(world.as_unsafe_world_cell(), last_change_tick, change_tick) - } + self.query_mut(world).into_iter() } /// Returns an [`Iterator`] over the query results for the given [`World`] without updating the query's archetypes. @@ -1290,15 +1065,7 @@ impl QueryState { /// This can only be called for read-only queries. #[inline] pub fn iter_manual<'w, 's>(&'s self, world: &'w World) -> QueryIter<'w, 's, D::ReadOnly, F> { - self.validate_world(world.id()); - // SAFETY: query is read only and world is validated - unsafe { - self.as_readonly().iter_unchecked_manual( - world.as_unsafe_world_cell_readonly(), - world.last_change_tick(), - world.read_change_tick(), - ) - } + self.query_manual(world).into_iter() } /// Returns an [`Iterator`] over all possible combinations of `K` query results without repetition. @@ -1330,15 +1097,7 @@ impl QueryState { &'s mut self, world: &'w World, ) -> QueryCombinationIter<'w, 's, D::ReadOnly, F, K> { - self.update_archetypes(world); - // SAFETY: query is read only - unsafe { - self.as_readonly().iter_combinations_unchecked_manual( - world.as_unsafe_world_cell_readonly(), - world.last_change_tick(), - world.read_change_tick(), - ) - } + self.query(world).iter_combinations_inner() } /// Returns an [`Iterator`] over all possible combinations of `K` query results without repetition. @@ -1363,17 +1122,7 @@ impl QueryState { &'s mut self, world: &'w mut World, ) -> QueryCombinationIter<'w, 's, D, F, K> { - self.update_archetypes(world); - let change_tick = world.change_tick(); - let last_change_tick = world.last_change_tick(); - // SAFETY: query has unique world access - unsafe { - self.iter_combinations_unchecked_manual( - world.as_unsafe_world_cell(), - last_change_tick, - change_tick, - ) - } + self.query_mut(world).iter_combinations_inner() } /// Returns an [`Iterator`] over the read-only query items generated from an [`Entity`] list. @@ -1390,16 +1139,7 @@ impl QueryState { world: &'w World, entities: EntityList, ) -> QueryManyIter<'w, 's, D::ReadOnly, F, EntityList::IntoIter> { - self.update_archetypes(world); - // SAFETY: query is read only - unsafe { - self.as_readonly().iter_many_unchecked_manual( - entities, - world.as_unsafe_world_cell_readonly(), - world.last_change_tick(), - world.read_change_tick(), - ) - } + self.query(world).iter_many_inner(entities) } /// Returns an [`Iterator`] over the read-only query items generated from an [`Entity`] list. @@ -1422,16 +1162,7 @@ impl QueryState { world: &'w World, entities: EntityList, ) -> QueryManyIter<'w, 's, D::ReadOnly, F, EntityList::IntoIter> { - self.validate_world(world.id()); - // SAFETY: query is read only, world id is validated - unsafe { - self.as_readonly().iter_many_unchecked_manual( - entities, - world.as_unsafe_world_cell_readonly(), - world.last_change_tick(), - world.read_change_tick(), - ) - } + self.query_manual(world).iter_many_inner(entities) } /// Returns an iterator over the query items generated from an [`Entity`] list. @@ -1444,18 +1175,7 @@ impl QueryState { world: &'w mut World, entities: EntityList, ) -> QueryManyIter<'w, 's, D, F, EntityList::IntoIter> { - self.update_archetypes(world); - let change_tick = world.change_tick(); - let last_change_tick = world.last_change_tick(); - // SAFETY: Query has unique world access. - unsafe { - self.iter_many_unchecked_manual( - entities, - world.as_unsafe_world_cell(), - last_change_tick, - change_tick, - ) - } + self.query_mut(world).iter_many_inner(entities) } /// Returns an [`Iterator`] over the unique read-only query items generated from an [`EntitySet`]. @@ -1472,16 +1192,7 @@ impl QueryState { world: &'w World, entities: EntityList, ) -> QueryManyUniqueIter<'w, 's, D::ReadOnly, F, EntityList::IntoIter> { - self.update_archetypes(world); - // SAFETY: query is read only - unsafe { - self.as_readonly().iter_many_unique_unchecked_manual( - entities, - world.as_unsafe_world_cell_readonly(), - world.last_change_tick(), - world.read_change_tick(), - ) - } + self.query(world).iter_many_unique_inner(entities) } /// Returns an [`Iterator`] over the unique read-only query items generated from an [`EntitySet`]. @@ -1505,16 +1216,7 @@ impl QueryState { world: &'w World, entities: EntityList, ) -> QueryManyUniqueIter<'w, 's, D::ReadOnly, F, EntityList::IntoIter> { - self.validate_world(world.id()); - // SAFETY: query is read only, world id is validated - unsafe { - self.as_readonly().iter_many_unique_unchecked_manual( - entities, - world.as_unsafe_world_cell_readonly(), - world.last_change_tick(), - world.read_change_tick(), - ) - } + self.query_manual(world).iter_many_unique_inner(entities) } /// Returns an iterator over the unique query items generated from an [`EntitySet`]. @@ -1527,18 +1229,7 @@ impl QueryState { world: &'w mut World, entities: EntityList, ) -> QueryManyUniqueIter<'w, 's, D, F, EntityList::IntoIter> { - self.update_archetypes(world); - let last_change_tick = world.last_change_tick(); - let change_tick = world.change_tick(); - // SAFETY: Query has unique world access. - unsafe { - self.iter_many_unique_unchecked_manual( - entities, - world.as_unsafe_world_cell(), - last_change_tick, - change_tick, - ) - } + self.query_mut(world).iter_many_unique_inner(entities) } /// Returns an [`Iterator`] over the query results for the given [`World`]. /// @@ -1554,8 +1245,7 @@ impl QueryState { &'s mut self, world: UnsafeWorldCell<'w>, ) -> QueryIter<'w, 's, D, F> { - self.update_archetypes_unsafe_world_cell(world); - self.iter_unchecked_manual(world, world.last_change_tick(), world.change_tick()) + self.query_unchecked(world).into_iter() } /// Returns an [`Iterator`] over all possible combinations of `K` query results for the @@ -1574,107 +1264,7 @@ impl QueryState { &'s mut self, world: UnsafeWorldCell<'w>, ) -> QueryCombinationIter<'w, 's, D, F, K> { - self.update_archetypes_unsafe_world_cell(world); - self.iter_combinations_unchecked_manual( - world, - world.last_change_tick(), - world.change_tick(), - ) - } - - /// Returns an [`Iterator`] for the given [`World`], where the last change and - /// the current change tick are given. - /// - /// This iterator is always guaranteed to return results from each matching entity once and only once. - /// Iteration order is not guaranteed. - /// - /// # Safety - /// - /// This does not check for mutable query correctness. To be safe, make sure mutable queries - /// have unique access to the components they query. - /// This does not validate that `world.id()` matches `self.world_id`. Calling this on a `world` - /// with a mismatched [`WorldId`] is unsound. - #[inline] - pub(crate) unsafe fn iter_unchecked_manual<'w, 's>( - &'s self, - world: UnsafeWorldCell<'w>, - last_run: Tick, - this_run: Tick, - ) -> QueryIter<'w, 's, D, F> { - QueryIter::new(world, self, last_run, this_run) - } - - /// Returns an [`Iterator`] for the given [`World`] and list of [`Entity`]'s, where the last change and - /// the current change tick are given. - /// - /// This iterator is always guaranteed to return results from each unique pair of matching entities. - /// Iteration order is not guaranteed. - /// - /// # Safety - /// - /// This does not check for mutable query correctness. To be safe, make sure mutable queries - /// have unique access to the components they query. - /// This does not check for entity uniqueness - /// This does not validate that `world.id()` matches `self.world_id`. Calling this on a `world` - /// with a mismatched [`WorldId`] is unsound. - #[inline] - pub(crate) unsafe fn iter_many_unchecked_manual<'w, 's, EntityList>( - &'s self, - entities: EntityList, - world: UnsafeWorldCell<'w>, - last_run: Tick, - this_run: Tick, - ) -> QueryManyIter<'w, 's, D, F, EntityList::IntoIter> - where - EntityList: IntoIterator, - { - QueryManyIter::new(world, self, entities, last_run, this_run) - } - - /// Returns an [`Iterator`] for the given [`World`] and an [`EntitySet`], where the last change and - /// the current change tick are given. - /// - /// Items are returned in the order of the list of entities. - /// Entities that don't match the query are skipped. - /// - /// # Safety - /// - /// This does not check for mutable query correctness. To be safe, make sure mutable queries - /// have unique access to the components they query. - /// This does not validate that `world.id()` matches `self.world_id`. Calling this on a `world` - /// with a mismatched [`WorldId`] is unsound. - #[inline] - pub(crate) unsafe fn iter_many_unique_unchecked_manual<'w, 's, EntityList: EntitySet>( - &'s self, - entities: EntityList, - world: UnsafeWorldCell<'w>, - last_run: Tick, - this_run: Tick, - ) -> QueryManyUniqueIter<'w, 's, D, F, EntityList::IntoIter> { - QueryManyUniqueIter::new(world, self, entities, last_run, this_run) - } - - /// Returns an [`Iterator`] over all possible combinations of `K` query results for the - /// given [`World`] without repetition. - /// This can only be called for read-only queries. - /// - /// This iterator is always guaranteed to return results from each unique pair of matching entities. - /// Iteration order is not guaranteed. - /// - /// # Safety - /// - /// This does not check for mutable query correctness. To be safe, make sure mutable queries - /// have unique access to the components they query. - /// This does not validate that `world.id()` matches `self.world_id`. Calling this on a `world` - /// with a mismatched [`WorldId`] is unsound. - #[inline] - pub(crate) unsafe fn iter_combinations_unchecked_manual<'w, 's, const K: usize>( - &'s self, - world: UnsafeWorldCell<'w>, - last_run: Tick, - this_run: Tick, - ) -> QueryCombinationIter<'w, 's, D, F, K> { - QueryCombinationIter::new(world, self, last_run, this_run) + self.query_unchecked(world).iter_combinations_inner() } /// Returns a parallel iterator over the query results for the given [`World`]. @@ -1690,14 +1280,7 @@ impl QueryState { &'s mut self, world: &'w World, ) -> QueryParIter<'w, 's, D::ReadOnly, F> { - self.update_archetypes(world); - QueryParIter { - world: world.as_unsafe_world_cell_readonly(), - state: self.as_readonly(), - last_run: world.last_change_tick(), - this_run: world.read_change_tick(), - batching_strategy: BatchingStrategy::new(), - } + self.query(world).par_iter_inner() } /// Returns a parallel iterator over the query results for the given [`World`]. @@ -1746,16 +1329,7 @@ impl QueryState { /// [`ComputeTaskPool`]: bevy_tasks::ComputeTaskPool #[inline] pub fn par_iter_mut<'w, 's>(&'s mut self, world: &'w mut World) -> QueryParIter<'w, 's, D, F> { - self.update_archetypes(world); - let this_run = world.change_tick(); - let last_run = world.last_change_tick(); - QueryParIter { - world: world.as_unsafe_world_cell(), - state: self, - last_run, - this_run, - batching_strategy: BatchingStrategy::new(), - } + self.query_mut(world).par_iter_inner() } /// Runs `func` on each query result in parallel for the given [`World`], where the last change and @@ -1809,7 +1383,9 @@ impl QueryState { scope.spawn(async move { #[cfg(feature = "trace")] let _span = self.par_iter_span.enter(); - let mut iter = self.iter_unchecked_manual(world, last_run, this_run); + let mut iter = self + .query_unchecked_manual_with_ticks(world, last_run, this_run) + .into_iter(); let mut accum = init_accum(); for storage_id in queue { accum = iter.fold_over_storage_range(accum, &mut func, storage_id, None); @@ -1828,7 +1404,8 @@ impl QueryState { #[cfg(feature = "trace")] let _span = self.par_iter_span.enter(); let accum = init_accum(); - self.iter_unchecked_manual(world, last_run, this_run) + self.query_unchecked_manual_with_ticks(world, last_run, this_run) + .into_iter() .fold_over_storage_range(accum, &mut func, storage_id, Some(batch)); }); } @@ -1881,10 +1458,7 @@ impl QueryState { #[track_caller] #[inline] pub fn single<'w>(&mut self, world: &'w World) -> ROQueryItem<'w, D> { - match self.get_single(world) { - Ok(items) => items, - Err(error) => panic!("Cannot get single query result: {error}"), - } + self.query(world).single_inner() } /// Returns a single immutable query result when there is exactly one entity matching @@ -1900,16 +1474,7 @@ impl QueryState { &mut self, world: &'w World, ) -> Result, QuerySingleError> { - self.update_archetypes(world); - - // SAFETY: query is read only - unsafe { - self.as_readonly().get_single_unchecked_manual( - world.as_unsafe_world_cell_readonly(), - world.last_change_tick(), - world.read_change_tick(), - ) - } + self.query(world).get_single_inner() } /// Returns a single mutable query result when there is exactly one entity matching @@ -1922,11 +1487,7 @@ impl QueryState { #[track_caller] #[inline] pub fn single_mut<'w>(&mut self, world: &'w mut World) -> D::Item<'w> { - // SAFETY: query has unique world access - match self.get_single_mut(world) { - Ok(items) => items, - Err(error) => panic!("Cannot get single query result: {error}"), - } + self.query_mut(world).single_inner() } /// Returns a single mutable query result when there is exactly one entity matching @@ -1939,18 +1500,7 @@ impl QueryState { &mut self, world: &'w mut World, ) -> Result, QuerySingleError> { - self.update_archetypes(world); - - let change_tick = world.change_tick(); - let last_change_tick = world.last_change_tick(); - // SAFETY: query has unique world access - unsafe { - self.get_single_unchecked_manual( - world.as_unsafe_world_cell(), - last_change_tick, - change_tick, - ) - } + self.query_mut(world).get_single_inner() } /// Returns a query result when there is exactly one entity matching the query. @@ -1967,8 +1517,7 @@ impl QueryState { &mut self, world: UnsafeWorldCell<'w>, ) -> Result, QuerySingleError> { - self.update_archetypes_unsafe_world_cell(world); - self.get_single_unchecked_manual(world, world.last_change_tick(), world.change_tick()) + self.query_unchecked(world).get_single_inner() } /// Returns a query result when there is exactly one entity matching the query, @@ -1988,17 +1537,8 @@ impl QueryState { last_run: Tick, this_run: Tick, ) -> Result, QuerySingleError> { - let mut query = self.iter_unchecked_manual(world, last_run, this_run); - let first = query.next(); - let extra = query.next().is_some(); - - match (first, extra) { - (Some(r), false) => Ok(r), - (None, _) => Err(QuerySingleError::NoEntities(core::any::type_name::())), - (Some(_), _) => Err(QuerySingleError::MultipleEntities(core::any::type_name::< - Self, - >())), - } + self.query_unchecked_manual_with_ticks(world, last_run, this_run) + .get_single_inner() } } @@ -2022,70 +1562,38 @@ mod tests { let entities: Vec = (0..10).map(|_| world.spawn_empty().id()).collect(); - let query_state = world.query::(); - - // These don't matter for the test - let last_change_tick = world.last_change_tick(); - let change_tick = world.change_tick(); + let mut query_state = world.query::(); - // It's best to test get_many_unchecked_manual directly, - // as it is shared and unsafe + // It's best to test get_many_inner directly, as it is shared // We don't care about aliased mutability for the read-only equivalent // SAFETY: Query does not access world data. - assert!(unsafe { - query_state - .get_many_unchecked_manual::<10>( - world.as_unsafe_world_cell_readonly(), - entities.clone().try_into().unwrap(), - last_change_tick, - change_tick, - ) - .is_ok() - }); + assert!(query_state + .query_mut(&mut world) + .get_many_inner::<10>(entities.clone().try_into().unwrap()) + .is_ok()); assert_eq!( - // SAFETY: Query does not access world data. - unsafe { - query_state - .get_many_unchecked_manual( - world.as_unsafe_world_cell_readonly(), - [entities[0], entities[0]], - last_change_tick, - change_tick, - ) - .unwrap_err() - }, + query_state + .query_mut(&mut world) + .get_many_inner([entities[0], entities[0]]) + .unwrap_err(), QueryEntityError::AliasedMutability(entities[0]) ); assert_eq!( - // SAFETY: Query does not access world data. - unsafe { - query_state - .get_many_unchecked_manual( - world.as_unsafe_world_cell_readonly(), - [entities[0], entities[1], entities[0]], - last_change_tick, - change_tick, - ) - .unwrap_err() - }, + query_state + .query_mut(&mut world) + .get_many_inner([entities[0], entities[1], entities[0]]) + .unwrap_err(), QueryEntityError::AliasedMutability(entities[0]) ); assert_eq!( - // SAFETY: Query does not access world data. - unsafe { - query_state - .get_many_unchecked_manual( - world.as_unsafe_world_cell_readonly(), - [entities[9], entities[9]], - last_change_tick, - change_tick, - ) - .unwrap_err() - }, + query_state + .query_mut(&mut world) + .get_many_inner([entities[9], entities[9]]) + .unwrap_err(), QueryEntityError::AliasedMutability(entities[9]) ); } diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index fe5662febc77f..6f52ad4afc5a2 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -3,14 +3,15 @@ use crate::{ component::Tick, entity::{Entity, EntityBorrow, EntitySet}, query::{ - QueryCombinationIter, QueryData, QueryEntityError, QueryFilter, QueryIter, QueryManyIter, - QueryManyUniqueIter, QueryParIter, QuerySingleError, QueryState, ROQueryItem, - ReadOnlyQueryData, + DebugCheckedUnwrap, NopWorldQuery, QueryCombinationIter, QueryData, QueryEntityError, + QueryFilter, QueryIter, QueryManyIter, QueryManyUniqueIter, QueryParIter, QuerySingleError, + QueryState, ROQueryItem, ReadOnlyQueryData, }, world::unsafe_world_cell::UnsafeWorldCell, }; use core::{ marker::PhantomData, + mem::MaybeUninit, ops::{Deref, DerefMut}, }; @@ -439,6 +440,15 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> { unsafe { self.reborrow_unsafe() }.into_readonly() } + /// Returns another `Query` from this does not return any data, which can be faster. + fn as_nop(&self) -> Query<'w, 's, NopWorldQuery, F> { + let new_state = self.state.as_nop(); + // SAFETY: + // - This is memory safe because it performs no access. + // - The world matches because it was the same one used to construct self. + unsafe { Query::new(self.world, new_state, self.last_run, self.this_run) } + } + /// Returns another `Query` from this that fetches the read-only version of the query items. /// /// For example, `Query<(&mut D1, &D2, &mut D3), With>` will become `Query<(&D1, &D2, &D3), With>`. @@ -498,6 +508,24 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> { /// /// - [`reborrow`](Self::reborrow) for the safe versions. pub unsafe fn reborrow_unsafe(&self) -> Query<'_, 's, D, F> { + // SAFETY: + // - This is memory safe because the caller ensures that there are no conflicting references. + // - The world matches because it was the same one used to construct self. + unsafe { self.copy_unsafe() } + } + + /// Returns a new `Query` copying the access from this one. + /// The current query will still be usable while the new one exists, but must not be used in a way that violates aliasing. + /// + /// # Safety + /// + /// This function makes it possible to violate Rust's aliasing guarantees. + /// You must make sure this call does not result in a mutable or shared reference to a component with a mutable reference. + /// + /// # See also + /// + /// - [`reborrow_unsafe`](Self::reborrow_unsafe) for a safer version that constrains the returned `'w` lifetime to the length of the borrow. + unsafe fn copy_unsafe(&self) -> Query<'w, 's, D, F> { // SAFETY: // - This is memory safe because the caller ensures that there are no conflicting references. // - The world matches because it was the same one used to construct self. @@ -653,10 +681,7 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> { #[inline] pub fn iter_combinations_inner(self) -> QueryCombinationIter<'w, 's, D, F, K> { // SAFETY: `self.world` has permission to access the required components. - unsafe { - self.state - .iter_combinations_unchecked_manual(self.world, self.last_run, self.this_run) - } + unsafe { QueryCombinationIter::new(self.world, self.state, self.last_run, self.this_run) } } /// Returns an [`Iterator`] over the read-only query items generated from an [`Entity`] list. @@ -766,9 +791,10 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> { ) -> QueryManyIter<'w, 's, D, F, EntityList::IntoIter> { // SAFETY: `self.world` has permission to access the required components. unsafe { - self.state.iter_many_unchecked_manual( - entities, + QueryManyIter::new( self.world, + self.state, + entities, self.last_run, self.this_run, ) @@ -822,22 +848,13 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> { /// # See also /// /// - [`iter_many_unique_mut`](Self::iter_many_unique_mut) to get mutable query items. + /// - [`iter_many_unique_inner`](Self::iter_many_unique_inner) to get with the actual "inner" world lifetime. #[inline] pub fn iter_many_unique( &self, entities: EntityList, ) -> QueryManyUniqueIter<'_, 's, D::ReadOnly, F, EntityList::IntoIter> { - // SAFETY: - // - `self.world` has permission to access the required components. - // - The query is read-only, so it can be aliased even if it was originally mutable. - unsafe { - self.state.as_readonly().iter_many_unique_unchecked_manual( - entities, - self.world, - self.last_run, - self.this_run, - ) - } + self.as_readonly().iter_many_unique_inner(entities) } /// Returns an iterator over the unique query items generated from an [`EntitySet`]. @@ -883,16 +900,76 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> { /// } /// # bevy_ecs::system::assert_is_system(system); /// ``` + /// # See also + /// + /// - [`iter_many_unique`](Self::iter_many_unique) to get read-only query items. + /// - [`iter_many_unique_inner`](Self::iter_many_unique_inner) to get with the actual "inner" world lifetime. #[inline] pub fn iter_many_unique_mut( &mut self, entities: EntityList, ) -> QueryManyUniqueIter<'_, 's, D, F, EntityList::IntoIter> { + self.reborrow().iter_many_unique_inner(entities) + } + + /// Returns an iterator over the unique query items generated from an [`EntitySet`]. + /// This consumes the [`Query`] to return results with the actual "inner" world lifetime. + /// + /// Items are returned in the order of the list of entities. Entities that don't match the query are skipped. + /// + /// # Examples + /// + /// ``` + /// # use bevy_ecs::{prelude::*, entity::{EntitySet, UniqueEntityIter}}; + /// # use core::slice; + /// #[derive(Component)] + /// struct Counter { + /// value: i32 + /// } + /// + /// // `Friends` ensures that it only lists unique entities. + /// #[derive(Component)] + /// struct Friends { + /// unique_list: Vec, + /// } + /// + /// impl<'a> IntoIterator for &'a Friends { + /// type Item = &'a Entity; + /// type IntoIter = UniqueEntityIter>; + /// + /// fn into_iter(self) -> Self::IntoIter { + /// // SAFETY: `Friends` ensures that it unique_list contains only unique entities. + /// unsafe { UniqueEntityIter::from_iterator_unchecked(self.unique_list.iter()) } + /// } + /// } + /// + /// fn system( + /// friends_query: Query<&Friends>, + /// mut counter_query: Query<&mut Counter>, + /// ) { + /// let friends = friends_query.single(); + /// for mut counter in counter_query.iter_many_unique_inner(friends) { + /// println!("Friend's counter: {:?}", counter.value); + /// counter.value += 1; + /// } + /// } + /// # bevy_ecs::system::assert_is_system(system); + /// ``` + /// # See also + /// + /// - [`iter_many_unique`](Self::iter_many_unique) to get read-only query items. + /// - [`iter_many_unique_mut`](Self::iter_many_unique_mut) to get mutable query items. + #[inline] + pub fn iter_many_unique_inner( + self, + entities: EntityList, + ) -> QueryManyUniqueIter<'w, 's, D, F, EntityList::IntoIter> { // SAFETY: `self.world` has permission to access the required components. unsafe { - self.state.iter_many_unique_unchecked_manual( - entities, + QueryManyUniqueIter::new( self.world, + self.state, + entities, self.last_run, self.this_run, ) @@ -972,22 +1049,15 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> { /// /// # See also /// - /// - [`iter_many_mut`](Self::iter_many_mut) to safely access the query items. + /// - [`iter_many_unique`](Self::iter_many_unique) to get read-only query items. + /// - [`iter_many_unique_mut`](Self::iter_many_unique_mut) to get mutable query items. + /// - [`iter_many_unique_inner`](Self::iter_many_unique_inner) to get with the actual "inner" world lifetime. pub unsafe fn iter_many_unique_unsafe( &self, entities: EntityList, ) -> QueryManyUniqueIter<'_, 's, D, F, EntityList::IntoIter> { - // SAFETY: - // - `self.world` has permission to access the required components. - // - The caller ensures that this operation will not result in any aliased mutable accesses. - unsafe { - self.state.iter_many_unique_unchecked_manual( - entities, - self.world, - self.last_run, - self.this_run, - ) - } + // SAFETY: The caller promises that this will not result in multiple mutable references. + unsafe { self.reborrow_unsafe() }.iter_many_unique_inner(entities) } /// Returns a parallel iterator over the query results for the given [`World`]. @@ -1126,6 +1196,39 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> { /// In case of a nonexisting entity or mismatched component, a [`QueryEntityError`] is returned instead. /// The elements of the array do not need to be unique, unlike `get_many_mut`. /// + /// # Examples + /// + /// ``` + /// use bevy_ecs::prelude::*; + /// use bevy_ecs::query::QueryEntityError; + /// + /// #[derive(Component, PartialEq, Debug)] + /// struct A(usize); + /// + /// let mut world = World::new(); + /// let entity_vec: Vec = (0..3).map(|i| world.spawn(A(i)).id()).collect(); + /// let entities: [Entity; 3] = entity_vec.try_into().unwrap(); + /// + /// world.spawn(A(73)); + /// + /// let mut query_state = world.query::<&A>(); + /// let query = query_state.query(&world); + /// + /// let component_values = query.get_many(entities).unwrap(); + /// + /// assert_eq!(component_values, [&A(0), &A(1), &A(2)]); + /// + /// let wrong_entity = Entity::from_raw(365); + /// + /// assert_eq!( + /// match query.get_many([wrong_entity]).unwrap_err() { + /// QueryEntityError::NoSuchEntity(entity, _) => entity, + /// _ => panic!(), + /// }, + /// wrong_entity + /// ); + /// ``` + /// /// # See also /// /// - [`get_many_mut`](Self::get_many_mut) to get mutable query items. @@ -1239,8 +1342,55 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> { // SAFETY: system runs without conflicts with other systems. // same-system queries have runtime borrow checks when they conflict unsafe { - self.state - .get_unchecked_manual(self.world, entity, self.last_run, self.this_run) + let location = + self.world + .entities() + .get(entity) + .ok_or(QueryEntityError::NoSuchEntity( + entity, + self.world + .entities() + .entity_does_not_exist_error_details(entity), + ))?; + if !self + .state + .matched_archetypes + .contains(location.archetype_id.index()) + { + return Err(QueryEntityError::QueryDoesNotMatch(entity, self.world)); + } + let archetype = self + .world + .archetypes() + .get(location.archetype_id) + .debug_checked_unwrap(); + let mut fetch = D::init_fetch( + self.world, + &self.state.fetch_state, + self.last_run, + self.this_run, + ); + let mut filter = F::init_fetch( + self.world, + &self.state.filter_state, + self.last_run, + self.this_run, + ); + + let table = self + .world + .storages() + .tables + .get(location.table_id) + .debug_checked_unwrap(); + D::set_archetype(&mut fetch, &self.state.fetch_state, archetype, table); + F::set_archetype(&mut filter, &self.state.filter_state, archetype, table); + + if F::filter_fetch(&mut filter, entity, location.table_row) { + Ok(D::fetch(&mut fetch, entity, location.table_row)) + } else { + Err(QueryEntityError::QueryDoesNotMatch(entity, self.world)) + } } } @@ -1249,6 +1399,65 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> { /// The returned query items are in the same order as the input. /// In case of a nonexisting entity, duplicate entities or mismatched component, a [`QueryEntityError`] is returned instead. /// + /// # Examples + /// + /// ``` + /// use bevy_ecs::prelude::*; + /// use bevy_ecs::query::QueryEntityError; + /// + /// #[derive(Component, PartialEq, Debug)] + /// struct A(usize); + /// + /// let mut world = World::new(); + /// + /// let entities: Vec = (0..3).map(|i| world.spawn(A(i)).id()).collect(); + /// let entities: [Entity; 3] = entities.try_into().unwrap(); + /// + /// world.spawn(A(73)); + /// let wrong_entity = Entity::from_raw(57); + /// let invalid_entity = world.spawn_empty().id(); + /// + /// + /// let mut query_state = world.query::<&mut A>(); + /// let mut query = query_state.query_mut(&mut world); + /// + /// let mut mutable_component_values = query.get_many_mut(entities).unwrap(); + /// + /// for mut a in &mut mutable_component_values { + /// a.0 += 5; + /// } + /// + /// let component_values = query.get_many(entities).unwrap(); + /// + /// assert_eq!(component_values, [&A(5), &A(6), &A(7)]); + /// + /// assert_eq!( + /// match query + /// .get_many_mut([wrong_entity]) + /// .unwrap_err() + /// { + /// QueryEntityError::NoSuchEntity(entity, _) => entity, + /// _ => panic!(), + /// }, + /// wrong_entity + /// ); + /// assert_eq!( + /// match query + /// .get_many_mut([invalid_entity]) + /// .unwrap_err() + /// { + /// QueryEntityError::QueryDoesNotMatch(entity, _) => entity, + /// _ => panic!(), + /// }, + /// invalid_entity + /// ); + /// assert_eq!( + /// query + /// .get_many_mut([entities[0], entities[0]]) + /// .unwrap_err(), + /// QueryEntityError::AliasedMutability(entities[0]) + /// ); + /// ``` /// # See also /// /// - [`get_many`](Self::get_many) to get read-only query items without checking for duplicate entities. @@ -1278,11 +1487,17 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> { self, entities: [Entity; N], ) -> Result<[D::Item<'w>; N], QueryEntityError<'w>> { - // SAFETY: scheduler ensures safe Query world access - unsafe { - self.state - .get_many_unchecked_manual(self.world, entities, self.last_run, self.this_run) + // Verify that all entities are unique + for i in 0..N { + for j in 0..i { + if entities[i] == entities[j] { + return Err(QueryEntityError::AliasedMutability(entities[i])); + } + } } + + // SAFETY: All entities are unique, so the results don't alias. + unsafe { self.get_many_impl(entities) } } /// Returns the query items for the given array of [`Entity`]. @@ -1304,11 +1519,31 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> { 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) + // SAFETY: The query results are read-only, so they don't conflict if there are duplicate entities. + unsafe { self.get_many_impl(entities) } + } + + /// Returns the query items for the given array of [`Entity`]. + /// This consumes the [`Query`] to return results with the actual "inner" world lifetime. + /// + /// # Safety + /// + /// The caller must ensure that the query data returned for the entities does not conflict, + /// either because they are all unique or because the data is read-only. + unsafe fn get_many_impl( + self, + entities: [Entity; N], + ) -> Result<[D::Item<'w>; N], QueryEntityError<'w>> { + let mut values = [(); N].map(|_| MaybeUninit::uninit()); + + for (value, entity) in core::iter::zip(&mut values, entities) { + // SAFETY: The caller asserts that the results don't alias + let item = unsafe { self.copy_unsafe() }.get_inner(entity)?; + *value = MaybeUninit::new(item); } + + // SAFETY: Each value has been fully initialized. + Ok(values.map(|x| unsafe { x.assume_init() })) } /// Returns the query items for the given array of [`Entity`]. @@ -1516,6 +1751,40 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> { self.reborrow().get_single_inner() } + /// Returns a single query item when there is exactly one entity matching the query. + /// This consumes the [`Query`] to return results with the actual "inner" world lifetime. + /// + /// # Panics + /// + /// This method panics if the number of query items is **not** exactly one. + /// + /// # Example + /// + /// ``` + /// # use bevy_ecs::prelude::*; + /// # + /// # #[derive(Component)] + /// # struct Player; + /// # #[derive(Component)] + /// # struct Health(u32); + /// # + /// fn regenerate_player_health_system(query: Query<&mut Health, With>) { + /// let mut health = query.single_inner(); + /// health.0 += 1; + /// } + /// # bevy_ecs::system::assert_is_system(regenerate_player_health_system); + /// ``` + /// + /// # See also + /// + /// - [`get_single_inner`](Self::get_single_inner) for the non-panicking version. + /// - [`single`](Self::single) to get the read-only query item. + /// - [`single_mut`](Self::single_mut) to get the mutable query item. + #[track_caller] + pub fn single_inner(self) -> D::Item<'w> { + self.get_single_inner().unwrap() + } + /// Returns a single query item when there is exactly one entity matching the query. /// This consumes the [`Query`] to return results with the actual "inner" world lifetime. /// @@ -1542,14 +1811,19 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> { /// /// - [`get_single`](Self::get_single) to get the read-only query item. /// - [`get_single_mut`](Self::get_single_mut) to get the mutable query item. + /// - [`single_inner`](Self::single_inner) for the panicking version. #[inline] pub fn get_single_inner(self) -> Result, QuerySingleError> { - // SAFETY: - // the query ensures mutable access to the components it accesses, and the query - // is uniquely borrowed - unsafe { - self.state - .get_single_unchecked_manual(self.world, self.last_run, self.this_run) + let mut query = self.into_iter(); + let first = query.next(); + let extra = query.next().is_some(); + + match (first, extra) { + (Some(r), false) => Ok(r), + (None, _) => Err(QuerySingleError::NoEntities(core::any::type_name::())), + (Some(_), _) => Err(QuerySingleError::MultipleEntities(core::any::type_name::< + Self, + >())), } } @@ -1583,14 +1857,7 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> { /// [`Changed`]: crate::query::Changed #[inline] pub fn is_empty(&self) -> bool { - // SAFETY: - // - `self.world` has permission to read any data required by the WorldQuery. - // - `&self` ensures that no one currently has write access. - // - `self.world` matches `self.state`. - unsafe { - self.state - .is_empty_unsafe_world_cell(self.world, self.last_run, self.this_run) - } + self.as_nop().iter().next().is_none() } /// Returns `true` if the given [`Entity`] matches the query. @@ -1619,13 +1886,7 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> { /// ``` #[inline] pub fn contains(&self, entity: Entity) -> bool { - // SAFETY: NopFetch does not access any members while &self ensures no one has exclusive access - unsafe { - self.state - .as_nop() - .get_unchecked_manual(self.world, entity, self.last_run, self.this_run) - .is_ok() - } + self.as_nop().get(entity).is_ok() } /// Returns a [`QueryLens`] that can be used to get a query with a more general fetch. @@ -2060,10 +2321,7 @@ impl<'w, 's, D: QueryData, F: QueryFilter> IntoIterator for Query<'w, 's, D, F> // - `self.world` has permission to access the required components. // - We consume the query, so mutable queries cannot alias. // Read-only queries are `Copy`, but may alias themselves. - unsafe { - self.state - .iter_unchecked_manual(self.world, self.last_run, self.this_run) - } + unsafe { QueryIter::new(self.world, self.state, self.last_run, self.this_run) } } } From 6da68a5535c1eb00b72f9a315ef125f28ee22d19 Mon Sep 17 00:00:00 2001 From: Chris Russell <8494645+chescock@users.noreply.github.com> Date: Wed, 12 Feb 2025 10:30:43 -0500 Subject: [PATCH 2/6] Replace use of removed `is_empty_unsafe_world_cell`. --- crates/bevy_ecs/src/system/system_param.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index a12323e26417a..6af70ae6ef6fd 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -562,9 +562,14 @@ unsafe impl SystemParam // SAFETY: // - We have read-only access to the components accessed by query. // - The world has been validated. - !unsafe { - state.is_empty_unsafe_world_cell(world, system_meta.last_run, world.change_tick()) - } + let query = unsafe { + state.query_unchecked_manual_with_ticks( + world, + system_meta.last_run, + world.change_tick(), + ) + }; + !query.is_empty() } } From d55f688b655f03c27570ed96ba1a16b8ed8c548c Mon Sep 17 00:00:00 2001 From: Chris Russell <8494645+chescock@users.noreply.github.com> Date: Wed, 12 Feb 2025 11:46:08 -0500 Subject: [PATCH 3/6] Move `get_many_unchecked_manual_uniqueness()` test from the file with `QueryState` to the file with `Query`. --- crates/bevy_ecs/src/query/state.rs | 45 +-------------------------- crates/bevy_ecs/src/system/query.rs | 48 +++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 44 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 458753ace9112..150a741829e93 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -1552,51 +1552,8 @@ impl From> for QueryState = (0..10).map(|_| world.spawn_empty().id()).collect(); - - let mut query_state = world.query::(); - - // It's best to test get_many_inner directly, as it is shared - // We don't care about aliased mutability for the read-only equivalent - - // SAFETY: Query does not access world data. - assert!(query_state - .query_mut(&mut world) - .get_many_inner::<10>(entities.clone().try_into().unwrap()) - .is_ok()); - - assert_eq!( - query_state - .query_mut(&mut world) - .get_many_inner([entities[0], entities[0]]) - .unwrap_err(), - QueryEntityError::AliasedMutability(entities[0]) - ); - - assert_eq!( - query_state - .query_mut(&mut world) - .get_many_inner([entities[0], entities[1], entities[0]]) - .unwrap_err(), - QueryEntityError::AliasedMutability(entities[0]) - ); - - assert_eq!( - query_state - .query_mut(&mut world) - .get_many_inner([entities[9], entities[9]]) - .unwrap_err(), - QueryEntityError::AliasedMutability(entities[9]) - ); - } #[test] #[should_panic] diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 6f52ad4afc5a2..01261d4d885ca 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -2481,3 +2481,51 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Populated<'w, 's, D, F> { self.0 } } + +#[cfg(test)] +mod tests { + use crate::{prelude::*, query::QueryEntityError}; + use alloc::vec::Vec; + + #[test] + fn get_many_uniqueness() { + let mut world = World::new(); + + let entities: Vec = (0..10).map(|_| world.spawn_empty().id()).collect(); + + let mut query_state = world.query::(); + + // It's best to test get_many_inner directly, as it is shared + // We don't care about aliased mutability for the read-only equivalent + + // SAFETY: Query does not access world data. + assert!(query_state + .query_mut(&mut world) + .get_many_inner::<10>(entities.clone().try_into().unwrap()) + .is_ok()); + + assert_eq!( + query_state + .query_mut(&mut world) + .get_many_inner([entities[0], entities[0]]) + .unwrap_err(), + QueryEntityError::AliasedMutability(entities[0]) + ); + + assert_eq!( + query_state + .query_mut(&mut world) + .get_many_inner([entities[0], entities[1], entities[0]]) + .unwrap_err(), + QueryEntityError::AliasedMutability(entities[0]) + ); + + assert_eq!( + query_state + .query_mut(&mut world) + .get_many_inner([entities[9], entities[9]]) + .unwrap_err(), + QueryEntityError::AliasedMutability(entities[9]) + ); + } +} From 5e38516ee497850fdae557455a51ee677bdf34e5 Mon Sep 17 00:00:00 2001 From: Chris Russell <8494645+chescock@users.noreply.github.com> Date: Thu, 13 Feb 2025 19:30:08 -0500 Subject: [PATCH 4/6] Update new uses of `iter_many_unchecked_manual` and `iter_many_unique_unchecked_manual` for parallel iteration. --- crates/bevy_ecs/src/query/par_iter.rs | 32 +++++++-------------------- crates/bevy_ecs/src/query/state.rs | 12 ++++++---- 2 files changed, 16 insertions(+), 28 deletions(-) diff --git a/crates/bevy_ecs/src/query/par_iter.rs b/crates/bevy_ecs/src/query/par_iter.rs index 3a43f9c56df13..0805774f8cdde 100644 --- a/crates/bevy_ecs/src/query/par_iter.rs +++ b/crates/bevy_ecs/src/query/par_iter.rs @@ -264,12 +264,8 @@ impl<'w, 's, D: ReadOnlyQueryData, F: QueryFilter, E: EntityBorrow + Sync> // at the same time. unsafe { self.state - .iter_many_unchecked_manual( - &self.entity_list, - self.world, - self.last_run, - self.this_run, - ) + .query_unchecked_manual_with_ticks(self.world, self.last_run, self.this_run) + .iter_many_inner(&self.entity_list) .fold(init, func); } } @@ -281,12 +277,8 @@ impl<'w, 's, D: ReadOnlyQueryData, F: QueryFilter, E: EntityBorrow + Sync> // SAFETY: See the safety comment above. unsafe { self.state - .iter_many_unchecked_manual( - &self.entity_list, - self.world, - self.last_run, - self.this_run, - ) + .query_unchecked_manual_with_ticks(self.world, self.last_run, self.this_run) + .iter_many_inner(&self.entity_list) .fold(init, func); } } else { @@ -432,12 +424,8 @@ impl<'w, 's, D: QueryData, F: QueryFilter, E: TrustedEntityBorrow + Sync> // at the same time. unsafe { self.state - .iter_many_unique_unchecked_manual( - self.entity_list, - self.world, - self.last_run, - self.this_run, - ) + .query_unchecked_manual_with_ticks(self.world, self.last_run, self.this_run) + .iter_many_unique_inner(self.entity_list) .fold(init, func); } } @@ -449,12 +437,8 @@ impl<'w, 's, D: QueryData, F: QueryFilter, E: TrustedEntityBorrow + Sync> // SAFETY: See the safety comment above. unsafe { self.state - .iter_many_unique_unchecked_manual( - self.entity_list, - self.world, - self.last_run, - self.this_run, - ) + .query_unchecked_manual_with_ticks(self.world, self.last_run, self.this_run) + .iter_many_unique_inner(self.entity_list) .fold(init, func); } } else { diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index bb2f4a4e2d3fd..770c8faeb4d4e 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -1495,7 +1495,8 @@ impl QueryState { #[cfg(feature = "trace")] let _span = self.par_iter_span.enter(); let accum = init_accum(); - self.iter_many_unique_unchecked_manual(batch, world, last_run, this_run) + self.query_unchecked_manual_with_ticks(world, last_run, this_run) + .iter_many_unique(batch) .fold(accum, &mut func); }); } @@ -1503,7 +1504,8 @@ impl QueryState { #[cfg(feature = "trace")] let _span = self.par_iter_span.enter(); let accum = init_accum(); - self.iter_many_unique_unchecked_manual(remainder, world, last_run, this_run) + self.query_unchecked_manual_with_ticks(world, last_run, this_run) + .iter_many_unique(remainder) .fold(accum, &mut func); }); } @@ -1556,7 +1558,8 @@ impl QueryState { #[cfg(feature = "trace")] let _span = self.par_iter_span.enter(); let accum = init_accum(); - self.iter_many_unchecked_manual(batch, world, last_run, this_run) + self.query_unchecked_manual_with_ticks(world, last_run, this_run) + .iter_many_inner(batch) .fold(accum, &mut func); }); } @@ -1564,7 +1567,8 @@ impl QueryState { #[cfg(feature = "trace")] let _span = self.par_iter_span.enter(); let accum = init_accum(); - self.iter_many_unchecked_manual(remainder, world, last_run, this_run) + self.query_unchecked_manual_with_ticks(world, last_run, this_run) + .iter_many_inner(remainder) .fold(accum, &mut func); }); } From 51046cecc92eaef934f34c4f6b429f54593f001d Mon Sep 17 00:00:00 2001 From: Chris Russell <8494645+chescock@users.noreply.github.com> Date: Thu, 13 Feb 2025 20:07:27 -0500 Subject: [PATCH 5/6] Missed an `_inner` suffix. --- crates/bevy_ecs/src/query/state.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 770c8faeb4d4e..f3ad5ca77a458 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -1496,7 +1496,7 @@ impl QueryState { let _span = self.par_iter_span.enter(); let accum = init_accum(); self.query_unchecked_manual_with_ticks(world, last_run, this_run) - .iter_many_unique(batch) + .iter_many_unique_inner(batch) .fold(accum, &mut func); }); } @@ -1505,7 +1505,7 @@ impl QueryState { let _span = self.par_iter_span.enter(); let accum = init_accum(); self.query_unchecked_manual_with_ticks(world, last_run, this_run) - .iter_many_unique(remainder) + .iter_many_unique_inner(remainder) .fold(accum, &mut func); }); } From 14007f9ad5a4ad87d0876048fd8a6da4623c60e1 Mon Sep 17 00:00:00 2001 From: Chris Russell <8494645+chescock@users.noreply.github.com> Date: Fri, 14 Feb 2025 16:22:04 -0500 Subject: [PATCH 6/6] Remove `validate_world()` call from `query_unchecked_manual_with_ticks()` and make it a safety requirement. --- crates/bevy_ecs/src/query/iter.rs | 2 ++ crates/bevy_ecs/src/query/state.rs | 35 +++++++++++++++++----- crates/bevy_ecs/src/system/system_param.rs | 11 +++---- 3 files changed, 36 insertions(+), 12 deletions(-) diff --git a/crates/bevy_ecs/src/query/iter.rs b/crates/bevy_ecs/src/query/iter.rs index 33ae0e262f12c..d81ce7b85928f 100644 --- a/crates/bevy_ecs/src/query/iter.rs +++ b/crates/bevy_ecs/src/query/iter.rs @@ -849,6 +849,7 @@ impl<'w, 's, D: QueryData, F: QueryFilter> QueryIter<'w, 's, D, F> { // SAFETY: // `self.world` has permission to access the required components. // The original query iter has not been iterated on, so no items are aliased from it. + // `QueryIter::new` ensures `world` is the same one used to initialize `query_state`. let query_lens = unsafe { query_lens_state.query_unchecked_manual(world) }.into_iter(); let mut keyed_query: Vec<_> = query_lens .map(|(key, entity)| (key, NeutralOrd(entity))) @@ -1683,6 +1684,7 @@ impl<'w, 's, D: QueryData, F: QueryFilter, I: Iterator> // SAFETY: // `self.world` has permission to access the required components. // The original query iter has not been iterated on, so no items are aliased from it. + // `QueryIter::new` ensures `world` is the same one used to initialize `query_state`. let query_lens = unsafe { query_lens_state.query_unchecked_manual(world) } .iter_many_inner(self.entity_iter); let mut keyed_query: Vec<_> = query_lens diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index f3ad5ca77a458..b568897ee3553 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -337,7 +337,10 @@ impl QueryState { /// /// This will create read-only queries, see [`Self::query_mut`] for mutable queries. pub fn query_manual<'w, 's>(&'s self, world: &'w World) -> Query<'w, 's, D::ReadOnly, F> { - // SAFETY: We have read access to the entire world, and we call `as_readonly()` so the query only performs read access. + self.validate_world(world.id()); + // SAFETY: + // - We have read access to the entire world, and we call `as_readonly()` so the query only performs read access. + // - We called `validate_world`. unsafe { self.as_readonly() .query_unchecked_manual(world.as_unsafe_world_cell_readonly()) @@ -381,13 +384,17 @@ impl QueryState { /// /// This does not check for mutable query correctness. To be safe, make sure mutable queries /// have unique access to the components they query. + /// This does not validate that `world.id()` matches `self.world_id`. Calling this on a `world` + /// with a mismatched [`WorldId`] is unsound. pub unsafe fn query_unchecked_manual<'w, 's>( &'s self, world: UnsafeWorldCell<'w>, ) -> Query<'w, 's, D, F> { let last_run = world.last_change_tick(); let this_run = world.change_tick(); - // SAFETY: The caller ensured we have the correct access to the world. + // SAFETY: + // - The caller ensured we have the correct access to the world. + // - The caller ensured that the world matches. unsafe { self.query_unchecked_manual_with_ticks(world, last_run, this_run) } } @@ -404,7 +411,9 @@ impl QueryState { this_run: Tick, ) -> Query<'w, 's, D, F> { self.update_archetypes_unsafe_world_cell(world); - // SAFETY: The caller ensured we have the correct access to the world. + // SAFETY: + // - The caller ensured we have the correct access to the world. + // - We called `update_archetypes_unsafe_world_cell`, which calls `validate_world`. unsafe { self.query_unchecked_manual_with_ticks(world, last_run, this_run) } } @@ -422,16 +431,17 @@ impl QueryState { /// /// This does not check for mutable query correctness. To be safe, make sure mutable queries /// have unique access to the components they query. + /// This does not validate that `world.id()` matches `self.world_id`. Calling this on a `world` + /// with a mismatched [`WorldId`] is unsound. pub unsafe fn query_unchecked_manual_with_ticks<'w, 's>( &'s self, world: UnsafeWorldCell<'w>, last_run: Tick, this_run: Tick, ) -> Query<'w, 's, D, F> { - self.validate_world(world.id()); // SAFETY: // - The caller ensured we have the correct access to the world. - // - `validate_world` did not panic, so the world matches. + // - The caller ensured that the world matches. unsafe { Query::new(world, self, last_run, this_run) } } @@ -450,7 +460,10 @@ impl QueryState { /// [`Changed`]: crate::query::Changed #[inline] pub fn is_empty(&self, world: &World, last_run: Tick, this_run: Tick) -> bool { - // SAFETY: We have read access to the entire world, and `is_empty()` only performs read access. + self.validate_world(world.id()); + // SAFETY: + // - We have read access to the entire world, and `is_empty()` only performs read access. + // - We called `validate_world`. unsafe { self.query_unchecked_manual_with_ticks( world.as_unsafe_world_cell_readonly(), @@ -466,7 +479,10 @@ impl QueryState { /// This is always guaranteed to run in `O(1)` time. #[inline] pub fn contains(&self, entity: Entity, world: &World, last_run: Tick, this_run: Tick) -> bool { - // SAFETY: We have read access to the entire world, and `contains()` only performs read access. + self.validate_world(world.id()); + // SAFETY: + // - We have read access to the entire world, and `is_empty()` only performs read access. + // - We called `validate_world`. unsafe { self.query_unchecked_manual_with_ticks( world.as_unsafe_world_cell_readonly(), @@ -1660,6 +1676,8 @@ impl QueryState { /// /// This does not check for mutable query correctness. To be safe, make sure mutable queries /// have unique access to the components they query. + /// This does not validate that `world.id()` matches `self.world_id`. Calling this on a `world` + /// with a mismatched [`WorldId`] is unsound. #[inline] pub unsafe fn get_single_unchecked_manual<'w>( &self, @@ -1667,6 +1685,9 @@ impl QueryState { last_run: Tick, this_run: Tick, ) -> Result, QuerySingleError> { + // SAFETY: + // - The caller ensured we have the correct access to the world. + // - The caller ensured that the world matches. self.query_unchecked_manual_with_ticks(world, last_run, this_run) .get_single_inner() } diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 6af70ae6ef6fd..8ef804a926be5 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -333,6 +333,7 @@ unsafe impl SystemParam for Qu // SAFETY: We have registered all of the query's world accesses, // so the caller ensures that `world` has permission to access any // world data that the query needs. + // The caller ensures the world matches the one used in init_state. unsafe { state.query_unchecked_manual_with_ticks(world, system_meta.last_run, change_tick) } } } @@ -401,8 +402,8 @@ unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam fo world: UnsafeWorldCell<'w>, change_tick: Tick, ) -> Self::Item<'w, 's> { - state.validate_world(world.id()); // SAFETY: State ensures that the components it accesses are not accessible somewhere elsewhere. + // The caller ensures the world matches the one used in init_state. let query = unsafe { state.query_unchecked_manual_with_ticks(world, system_meta.last_run, change_tick) }; @@ -421,9 +422,9 @@ unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam fo system_meta: &SystemMeta, world: UnsafeWorldCell, ) -> bool { - state.validate_world(world.id()); // SAFETY: State ensures that the components it accesses are not mutably accessible elsewhere // and the query is read only. + // The caller ensures the world matches the one used in init_state. let query = unsafe { state.query_unchecked_manual_with_ticks( world, @@ -469,6 +470,7 @@ unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam ) -> Self::Item<'w, 's> { state.validate_world(world.id()); // SAFETY: State ensures that the components it accesses are not accessible elsewhere. + // The caller ensures the world matches the one used in init_state. let query = unsafe { state.query_unchecked_manual_with_ticks(world, system_meta.last_run, change_tick) }; @@ -488,9 +490,9 @@ unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam system_meta: &SystemMeta, world: UnsafeWorldCell, ) -> bool { - state.validate_world(world.id()); // SAFETY: State ensures that the components it accesses are not mutably accessible elsewhere // and the query is read only. + // The caller ensures the world matches the one used in init_state. let query = unsafe { state.query_unchecked_manual_with_ticks( world, @@ -558,10 +560,9 @@ unsafe impl SystemParam system_meta: &SystemMeta, world: UnsafeWorldCell, ) -> bool { - state.validate_world(world.id()); // SAFETY: // - We have read-only access to the components accessed by query. - // - The world has been validated. + // - The caller ensures the world matches the one used in init_state. let query = unsafe { state.query_unchecked_manual_with_ticks( world,