From bb2bbb9d3ccccaea48bd902d0e6c81969128db22 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Sat, 10 Jun 2023 21:03:09 -0400 Subject: [PATCH 01/22] port WorldQuery to UnsafeWorldCell --- crates/bevy_ecs/macros/src/fetch.rs | 2 +- crates/bevy_ecs/src/query/fetch.rs | 54 ++++++++++++++++++++--------- crates/bevy_ecs/src/query/filter.rs | 30 ++++++++++++---- 3 files changed, 62 insertions(+), 24 deletions(-) diff --git a/crates/bevy_ecs/macros/src/fetch.rs b/crates/bevy_ecs/macros/src/fetch.rs index 6383aa6fa306a..c2d925a56b94d 100644 --- a/crates/bevy_ecs/macros/src/fetch.rs +++ b/crates/bevy_ecs/macros/src/fetch.rs @@ -257,7 +257,7 @@ pub fn derive_world_query_impl(input: TokenStream) -> TokenStream { } unsafe fn init_fetch<'__w>( - _world: &'__w #path::world::World, + _world: #path::world::unsafe_world_cell::UnsafeWorldCell<'__w>, state: &Self::State, _last_run: #path::component::Tick, _this_run: #path::component::Tick, diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 4540909f61685..5fc02c5ab4d5e 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -5,7 +5,7 @@ use crate::{ entity::Entity, query::{Access, DebugCheckedUnwrap, FilteredAccess}, storage::{ComponentSparseSet, Table, TableRow}, - world::{Mut, Ref, World}, + world::{unsafe_world_cell::UnsafeWorldCell, Mut, Ref, World}, }; pub use bevy_ecs_macros::WorldQuery; use bevy_ptr::{ThinSlicePtr, UnsafeCellDeref}; @@ -335,10 +335,11 @@ pub unsafe trait WorldQuery { /// /// # Safety /// - /// `state` must have been initialized (via [`WorldQuery::init_state`]) using the same `world` passed - /// in to this function. + /// - `world` must have permission to access any of the components specified in `Self::update_archetype_component_access`. + /// - `state` must have been initialized (via [`WorldQuery::init_state`]) using the same `world` passed + /// in to this function. unsafe fn init_fetch<'w>( - world: &'w World, + world: UnsafeWorldCell<'w>, state: &Self::State, last_run: Tick, this_run: Tick, @@ -372,8 +373,10 @@ pub unsafe trait WorldQuery { /// /// # Safety /// - /// `archetype` and `tables` must be from the [`World`] [`WorldQuery::init_state`] was called on. `state` must - /// be the [`Self::State`] this was initialized with. + /// - `archetype` and `tables` must be from the same [`World`] that [`WorldQuery::init_state`] was called on. + /// - [`Self::update_archetype_component_access`] must have been previously called with `archetype`. + /// - `table` must correspond to `archetype`. + /// - `state` must be the [`State`](Self::state) that `fetch` was initialized with. unsafe fn set_archetype<'w>( fetch: &mut Self::Fetch<'w>, state: &Self::State, @@ -386,8 +389,10 @@ pub unsafe trait WorldQuery { /// /// # Safety /// - /// `table` must be from the [`World`] [`WorldQuery::init_state`] was called on. `state` must be the - /// [`Self::State`] this was initialized with. + /// - `table` must be from the same [`World`] that [`WorldQuery::init_state`] was called on. + /// - `table` must belong to an archetype that was previously registered with + /// [`Self::update_archetype_component_access`]. + /// - `state` must be the [`State`](Self::state) that `fetch` was initialized with. unsafe fn set_table<'w>(fetch: &mut Self::Fetch<'w>, state: &Self::State, table: &'w Table); /// Fetch [`Self::Item`](`WorldQuery::Item`) for either the given `entity` in the current [`Table`], @@ -475,7 +480,7 @@ unsafe impl WorldQuery for Entity { const IS_ARCHETYPAL: bool = true; unsafe fn init_fetch<'w>( - _world: &'w World, + _world: UnsafeWorldCell<'w>, _state: &Self::State, _last_run: Tick, _this_run: Tick, @@ -558,7 +563,7 @@ unsafe impl WorldQuery for &T { #[inline] unsafe fn init_fetch<'w>( - world: &'w World, + world: UnsafeWorldCell<'w>, &component_id: &ComponentId, _last_run: Tick, _this_run: Tick, @@ -567,6 +572,11 @@ unsafe impl WorldQuery for &T { table_components: None, sparse_set: (T::Storage::STORAGE_TYPE == StorageType::SparseSet).then(|| { world + // SAFETY: The underlying type associated with `component_id` is `T`, + // which we are allowed to access since we registered it in `update_archetype_component_access`. + // Note that we do not actually access any components in this function, we just get a shared + // reference to the sparse set, which is used to access the components in `Self::fetch`. + .unsafe_world() .storages() .sparse_sets .get(component_id) @@ -704,7 +714,7 @@ unsafe impl<'__w, T: Component> WorldQuery for Ref<'__w, T> { #[inline] unsafe fn init_fetch<'w>( - world: &'w World, + world: UnsafeWorldCell<'w>, &component_id: &ComponentId, last_run: Tick, this_run: Tick, @@ -713,6 +723,8 @@ unsafe impl<'__w, T: Component> WorldQuery for Ref<'__w, T> { table_data: None, sparse_set: (T::Storage::STORAGE_TYPE == StorageType::SparseSet).then(|| { world + // SAFETY: See &T::init_fetch. + .unsafe_world() .storages() .sparse_sets .get(component_id) @@ -866,7 +878,7 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T { #[inline] unsafe fn init_fetch<'w>( - world: &'w World, + world: UnsafeWorldCell<'w>, &component_id: &ComponentId, last_run: Tick, this_run: Tick, @@ -875,6 +887,8 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T { table_data: None, sparse_set: (T::Storage::STORAGE_TYPE == StorageType::SparseSet).then(|| { world + // SAFETY: See &T::init_fetch. + .unsafe_world() .storages() .sparse_sets .get(component_id) @@ -1011,7 +1025,7 @@ unsafe impl WorldQuery for Option { #[inline] unsafe fn init_fetch<'w>( - world: &'w World, + world: UnsafeWorldCell<'w>, state: &T::State, last_run: Tick, this_run: Tick, @@ -1116,7 +1130,7 @@ macro_rules! impl_tuple_fetch { #[inline] #[allow(clippy::unused_unit)] - unsafe fn init_fetch<'w>(_world: &'w World, state: &Self::State, _last_run: Tick, _this_run: Tick) -> Self::Fetch<'w> { + unsafe fn init_fetch<'w>(_world: UnsafeWorldCell<'w>, state: &Self::State, _last_run: Tick, _this_run: Tick) -> Self::Fetch<'w> { let ($($name,)*) = state; ($($name::init_fetch(_world, $name, _last_run, _this_run),)*) } @@ -1226,7 +1240,7 @@ macro_rules! impl_anytuple_fetch { #[inline] #[allow(clippy::unused_unit)] - unsafe fn init_fetch<'w>(_world: &'w World, state: &Self::State, _last_run: Tick, _this_run: Tick) -> Self::Fetch<'w> { + unsafe fn init_fetch<'w>(_world: UnsafeWorldCell<'w>, state: &Self::State, _last_run: Tick, _this_run: Tick) -> Self::Fetch<'w> { let ($($name,)*) = state; ($(($name::init_fetch(_world, $name, _last_run, _this_run), false),)*) } @@ -1350,7 +1364,13 @@ unsafe impl WorldQuery for NopWorldQuery { const IS_ARCHETYPAL: bool = true; #[inline(always)] - unsafe fn init_fetch(_world: &World, _state: &Q::State, _last_run: Tick, _this_run: Tick) {} + unsafe fn init_fetch( + _world: UnsafeWorldCell, + _state: &Q::State, + _last_run: Tick, + _this_run: Tick, + ) { + } unsafe fn clone_fetch<'w>(_fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> {} @@ -1408,7 +1428,7 @@ unsafe impl WorldQuery for PhantomData { fn shrink<'wlong: 'wshort, 'wshort>(_item: Self::Item<'wlong>) -> Self::Item<'wshort> {} unsafe fn init_fetch<'w>( - _world: &'w World, + _world: UnsafeWorldCell<'w>, _state: &Self::State, _last_run: Tick, _this_run: Tick, diff --git a/crates/bevy_ecs/src/query/filter.rs b/crates/bevy_ecs/src/query/filter.rs index 996ef81b12ea0..b3f039566eb0f 100644 --- a/crates/bevy_ecs/src/query/filter.rs +++ b/crates/bevy_ecs/src/query/filter.rs @@ -4,7 +4,7 @@ use crate::{ entity::Entity, query::{Access, DebugCheckedUnwrap, FilteredAccess, WorldQuery}, storage::{Column, ComponentSparseSet, Table, TableRow}, - world::World, + world::{unsafe_world_cell::UnsafeWorldCell, World}, }; use bevy_ptr::{ThinSlicePtr, UnsafeCellDeref}; use bevy_utils::all_tuples; @@ -51,7 +51,13 @@ unsafe impl WorldQuery for With { fn shrink<'wlong: 'wshort, 'wshort>(_: Self::Item<'wlong>) -> Self::Item<'wshort> {} #[inline] - unsafe fn init_fetch(_world: &World, _state: &ComponentId, _last_run: Tick, _this_run: Tick) {} + unsafe fn init_fetch( + _world: UnsafeWorldCell, + _state: &ComponentId, + _last_run: Tick, + _this_run: Tick, + ) { + } unsafe fn clone_fetch<'w>(_fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> {} @@ -148,7 +154,13 @@ unsafe impl WorldQuery for Without { fn shrink<'wlong: 'wshort, 'wshort>(_: Self::Item<'wlong>) -> Self::Item<'wshort> {} #[inline] - unsafe fn init_fetch(_world: &World, _state: &ComponentId, _last_run: Tick, _this_run: Tick) {} + unsafe fn init_fetch( + _world: UnsafeWorldCell, + _state: &ComponentId, + _last_run: Tick, + _this_run: Tick, + ) { + } unsafe fn clone_fetch<'w>(_fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> {} @@ -268,7 +280,7 @@ macro_rules! impl_query_filter_tuple { const IS_ARCHETYPAL: bool = true $(&& $filter::IS_ARCHETYPAL)*; #[inline] - unsafe fn init_fetch<'w>(world: &'w World, state: &Self::State, last_run: Tick, this_run: Tick) -> Self::Fetch<'w> { + unsafe fn init_fetch<'w>(world: UnsafeWorldCell<'w>, state: &Self::State, last_run: Tick, this_run: Tick) -> Self::Fetch<'w> { let ($($filter,)*) = state; ($(OrFetch { fetch: $filter::init_fetch(world, $filter, last_run, this_run), @@ -412,12 +424,18 @@ macro_rules! impl_tick_filter { } #[inline] - unsafe fn init_fetch<'w>(world: &'w World, &id: &ComponentId, last_run: Tick, this_run: Tick) -> Self::Fetch<'w> { + unsafe fn init_fetch<'w>( + world: UnsafeWorldCell<'w>, + &id: &ComponentId, + last_run: Tick, + this_run: Tick + ) -> Self::Fetch<'w> { Self::Fetch::<'w> { table_ticks: None, sparse_set: (T::Storage::STORAGE_TYPE == StorageType::SparseSet) .then(|| { - world.storages() + world.unsafe_world() + .storages() .sparse_sets .get(id) .debug_checked_unwrap() From 2aa96389707fad70992533a67b353e3a7f6f217a Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Sat, 10 Jun 2023 22:09:22 -0400 Subject: [PATCH 02/22] allow `UnsafeWorldCell` to be used with `QueryState::validate_world` --- crates/bevy_ecs/src/query/state.rs | 9 ++++--- .../bevy_ecs/src/world/unsafe_world_cell.rs | 27 +++++++++++++++++++ 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 16a0ea6c9d3a0..aabc81776d869 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -8,7 +8,7 @@ use crate::{ QueryIter, QueryParIter, WorldQuery, }, storage::{TableId, TableRow}, - world::{World, WorldId}, + world::{unsafe_world_cell::AsUnsafeWorldCell, World, WorldId}, }; use bevy_tasks::ComputeTaskPool; #[cfg(feature = "trace")] @@ -146,7 +146,8 @@ impl QueryState { /// # Panics /// /// If `world` does not match the one used to call `QueryState::new` for this instance. - pub fn update_archetypes(&mut self, world: &World) { + pub fn update_archetypes<'w>(&mut self, world: impl AsUnsafeWorldCell<'w>) { + let world = world.as_metadata(); self.validate_world(world); let archetypes = world.archetypes(); let new_generation = archetypes.generation(); @@ -165,9 +166,9 @@ impl QueryState { /// Many unsafe query methods require the world to match for soundness. This function is the easiest /// way of ensuring that it matches. #[inline] - pub fn validate_world(&self, world: &World) { + pub fn validate_world<'w>(&self, world: impl AsUnsafeWorldCell<'w>) { assert!( - world.id() == self.world_id, + world.as_metadata().id() == self.world_id, "Attempted to use {} with a mismatched World. QueryStates can only be used with the World they were created from.", std::any::type_name::(), ); diff --git a/crates/bevy_ecs/src/world/unsafe_world_cell.rs b/crates/bevy_ecs/src/world/unsafe_world_cell.rs index 997862981ae99..e7f272df15a33 100644 --- a/crates/bevy_ecs/src/world/unsafe_world_cell.rs +++ b/crates/bevy_ecs/src/world/unsafe_world_cell.rs @@ -18,6 +18,33 @@ use crate::{ use bevy_ptr::Ptr; use std::{any::TypeId, cell::UnsafeCell, marker::PhantomData}; +/// Types that can be safely coerced into an [`UnsafeWorldCell`]. +pub trait AsUnsafeWorldCell<'w> { + /// Returns an [`UnsafeWorldCell`] that can be used to access world metadata. + fn as_metadata(self) -> UnsafeWorldCell<'w>; +} + +impl<'w> AsUnsafeWorldCell<'w> for UnsafeWorldCell<'w> { + #[inline] + fn as_metadata(self) -> UnsafeWorldCell<'w> { + self + } +} + +impl<'w> AsUnsafeWorldCell<'w> for &'w World { + #[inline] + fn as_metadata(self) -> UnsafeWorldCell<'w> { + self.as_unsafe_world_cell_readonly() + } +} + +impl<'w> AsUnsafeWorldCell<'w> for &'w mut World { + #[inline] + fn as_metadata(self) -> UnsafeWorldCell<'w> { + self.as_unsafe_world_cell_readonly() + } +} + /// Variant of the [`World`] where resource and component accesses take `&self`, and the responsibility to avoid /// aliasing violations are given to the caller instead of being checked at compile-time by rust's unique XOR shared rule. /// From f3b8babcecdaccf1552d3059545c470aad933a4d Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Sat, 10 Jun 2023 22:11:03 -0400 Subject: [PATCH 03/22] use UnsafeWorldCell in Query --- crates/bevy_ecs/src/system/mod.rs | 10 +++- crates/bevy_ecs/src/system/query.rs | 66 +++++++++++++--------- crates/bevy_ecs/src/system/system_param.rs | 14 ++--- 3 files changed, 51 insertions(+), 39 deletions(-) diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index 1bf773eb99fed..e7bd481509dc0 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -1724,7 +1724,15 @@ mod tests { let world2 = World::new(); let qstate = world1.query::<()>(); // SAFETY: doesnt access anything - let query = unsafe { Query::new(&world2, &qstate, Tick::new(0), Tick::new(0), false) }; + let query = unsafe { + Query::new( + world2.as_unsafe_world_cell_readonly(), + &qstate, + Tick::new(0), + Tick::new(0), + false, + ) + }; query.iter(); } diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 1e18705dcede0..c53a2b7668531 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -5,7 +5,7 @@ use crate::{ BatchingStrategy, QueryCombinationIter, QueryEntityError, QueryIter, QueryManyIter, QueryParIter, QuerySingleError, QueryState, ROQueryItem, ReadOnlyWorldQuery, WorldQuery, }, - world::{Mut, World}, + world::{unsafe_world_cell::UnsafeWorldCell, Mut}, }; use std::{any::TypeId, borrow::Borrow, fmt::Debug}; @@ -274,7 +274,8 @@ use std::{any::TypeId, borrow::Borrow, fmt::Debug}; /// [`With`]: crate::query::With /// [`Without`]: crate::query::Without pub struct Query<'world, 'state, Q: WorldQuery, F: ReadOnlyWorldQuery = ()> { - world: &'world World, + // SAFETY: Must have access to the components registered in `state`. + world: UnsafeWorldCell<'world>, state: &'state QueryState, last_run: Tick, this_run: Tick, @@ -288,7 +289,12 @@ pub struct Query<'world, 'state, Q: WorldQuery, F: ReadOnlyWorldQuery = ()> { impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> std::fmt::Debug for Query<'w, 's, Q, F> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "Query {{ matched entities: {}, world: {:?}, state: {:?}, last_run: {:?}, this_run: {:?} }}", self.iter().count(), self.world, self.state, self.last_run, self.this_run) + write!( + f, "Query {{ matched entities: {}, world: {:?}, state: {:?}, last_run: {:?}, this_run: {:?} }}", + self.iter().count(), + // SAFETY: World's Debug implementation only accesses metadata. + unsafe { self.world.world_metadata() }, + self.state, self.last_run, self.this_run) } } @@ -305,7 +311,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// called in ways that ensure the queries have unique mutable access. #[inline] pub(crate) unsafe fn new( - world: &'w World, + world: UnsafeWorldCell<'w>, state: &'s QueryState, last_run: Tick, this_run: Tick, @@ -369,8 +375,9 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// - [`for_each`](Self::for_each) for the closure based alternative. #[inline] pub fn iter(&self) -> QueryIter<'_, 's, Q::ReadOnly, F::ReadOnly> { - // SAFETY: system runs without conflicts with other systems. - // same-system queries have runtime borrow checks when they conflict + // SAFETY: + // - `self.world` has permission to access the required components. + // - The query is read-only, so it can be aliased even if it was originaly mutable. unsafe { self.state .as_readonly() @@ -404,8 +411,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// - [`for_each_mut`](Self::for_each_mut) for the closure based alternative. #[inline] pub fn iter_mut(&mut self) -> QueryIter<'_, 's, Q, F> { - // SAFETY: system runs without conflicts with other systems. - // same-system queries have runtime borrow checks when they conflict + // SAFETY: `self.world` has permission to access the required components. unsafe { self.state .iter_unchecked_manual(self.world, self.last_run, self.this_run) @@ -435,8 +441,9 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { pub fn iter_combinations( &self, ) -> QueryCombinationIter<'_, 's, Q::ReadOnly, F::ReadOnly, K> { - // SAFETY: system runs without conflicts with other systems. - // same-system queries have runtime borrow checks when they conflict + // SAFETY: + // - `self.world` has permission to access the required components. + // - The query is read-only, so it can be aliased even if it was originaly mutable. unsafe { self.state.as_readonly().iter_combinations_unchecked_manual( self.world, @@ -469,8 +476,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { pub fn iter_combinations_mut( &mut self, ) -> QueryCombinationIter<'_, 's, Q, F, K> { - // SAFETY: system runs without conflicts with other systems. - // same-system queries have runtime borrow checks when they conflict + // 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) @@ -521,8 +527,9 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { where EntityList::Item: Borrow, { - // SAFETY: system runs without conflicts with other systems. - // same-system queries have runtime borrow checks when they conflict + // SAFETY: + // - `self.world` has permission to access the required components. + // - The query is read-only, so it can be aliased even if it was originaly mutable. unsafe { self.state.as_readonly().iter_many_unchecked_manual( entities, @@ -574,8 +581,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { where EntityList::Item: Borrow, { - // SAFETY: system runs without conflicts with other systems. - // same-system queries have runtime borrow checks when they conflict + // SAFETY: `self.world` has permission to access the required components. unsafe { self.state.iter_many_unchecked_manual( entities, @@ -598,8 +604,9 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// - [`iter`](Self::iter) and [`iter_mut`](Self::iter_mut) for the safe versions. #[inline] pub unsafe fn iter_unsafe(&self) -> QueryIter<'_, 's, Q, F> { - // SEMI-SAFETY: system runs without conflicts with other systems. - // same-system queries have runtime borrow checks when they conflict + // SAFETY: + // - `self.world` has permission to access the required components. + // - The caller ensures that this operation will not result in any aliased mutable accesses. self.state .iter_unchecked_manual(self.world, self.last_run, self.this_run) } @@ -618,8 +625,9 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { pub unsafe fn iter_combinations_unsafe( &self, ) -> QueryCombinationIter<'_, 's, Q, F, K> { - // SEMI-SAFETY: system runs without conflicts with other systems. - // same-system queries have runtime borrow checks when they conflict + // SAFETY: + // - `self.world` has permission to access the required components. + // - The caller ensures that this operation will not result in any aliased mutable accesses. self.state .iter_combinations_unchecked_manual(self.world, self.last_run, self.this_run) } @@ -642,6 +650,9 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { where EntityList::Item: Borrow, { + // SAFETY: + // - `self.world` has permission to access the required components. + // - The caller ensures that this operation will not result in any aliased mutable accesses. self.state .iter_many_unchecked_manual(entities, self.world, self.last_run, self.this_run) } @@ -672,8 +683,9 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// - [`iter`](Self::iter) for the iterator based alternative. #[inline] pub fn for_each<'this>(&'this self, f: impl FnMut(ROQueryItem<'this, Q>)) { - // SAFETY: system runs without conflicts with other systems. - // same-system queries have runtime borrow checks when they conflict + // SAFETY: + // - `self.world` has permission to access the required components. + // - The query is read-only, so it can be aliased even if it was originaly mutable. unsafe { self.state.as_readonly().for_each_unchecked_manual( self.world, @@ -710,8 +722,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// - [`iter_mut`](Self::iter_mut) for the iterator based alternative. #[inline] pub fn for_each_mut<'a>(&'a mut self, f: impl FnMut(Q::Item<'a>)) { - // SAFETY: system runs without conflicts with other systems. same-system queries have runtime - // borrow checks when they conflict + // SAFETY: `self.world` has permission to access the required components. unsafe { self.state .for_each_unchecked_manual(self.world, f, self.last_run, self.this_run); @@ -1044,9 +1055,9 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { .archetype_component_access .has_read(archetype_component) { - entity_ref - .get::() - .ok_or(QueryComponentError::MissingComponent) + // SAFETY: `self.world` must have access to the component `T` for this entity, + // since it was registered in `self.state`'s archetype component access set. + unsafe { entity_ref.get::() }.ok_or(QueryComponentError::MissingComponent) } else { Err(QueryComponentError::MissingReadAccess) } @@ -1112,7 +1123,6 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { } let world = self.world; let entity_ref = world - .as_unsafe_world_cell_migration_internal() .get_entity(entity) .ok_or(QueryComponentError::NoSuchEntity)?; let component_id = world diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 023657bcd9e7f..4a3be817b8dbb 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -198,16 +198,10 @@ unsafe impl SystemPara world: UnsafeWorldCell<'w>, change_tick: Tick, ) -> Self::Item<'w, 's> { - Query::new( - // 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. - world.unsafe_world(), - state, - system_meta.last_run, - change_tick, - false, - ) + // 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. + Query::new(world, state, system_meta.last_run, change_tick, false) } } From 2ff5ad14c1563c802b2c94c47480984578bc5522 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Sat, 10 Jun 2023 22:28:24 -0400 Subject: [PATCH 04/22] port QueryIter --- crates/bevy_ecs/src/query/iter.rs | 51 ++++++++++++++++--------------- 1 file changed, 26 insertions(+), 25 deletions(-) diff --git a/crates/bevy_ecs/src/query/iter.rs b/crates/bevy_ecs/src/query/iter.rs index 893df47aac3a3..2663a83a8da3f 100644 --- a/crates/bevy_ecs/src/query/iter.rs +++ b/crates/bevy_ecs/src/query/iter.rs @@ -2,9 +2,9 @@ use crate::{ archetype::{ArchetypeEntity, ArchetypeId, Archetypes}, component::Tick, entity::{Entities, Entity}, - prelude::World, query::{ArchetypeFilter, DebugCheckedUnwrap, QueryState, WorldQuery}, storage::{TableId, TableRow, Tables}, + world::unsafe_world_cell::UnsafeWorldCell, }; use std::{borrow::Borrow, iter::FusedIterator, marker::PhantomData, mem::MaybeUninit}; @@ -23,20 +23,19 @@ pub struct QueryIter<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> { impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> QueryIter<'w, 's, Q, F> { /// # 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 `query_state.world_id`. Calling this on a `world` - /// with a mismatched [`WorldId`](crate::world::WorldId) is unsound. + /// - `world` must have permission to access any of the components registered in `query_state`. + /// - `world` must be the same one used to initialize `query_state`. pub(crate) unsafe fn new( - world: &'w World, + world: UnsafeWorldCell<'w>, query_state: &'s QueryState, last_run: Tick, this_run: Tick, ) -> Self { QueryIter { query_state, - tables: &world.storages().tables, - archetypes: &world.archetypes, + // SAFETY: We only access table data that has been registered in `query_state`. + tables: &world.unsafe_world().storages().tables, + archetypes: world.archetypes(), cursor: QueryIterationCursor::init(world, query_state, last_run, this_run), } } @@ -91,12 +90,10 @@ where I::Item: Borrow, { /// # 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 `query_state.world_id`. Calling this on a `world` - /// with a mismatched [`WorldId`](crate::world::WorldId) is unsound. + /// - `world` must have permission to access any of the components registered in `query_state`. + /// - `world` must be the same one used to initialize `query_state`. pub(crate) unsafe fn new>( - world: &'w World, + world: UnsafeWorldCell<'w>, query_state: &'s QueryState, entity_list: EntityList, last_run: Tick, @@ -106,9 +103,11 @@ where let filter = F::init_fetch(world, &query_state.filter_state, last_run, this_run); QueryManyIter { query_state, - entities: &world.entities, - archetypes: &world.archetypes, - tables: &world.storages.tables, + entities: world.entities(), + archetypes: world.archetypes(), + // SAFETY: We only access table data that has been registered in `query_state`. + // This means `world` has permission to access the data we use. + tables: &world.unsafe_world().storages.tables, fetch, filter, entity_iter: entity_list.into_iter(), @@ -282,12 +281,10 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery, const K: usize> QueryCombinationIter<'w, 's, Q, F, K> { /// # 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 `query_state.world_id`. Calling this on a - /// `world` with a mismatched [`WorldId`](crate::world::WorldId) is unsound. + /// - `world` must have permission to access any of the components registered in `query_state`. + /// - `world` must be the same one used to initialize `query_state`. pub(crate) unsafe fn new( - world: &'w World, + world: UnsafeWorldCell<'w>, query_state: &'s QueryState, last_run: Tick, this_run: Tick, @@ -318,8 +315,9 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery, const K: usize> QueryCombinationIter { query_state, - tables: &world.storages().tables, - archetypes: &world.archetypes, + // SAFETY: We only access table data that has been registered in `query_state`. + tables: &world.unsafe_world().storages().tables, + archetypes: world.archetypes(), cursors: array.assume_init(), } } @@ -485,7 +483,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> QueryIterationCursor<'w, 's, const IS_DENSE: bool = Q::IS_DENSE && F::IS_DENSE; unsafe fn init_empty( - world: &'w World, + world: UnsafeWorldCell<'w>, query_state: &'s QueryState, last_run: Tick, this_run: Tick, @@ -497,8 +495,11 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> QueryIterationCursor<'w, 's, } } + /// # Safety + /// - `world` must have permission to access any of the components registered in `query_state`. + /// - `world` must be the same one used to initialize `query_state`. unsafe fn init( - world: &'w World, + world: UnsafeWorldCell<'w>, query_state: &'s QueryState, last_run: Tick, this_run: Tick, From b8427c13cb2117d2a91dcd564e9e75704ce4cb46 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Sat, 10 Jun 2023 23:09:43 -0400 Subject: [PATCH 05/22] update comments for a doctest --- crates/bevy_ecs/src/query/state.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index aabc81776d869..28d7c0b411ddc 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -1267,7 +1267,7 @@ mod tests { // as it is shared and unsafe // We don't care about aliased mutability for the read-only equivalent - // SAFETY: mutable access is not checked, but we own the world and don't use the query results + // SAFETY: Query does not access world data. assert!(unsafe { query_state .get_many_unchecked_manual::<10>( @@ -1280,7 +1280,7 @@ mod tests { }); assert_eq!( - // SAFETY: mutable access is not checked, but we own the world and don't use the query results + // SAFETY: Query does not access world data. unsafe { query_state .get_many_unchecked_manual( @@ -1295,7 +1295,7 @@ mod tests { ); assert_eq!( - // SAFETY: mutable access is not checked, but we own the world and don't use the query results + // SAFETY: Query does not access world data. unsafe { query_state .get_many_unchecked_manual( @@ -1310,7 +1310,7 @@ mod tests { ); assert_eq!( - // SAFETY: mutable access is not checked, but we own the world and don't use the query results + // SAFETY: Query does not access world data. unsafe { query_state .get_many_unchecked_manual( From b5a5df2325407888d58e2a96d29c79c3aac9e463 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Sat, 10 Jun 2023 23:17:17 -0400 Subject: [PATCH 06/22] port QueryState to UnsafeWorldCell --- crates/bevy_ecs/src/query/state.rs | 160 +++++++++++++++++----------- crates/bevy_ecs/src/system/query.rs | 16 ++- 2 files changed, 110 insertions(+), 66 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 28d7c0b411ddc..2900cce32a447 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -8,7 +8,10 @@ use crate::{ QueryIter, QueryParIter, WorldQuery, }, storage::{TableId, TableRow}, - world::{unsafe_world_cell::AsUnsafeWorldCell, World, WorldId}, + world::{ + unsafe_world_cell::{AsUnsafeWorldCell, UnsafeWorldCell}, + World, WorldId, + }, }; use bevy_tasks::ComputeTaskPool; #[cfg(feature = "trace")] @@ -134,7 +137,7 @@ impl QueryState { // SAFETY: NopFetch does not access any members while &self ensures no one has exclusive access unsafe { self.as_nop() - .iter_unchecked_manual(world, last_run, this_run) + .iter_unchecked_manual(world.as_unsafe_world_cell_readonly(), last_run, this_run) .next() .is_none() } @@ -218,7 +221,7 @@ impl QueryState { // SAFETY: query is read only unsafe { self.as_readonly().get_unchecked_manual( - world, + world.as_unsafe_world_cell_readonly(), entity, world.last_change_tick(), world.read_change_tick(), @@ -287,7 +290,14 @@ impl QueryState { self.update_archetypes(world); let change_tick = world.change_tick(); // SAFETY: query has unique world access - unsafe { self.get_unchecked_manual(world, entity, world.last_change_tick(), change_tick) } + unsafe { + self.get_unchecked_manual( + world.as_unsafe_world_cell(), + entity, + world.last_change_tick(), + change_tick, + ) + } } /// Returns the query results for the given array of [`Entity`]. @@ -340,7 +350,12 @@ impl QueryState { // SAFETY: method requires exclusive world access // and world has been validated via update_archetypes unsafe { - self.get_many_unchecked_manual(world, entities, world.last_change_tick(), change_tick) + self.get_many_unchecked_manual( + world.as_unsafe_world_cell(), + entities, + world.last_change_tick(), + change_tick, + ) } } @@ -365,7 +380,7 @@ impl QueryState { // SAFETY: query is read only and world is validated unsafe { self.as_readonly().get_unchecked_manual( - world, + world.as_unsafe_world_cell_readonly(), entity, world.last_change_tick(), world.read_change_tick(), @@ -382,16 +397,11 @@ impl QueryState { #[inline] pub unsafe fn get_unchecked<'w>( &mut self, - world: &'w World, + world: UnsafeWorldCell<'w>, entity: Entity, ) -> Result, QueryEntityError> { self.update_archetypes(world); - self.get_unchecked_manual( - world, - entity, - world.last_change_tick(), - world.read_change_tick(), - ) + 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 @@ -406,13 +416,13 @@ impl QueryState { /// use `QueryState::validate_world` to verify this. pub(crate) unsafe fn get_unchecked_manual<'w>( &self, - world: &'w World, + world: UnsafeWorldCell<'w>, entity: Entity, last_run: Tick, this_run: Tick, ) -> Result, QueryEntityError> { let location = world - .entities + .entities() .get(entity) .ok_or(QueryEntityError::NoSuchEntity(entity))?; if !self @@ -422,13 +432,14 @@ impl QueryState { return Err(QueryEntityError::QueryDoesNotMatch(entity)); } let archetype = world - .archetypes + .archetypes() .get(location.archetype_id) .debug_checked_unwrap(); let mut fetch = Q::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 + .unsafe_world() .storages() .tables .get(location.table_id) @@ -462,9 +473,12 @@ impl QueryState { for (value, entity) in std::iter::zip(&mut values, entities) { // SAFETY: fetch is read-only // and world must be validated - let item = self - .as_readonly() - .get_unchecked_manual(world, entity, last_run, this_run)?; + let item = self.as_readonly().get_unchecked_manual( + world.as_unsafe_world_cell_readonly(), + entity, + last_run, + this_run, + )?; *value = MaybeUninit::new(item); } @@ -484,7 +498,7 @@ impl QueryState { /// use `QueryState::validate_world` to verify this. pub(crate) unsafe fn get_many_unchecked_manual<'w, const N: usize>( &self, - world: &'w World, + world: UnsafeWorldCell<'w>, entities: [Entity; N], last_run: Tick, this_run: Tick, @@ -521,7 +535,7 @@ impl QueryState { // SAFETY: query is read only unsafe { self.as_readonly().iter_unchecked_manual( - world, + world.as_unsafe_world_cell_readonly(), world.last_change_tick(), world.read_change_tick(), ) @@ -534,7 +548,13 @@ impl QueryState { let change_tick = world.change_tick(); self.update_archetypes(world); // SAFETY: query has unique world access - unsafe { self.iter_unchecked_manual(world, world.last_change_tick(), change_tick) } + unsafe { + self.iter_unchecked_manual( + world.as_unsafe_world_cell(), + world.last_change_tick(), + change_tick, + ) + } } /// Returns an [`Iterator`] over the query results for the given [`World`] without updating the query's archetypes. @@ -550,7 +570,7 @@ impl QueryState { // SAFETY: query is read only and world is validated unsafe { self.as_readonly().iter_unchecked_manual( - world, + world.as_unsafe_world_cell_readonly(), world.last_change_tick(), world.read_change_tick(), ) @@ -587,7 +607,7 @@ impl QueryState { // SAFETY: query is read only unsafe { self.as_readonly().iter_combinations_unchecked_manual( - world, + world.as_unsafe_world_cell_readonly(), world.last_change_tick(), world.read_change_tick(), ) @@ -620,7 +640,11 @@ impl QueryState { self.update_archetypes(world); // SAFETY: query has unique world access unsafe { - self.iter_combinations_unchecked_manual(world, world.last_change_tick(), change_tick) + self.iter_combinations_unchecked_manual( + world.as_unsafe_world_cell(), + world.last_change_tick(), + change_tick, + ) } } @@ -646,7 +670,7 @@ impl QueryState { unsafe { self.as_readonly().iter_many_unchecked_manual( entities, - world, + world.as_unsafe_world_cell_readonly(), world.last_change_tick(), world.read_change_tick(), ) @@ -681,7 +705,7 @@ impl QueryState { unsafe { self.as_readonly().iter_many_unchecked_manual( entities, - world, + world.as_unsafe_world_cell_readonly(), world.last_change_tick(), world.read_change_tick(), ) @@ -705,7 +729,12 @@ impl QueryState { let change_tick = world.change_tick(); // SAFETY: Query has unique world access. unsafe { - self.iter_many_unchecked_manual(entities, world, world.last_change_tick(), change_tick) + self.iter_many_unchecked_manual( + entities, + world.as_unsafe_world_cell(), + world.last_change_tick(), + change_tick, + ) } } @@ -718,10 +747,10 @@ impl QueryState { #[inline] pub unsafe fn iter_unchecked<'w, 's>( &'s mut self, - world: &'w World, + world: UnsafeWorldCell<'w>, ) -> QueryIter<'w, 's, Q, F> { self.update_archetypes(world); - self.iter_unchecked_manual(world, world.last_change_tick(), world.read_change_tick()) + self.iter_unchecked_manual(world, world.last_change_tick(), world.change_tick()) } /// Returns an [`Iterator`] over all possible combinations of `K` query results for the @@ -735,13 +764,13 @@ impl QueryState { #[inline] pub unsafe fn iter_combinations_unchecked<'w, 's, const K: usize>( &'s mut self, - world: &'w World, + world: UnsafeWorldCell<'w>, ) -> QueryCombinationIter<'w, 's, Q, F, K> { self.update_archetypes(world); self.iter_combinations_unchecked_manual( world, world.last_change_tick(), - world.read_change_tick(), + world.change_tick(), ) } @@ -757,7 +786,7 @@ impl QueryState { #[inline] pub(crate) unsafe fn iter_unchecked_manual<'w, 's>( &'s self, - world: &'w World, + world: UnsafeWorldCell<'w>, last_run: Tick, this_run: Tick, ) -> QueryIter<'w, 's, Q, F> { @@ -778,7 +807,7 @@ impl QueryState { pub(crate) unsafe fn iter_many_unchecked_manual<'w, 's, EntityList: IntoIterator>( &'s self, entities: EntityList, - world: &'w World, + world: UnsafeWorldCell<'w>, last_run: Tick, this_run: Tick, ) -> QueryManyIter<'w, 's, Q, F, EntityList::IntoIter> @@ -801,7 +830,7 @@ impl QueryState { #[inline] pub(crate) unsafe fn iter_combinations_unchecked_manual<'w, 's, const K: usize>( &'s self, - world: &'w World, + world: UnsafeWorldCell<'w>, last_run: Tick, this_run: Tick, ) -> QueryCombinationIter<'w, 's, Q, F, K> { @@ -818,7 +847,7 @@ impl QueryState { // SAFETY: query is read only unsafe { self.as_readonly().for_each_unchecked_manual( - world, + world.as_unsafe_world_cell_readonly(), func, world.last_change_tick(), world.read_change_tick(), @@ -834,7 +863,12 @@ impl QueryState { self.update_archetypes(world); // SAFETY: query has unique world access unsafe { - self.for_each_unchecked_manual(world, func, world.last_change_tick(), change_tick); + self.for_each_unchecked_manual( + world.as_unsafe_world_cell(), + func, + world.last_change_tick(), + change_tick, + ); } } @@ -848,16 +882,11 @@ impl QueryState { #[inline] pub unsafe fn for_each_unchecked<'w, FN: FnMut(Q::Item<'w>)>( &mut self, - world: &'w World, + world: UnsafeWorldCell<'w>, func: FN, ) { self.update_archetypes(world); - self.for_each_unchecked_manual( - world, - func, - world.last_change_tick(), - world.read_change_tick(), - ); + self.for_each_unchecked_manual(world, func, world.last_change_tick(), world.change_tick()); } /// Returns a parallel iterator over the query results for the given [`World`]. @@ -907,7 +936,7 @@ impl QueryState { /// with a mismatched [`WorldId`] is unsound. pub(crate) unsafe fn for_each_unchecked_manual<'w, FN: FnMut(Q::Item<'w>)>( &self, - world: &'w World, + world: UnsafeWorldCell<'w>, mut func: FN, last_run: Tick, this_run: Tick, @@ -917,7 +946,7 @@ impl QueryState { let mut fetch = Q::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 tables = &world.storages().tables; + let tables = &world.unsafe_world().storages().tables; if Q::IS_DENSE && F::IS_DENSE { for table_id in &self.matched_table_ids { let table = tables.get(*table_id).debug_checked_unwrap(); @@ -935,7 +964,7 @@ impl QueryState { } } } else { - let archetypes = &world.archetypes; + let archetypes = world.archetypes(); for archetype_id in &self.matched_archetype_ids { let archetype = archetypes.get(*archetype_id).debug_checked_unwrap(); let table = tables.get(archetype.table_id()).debug_checked_unwrap(); @@ -981,7 +1010,7 @@ impl QueryState { FN: Fn(Q::Item<'w>) + Send + Sync + Clone, >( &self, - world: &'w World, + world: UnsafeWorldCell<'w>, batch_size: usize, func: FN, last_run: Tick, @@ -991,7 +1020,8 @@ impl QueryState { // QueryIter, QueryIterationCursor, QueryManyIter, QueryCombinationIter, QueryState::for_each_unchecked_manual, QueryState::par_for_each_unchecked_manual ComputeTaskPool::get().scope(|scope| { if Q::IS_DENSE && F::IS_DENSE { - let tables = &world.storages().tables; + // SAFETY: We only access table data that has been registered in `self.archetype_component_access`. + let tables = &world.unsafe_world().storages().tables; for table_id in &self.matched_table_ids { let table = &tables[*table_id]; if table.is_empty() { @@ -1007,7 +1037,7 @@ impl QueryState { Q::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 tables = &world.storages().tables; + let tables = &world.unsafe_world().storages().tables; let table = tables.get(*table_id).debug_checked_unwrap(); let entities = table.entities(); Q::set_table(&mut fetch, &self.fetch_state, table); @@ -1035,7 +1065,7 @@ impl QueryState { } } } else { - let archetypes = &world.archetypes; + let archetypes = world.archetypes(); for archetype_id in &self.matched_archetype_ids { let mut offset = 0; let archetype = &archetypes[*archetype_id]; @@ -1051,9 +1081,9 @@ impl QueryState { Q::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 tables = &world.storages().tables; + let tables = &world.unsafe_world().storages().tables; let archetype = - world.archetypes.get(*archetype_id).debug_checked_unwrap(); + world.archetypes().get(*archetype_id).debug_checked_unwrap(); let table = tables.get(archetype.table_id()).debug_checked_unwrap(); Q::set_archetype(&mut fetch, &self.fetch_state, archetype, table); F::set_archetype(&mut filter, &self.filter_state, archetype, table); @@ -1128,7 +1158,7 @@ impl QueryState { // SAFETY: query is read only unsafe { self.as_readonly().get_single_unchecked_manual( - world, + world.as_unsafe_world_cell_readonly(), world.last_change_tick(), world.read_change_tick(), ) @@ -1163,7 +1193,13 @@ impl QueryState { let change_tick = world.change_tick(); // SAFETY: query has unique world access - unsafe { self.get_single_unchecked_manual(world, world.last_change_tick(), change_tick) } + unsafe { + self.get_single_unchecked_manual( + world.as_unsafe_world_cell(), + world.last_change_tick(), + change_tick, + ) + } } /// Returns a query result when there is exactly one entity matching the query. @@ -1178,10 +1214,10 @@ impl QueryState { #[inline] pub unsafe fn get_single_unchecked<'w>( &mut self, - world: &'w World, + world: UnsafeWorldCell<'w>, ) -> Result, QuerySingleError> { self.update_archetypes(world); - self.get_single_unchecked_manual(world, world.last_change_tick(), world.read_change_tick()) + self.get_single_unchecked_manual(world, world.last_change_tick(), world.change_tick()) } /// Returns a query result when there is exactly one entity matching the query, @@ -1197,7 +1233,7 @@ impl QueryState { #[inline] pub unsafe fn get_single_unchecked_manual<'w>( &self, - world: &'w World, + world: UnsafeWorldCell<'w>, last_run: Tick, this_run: Tick, ) -> Result, QuerySingleError> { @@ -1271,7 +1307,7 @@ mod tests { assert!(unsafe { query_state .get_many_unchecked_manual::<10>( - &world, + world.as_unsafe_world_cell_readonly(), entities.clone().try_into().unwrap(), last_change_tick, change_tick, @@ -1284,7 +1320,7 @@ mod tests { unsafe { query_state .get_many_unchecked_manual( - &world, + world.as_unsafe_world_cell_readonly(), [entities[0], entities[0]], last_change_tick, change_tick, @@ -1299,7 +1335,7 @@ mod tests { unsafe { query_state .get_many_unchecked_manual( - &world, + world.as_unsafe_world_cell_readonly(), [entities[0], entities[1], entities[0]], last_change_tick, change_tick, @@ -1314,7 +1350,7 @@ mod tests { unsafe { query_state .get_many_unchecked_manual( - &world, + world.as_unsafe_world_cell_readonly(), [entities[9], entities[9]], last_change_tick, change_tick, diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index c53a2b7668531..ff8848b809fdf 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -823,8 +823,12 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { ) -> Result<[ROQueryItem<'_, Q>; N], QueryEntityError> { // SAFETY: it is the scheduler's responsibility to ensure that `Query` is never handed out on the wrong `World`. unsafe { - self.state - .get_many_read_only_manual(self.world, entities, self.last_run, self.this_run) + self.state.get_many_read_only_manual( + self.world.unsafe_world(), + entities, + self.last_run, + self.this_run, + ) } } @@ -1311,8 +1315,12 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// ``` #[inline] pub fn is_empty(&self) -> bool { - self.state - .is_empty(self.world, self.last_run, self.this_run) + self.state.is_empty( + // SAFETY: `QueryState::is_empty` does not access world data. + unsafe { self.world.unsafe_world() }, + self.last_run, + self.this_run, + ) } /// Returns `true` if the given [`Entity`] matches the query. From 3f8694986272bd7f8ad885f567de52c57edd8a92 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Mon, 12 Jun 2023 18:15:49 -0400 Subject: [PATCH 07/22] port par_iter --- crates/bevy_ecs/src/query/par_iter.rs | 7 ++++--- crates/bevy_ecs/src/query/state.rs | 4 ++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/crates/bevy_ecs/src/query/par_iter.rs b/crates/bevy_ecs/src/query/par_iter.rs index 77353bfb11fd0..ea54fa1aa4242 100644 --- a/crates/bevy_ecs/src/query/par_iter.rs +++ b/crates/bevy_ecs/src/query/par_iter.rs @@ -1,4 +1,4 @@ -use crate::{component::Tick, world::World}; +use crate::{component::Tick, world::unsafe_world_cell::UnsafeWorldCell}; use bevy_tasks::ComputeTaskPool; use std::ops::Range; @@ -82,7 +82,7 @@ impl BatchingStrategy { /// This struct is created by the [`Query::par_iter`](crate::system::Query::par_iter) and /// [`Query::par_iter_mut`](crate::system::Query::par_iter_mut) methods. pub struct QueryParIter<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> { - pub(crate) world: &'w World, + pub(crate) world: UnsafeWorldCell<'w>, pub(crate) state: &'s QueryState, pub(crate) last_run: Tick, pub(crate) this_run: Tick, @@ -178,7 +178,8 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> QueryParIter<'w, 's, Q, F> { "Attempted to run parallel iteration over a query with an empty TaskPool" ); let max_size = if Q::IS_DENSE && F::IS_DENSE { - let tables = &self.world.storages().tables; + // SAFETY: We only access table metadata. + let tables = unsafe { &self.world.world_metadata().storages().tables }; self.state .matched_table_ids .iter() diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 2900cce32a447..e7a5c16d6896f 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -898,7 +898,7 @@ impl QueryState { pub fn par_iter<'w, 's>(&'s mut self, world: &'w World) -> QueryParIter<'w, 's, Q, F> { self.update_archetypes(world); QueryParIter { - world, + world: world.as_unsafe_world_cell_readonly(), state: self, last_run: world.last_change_tick(), this_run: world.read_change_tick(), @@ -916,7 +916,7 @@ impl QueryState { self.update_archetypes(world); let this_run = world.change_tick(); QueryParIter { - world, + world: world.as_unsafe_world_cell(), state: self, last_run: world.last_change_tick(), this_run, From 79481f183f5fb94f994b06e79ea1d36187466a8b Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Mon, 12 Jun 2023 18:44:43 -0400 Subject: [PATCH 08/22] don't implement `AsUnsafeWorldCell` for `&mut World` --- crates/bevy_ecs/src/query/state.rs | 18 +++++++++--------- crates/bevy_ecs/src/world/unsafe_world_cell.rs | 10 ++++------ 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index e7a5c16d6896f..87a237b4fdd8b 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -127,7 +127,7 @@ impl QueryState { matched_archetypes: Default::default(), archetype_component_access: Default::default(), }; - state.update_archetypes(world); + state.update_archetypes(&*world); state } @@ -287,7 +287,7 @@ impl QueryState { world: &'w mut World, entity: Entity, ) -> Result, QueryEntityError> { - self.update_archetypes(world); + self.update_archetypes(&*world); let change_tick = world.change_tick(); // SAFETY: query has unique world access unsafe { @@ -344,7 +344,7 @@ impl QueryState { world: &'w mut World, entities: [Entity; N], ) -> Result<[Q::Item<'w>; N], QueryEntityError> { - self.update_archetypes(world); + self.update_archetypes(&*world); let change_tick = world.change_tick(); // SAFETY: method requires exclusive world access @@ -546,7 +546,7 @@ impl QueryState { #[inline] pub fn iter_mut<'w, 's>(&'s mut self, world: &'w mut World) -> QueryIter<'w, 's, Q, F> { let change_tick = world.change_tick(); - self.update_archetypes(world); + self.update_archetypes(&*world); // SAFETY: query has unique world access unsafe { self.iter_unchecked_manual( @@ -637,7 +637,7 @@ impl QueryState { world: &'w mut World, ) -> QueryCombinationIter<'w, 's, Q, F, K> { let change_tick = world.change_tick(); - self.update_archetypes(world); + self.update_archetypes(&*world); // SAFETY: query has unique world access unsafe { self.iter_combinations_unchecked_manual( @@ -725,7 +725,7 @@ impl QueryState { where EntityList::Item: Borrow, { - self.update_archetypes(world); + self.update_archetypes(&*world); let change_tick = world.change_tick(); // SAFETY: Query has unique world access. unsafe { @@ -860,7 +860,7 @@ impl QueryState { #[inline] pub fn for_each_mut<'w, FN: FnMut(Q::Item<'w>)>(&mut self, world: &'w mut World, func: FN) { let change_tick = world.change_tick(); - self.update_archetypes(world); + self.update_archetypes(&*world); // SAFETY: query has unique world access unsafe { self.for_each_unchecked_manual( @@ -913,7 +913,7 @@ impl QueryState { /// [`par_iter`]: Self::par_iter #[inline] pub fn par_iter_mut<'w, 's>(&'s mut self, world: &'w mut World) -> QueryParIter<'w, 's, Q, F> { - self.update_archetypes(world); + self.update_archetypes(&*world); let this_run = world.change_tick(); QueryParIter { world: world.as_unsafe_world_cell(), @@ -1189,7 +1189,7 @@ impl QueryState { &mut self, world: &'w mut World, ) -> Result, QuerySingleError> { - self.update_archetypes(world); + self.update_archetypes(&*world); let change_tick = world.change_tick(); // SAFETY: query has unique world access diff --git a/crates/bevy_ecs/src/world/unsafe_world_cell.rs b/crates/bevy_ecs/src/world/unsafe_world_cell.rs index e7f272df15a33..093109f6c4ae7 100644 --- a/crates/bevy_ecs/src/world/unsafe_world_cell.rs +++ b/crates/bevy_ecs/src/world/unsafe_world_cell.rs @@ -38,12 +38,10 @@ impl<'w> AsUnsafeWorldCell<'w> for &'w World { } } -impl<'w> AsUnsafeWorldCell<'w> for &'w mut World { - #[inline] - fn as_metadata(self) -> UnsafeWorldCell<'w> { - self.as_unsafe_world_cell_readonly() - } -} +// Note: We intentionally do not implement this for `&mut World`. +// Since reborrowing does not work for `impl AsUnsafeWorldCell` parameters, +// implementing this trait would cause mutable world references to be moved, +// which is an ergonomic footgun. /// Variant of the [`World`] where resource and component accesses take `&self`, and the responsibility to avoid /// aliasing violations are given to the caller instead of being checked at compile-time by rust's unique XOR shared rule. From 9a8c13bdf2b0be8f14d0422c4c4631281d792d75 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Mon, 12 Jun 2023 18:48:18 -0400 Subject: [PATCH 09/22] fix some borrow checker errors --- crates/bevy_ecs/src/query/state.rs | 34 +++++++++++++++++------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 87a237b4fdd8b..a5b09517899b8 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -289,12 +289,13 @@ impl QueryState { ) -> Result, QueryEntityError> { 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, - world.last_change_tick(), + last_change_tick, change_tick, ) } @@ -347,13 +348,14 @@ impl QueryState { 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, - world.last_change_tick(), + last_change_tick, change_tick, ) } @@ -545,15 +547,12 @@ impl QueryState { /// Returns an [`Iterator`] over the query results for the given [`World`]. #[inline] pub fn iter_mut<'w, 's>(&'s mut self, world: &'w mut World) -> QueryIter<'w, 's, Q, F> { - let change_tick = world.change_tick(); 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(), - world.last_change_tick(), - change_tick, - ) + self.iter_unchecked_manual(world.as_unsafe_world_cell(), last_change_tick, change_tick) } } @@ -636,13 +635,14 @@ impl QueryState { &'s mut self, world: &'w mut World, ) -> QueryCombinationIter<'w, 's, Q, F, K> { - let change_tick = world.change_tick(); 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(), - world.last_change_tick(), + last_change_tick, change_tick, ) } @@ -727,12 +727,13 @@ impl QueryState { { 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(), - world.last_change_tick(), + last_change_tick, change_tick, ) } @@ -859,14 +860,15 @@ impl QueryState { /// `iter_mut()` method, but cannot be chained like a normal [`Iterator`]. #[inline] pub fn for_each_mut<'w, FN: FnMut(Q::Item<'w>)>(&mut self, world: &'w mut World, func: FN) { - let change_tick = world.change_tick(); 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.for_each_unchecked_manual( world.as_unsafe_world_cell(), func, - world.last_change_tick(), + last_change_tick, change_tick, ); } @@ -915,10 +917,11 @@ impl QueryState { pub fn par_iter_mut<'w, 's>(&'s mut self, world: &'w mut World) -> QueryParIter<'w, 's, Q, 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: world.last_change_tick(), + last_run, this_run, batching_strategy: BatchingStrategy::new(), } @@ -1192,11 +1195,12 @@ impl QueryState { 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(), - world.last_change_tick(), + last_change_tick, change_tick, ) } From a3b02d6614da244790d008a2406dc1fcc95faa61 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Mon, 12 Jun 2023 18:48:24 -0400 Subject: [PATCH 10/22] fix a clippy lint --- crates/bevy_ecs/src/world/unsafe_world_cell.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/bevy_ecs/src/world/unsafe_world_cell.rs b/crates/bevy_ecs/src/world/unsafe_world_cell.rs index 093109f6c4ae7..69e15202b5938 100644 --- a/crates/bevy_ecs/src/world/unsafe_world_cell.rs +++ b/crates/bevy_ecs/src/world/unsafe_world_cell.rs @@ -21,6 +21,7 @@ use std::{any::TypeId, cell::UnsafeCell, marker::PhantomData}; /// Types that can be safely coerced into an [`UnsafeWorldCell`]. pub trait AsUnsafeWorldCell<'w> { /// Returns an [`UnsafeWorldCell`] that can be used to access world metadata. + #[allow(clippy::wrong_self_convention)] fn as_metadata(self) -> UnsafeWorldCell<'w>; } From 52f71a091dc5b85e44646b1322f8f03e9b778a41 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Mon, 12 Jun 2023 18:51:57 -0400 Subject: [PATCH 11/22] port `SystemState` --- crates/bevy_ecs/src/system/function_system.rs | 22 ++++++++----------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 1e4647389a97e..e1df722da835e 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -186,7 +186,7 @@ impl SystemState { self.validate_world(world); self.update_archetypes(world); // SAFETY: Param is read-only and doesn't allow mutable access to World. It also matches the World this SystemState was created with. - unsafe { self.get_unchecked_manual(world) } + unsafe { self.get_unchecked_manual(world.as_unsafe_world_cell_readonly()) } } /// Retrieve the mutable [`SystemParam`] values. @@ -195,7 +195,7 @@ impl SystemState { self.validate_world(world); self.update_archetypes(world); // SAFETY: World is uniquely borrowed and matches the World this SystemState was created with. - unsafe { self.get_unchecked_manual(world) } + unsafe { self.get_unchecked_manual(world.as_unsafe_world_cell()) } } /// Applies all state queued up for [`SystemParam`] values. For example, this will apply commands queued up @@ -256,8 +256,9 @@ impl SystemState { { self.validate_world(world); let change_tick = world.read_change_tick(); - // SAFETY: Param is read-only and doesn't allow mutable access to World. It also matches the World this SystemState was created with. - unsafe { self.fetch(world, change_tick) } + // SAFETY: Param is read-only and doesn't allow mutable access to World. + // It also matches the World this SystemState was created with. + unsafe { self.fetch(world.as_unsafe_world_cell_readonly(), change_tick) } } /// Retrieve the mutable [`SystemParam`] values. This will not update the state's view of the world's archetypes @@ -275,7 +276,7 @@ impl SystemState { self.validate_world(world); let change_tick = world.change_tick(); // SAFETY: World is uniquely borrowed and matches the World this SystemState was created with. - unsafe { self.fetch(world, change_tick) } + unsafe { self.fetch(world.as_unsafe_world_cell(), change_tick) } } /// Retrieve the [`SystemParam`] values. This will not update archetypes automatically. @@ -287,7 +288,7 @@ impl SystemState { #[inline] pub unsafe fn get_unchecked_manual<'w, 's>( &'s mut self, - world: &'w World, + world: UnsafeWorldCell<'w>, ) -> SystemParamItem<'w, 's, Param> { let change_tick = world.increment_change_tick(); self.fetch(world, change_tick) @@ -300,15 +301,10 @@ impl SystemState { #[inline] unsafe fn fetch<'w, 's>( &'s mut self, - world: &'w World, + world: UnsafeWorldCell<'w>, change_tick: Tick, ) -> SystemParamItem<'w, 's, Param> { - let param = Param::get_param( - &mut self.param_state, - &self.meta, - world.as_unsafe_world_cell_migration_internal(), - change_tick, - ); + let param = Param::get_param(&mut self.param_state, &self.meta, world, change_tick); self.meta.last_run = change_tick; param } From 1241727e2bda6ea0d3b66e59c307d22ce893674d Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Mon, 12 Jun 2023 18:52:14 -0400 Subject: [PATCH 12/22] remove `as_unsafe_world_cell_migration_internal` --- crates/bevy_ecs/src/world/mod.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 1e4598e1d72c0..0a2b4011ab3de 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -122,14 +122,6 @@ impl World { UnsafeWorldCell::new_readonly(self) } - /// Creates a new [`UnsafeWorldCell`] with read+write access from a [&World](World). - /// This is only a temporary measure until every `&World` that is semantically a [`UnsafeWorldCell`] - /// has been replaced. - #[inline] - pub(crate) fn as_unsafe_world_cell_migration_internal(&self) -> UnsafeWorldCell<'_> { - UnsafeWorldCell::new_readonly(self) - } - /// Retrieves this world's [Entities] collection #[inline] pub fn entities(&self) -> &Entities { From c7233c946455cc30af7927b8b608c2ef5d78178a Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Mon, 12 Jun 2023 19:33:04 -0400 Subject: [PATCH 13/22] remove `AsUnsafeWorldCell` --- crates/bevy_ecs/src/query/state.rs | 56 +++++++++---------- crates/bevy_ecs/src/system/query.rs | 2 +- .../bevy_ecs/src/world/unsafe_world_cell.rs | 21 ------- 3 files changed, 27 insertions(+), 52 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index a5b09517899b8..4dcf82aeb6201 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -8,10 +8,7 @@ use crate::{ QueryIter, QueryParIter, WorldQuery, }, storage::{TableId, TableRow}, - world::{ - unsafe_world_cell::{AsUnsafeWorldCell, UnsafeWorldCell}, - World, WorldId, - }, + world::{unsafe_world_cell::UnsafeWorldCell, World, WorldId}, }; use bevy_tasks::ComputeTaskPool; #[cfg(feature = "trace")] @@ -127,7 +124,7 @@ impl QueryState { matched_archetypes: Default::default(), archetype_component_access: Default::default(), }; - state.update_archetypes(&*world); + state.update_archetypes(world.as_unsafe_world_cell()); state } @@ -149,9 +146,8 @@ impl QueryState { /// # Panics /// /// If `world` does not match the one used to call `QueryState::new` for this instance. - pub fn update_archetypes<'w>(&mut self, world: impl AsUnsafeWorldCell<'w>) { - let world = world.as_metadata(); - self.validate_world(world); + pub fn update_archetypes(&mut self, world: UnsafeWorldCell) { + self.validate_world(world.id()); let archetypes = world.archetypes(); let new_generation = archetypes.generation(); let old_generation = std::mem::replace(&mut self.archetype_generation, new_generation); @@ -164,14 +160,14 @@ impl QueryState { /// # Panics /// - /// If `world` does not match the one used to call `QueryState::new` for this instance. + /// If `world_id` does not match the [`World`] used to call `QueryState::new` for this instance. /// /// Many unsafe query methods require the world to match for soundness. This function is the easiest /// way of ensuring that it matches. #[inline] - pub fn validate_world<'w>(&self, world: impl AsUnsafeWorldCell<'w>) { + pub fn validate_world(&self, world_id: WorldId) { assert!( - world.as_metadata().id() == self.world_id, + world_id == self.world_id, "Attempted to use {} with a mismatched World. QueryStates can only be used with the World they were created from.", std::any::type_name::(), ); @@ -217,7 +213,7 @@ impl QueryState { world: &'w World, entity: Entity, ) -> Result, QueryEntityError> { - self.update_archetypes(world); + self.update_archetypes(world.as_unsafe_world_cell_readonly()); // SAFETY: query is read only unsafe { self.as_readonly().get_unchecked_manual( @@ -267,7 +263,7 @@ impl QueryState { world: &'w World, entities: [Entity; N], ) -> Result<[ROQueryItem<'w, Q>; N], QueryEntityError> { - self.update_archetypes(world); + self.update_archetypes(world.as_unsafe_world_cell_readonly()); // SAFETY: update_archetypes validates the `World` matches unsafe { @@ -287,7 +283,7 @@ impl QueryState { world: &'w mut World, entity: Entity, ) -> Result, QueryEntityError> { - self.update_archetypes(&*world); + self.update_archetypes(world.as_unsafe_world_cell()); let change_tick = world.change_tick(); let last_change_tick = world.last_change_tick(); // SAFETY: query has unique world access @@ -345,7 +341,7 @@ impl QueryState { world: &'w mut World, entities: [Entity; N], ) -> Result<[Q::Item<'w>; N], QueryEntityError> { - self.update_archetypes(&*world); + self.update_archetypes(world.as_unsafe_world_cell()); let change_tick = world.change_tick(); let last_change_tick = world.last_change_tick(); @@ -378,7 +374,7 @@ impl QueryState { world: &'w World, entity: Entity, ) -> Result, QueryEntityError> { - self.validate_world(world); + self.validate_world(world.id()); // SAFETY: query is read only and world is validated unsafe { self.as_readonly().get_unchecked_manual( @@ -533,7 +529,7 @@ impl QueryState { &'s mut self, world: &'w World, ) -> QueryIter<'w, 's, Q::ReadOnly, F::ReadOnly> { - self.update_archetypes(world); + self.update_archetypes(world.as_unsafe_world_cell_readonly()); // SAFETY: query is read only unsafe { self.as_readonly().iter_unchecked_manual( @@ -547,7 +543,7 @@ impl QueryState { /// Returns an [`Iterator`] over the query results for the given [`World`]. #[inline] pub fn iter_mut<'w, 's>(&'s mut self, world: &'w mut World) -> QueryIter<'w, 's, Q, F> { - self.update_archetypes(&*world); + self.update_archetypes(world.as_unsafe_world_cell()); let change_tick = world.change_tick(); let last_change_tick = world.last_change_tick(); // SAFETY: query has unique world access @@ -565,7 +561,7 @@ impl QueryState { &'s self, world: &'w World, ) -> QueryIter<'w, 's, Q::ReadOnly, F::ReadOnly> { - self.validate_world(world); + self.validate_world(world.id()); // SAFETY: query is read only and world is validated unsafe { self.as_readonly().iter_unchecked_manual( @@ -602,7 +598,7 @@ impl QueryState { &'s mut self, world: &'w World, ) -> QueryCombinationIter<'w, 's, Q::ReadOnly, F::ReadOnly, K> { - self.update_archetypes(world); + self.update_archetypes(world.as_unsafe_world_cell_readonly()); // SAFETY: query is read only unsafe { self.as_readonly().iter_combinations_unchecked_manual( @@ -635,7 +631,7 @@ impl QueryState { &'s mut self, world: &'w mut World, ) -> QueryCombinationIter<'w, 's, Q, F, K> { - self.update_archetypes(&*world); + self.update_archetypes(world.as_unsafe_world_cell()); let change_tick = world.change_tick(); let last_change_tick = world.last_change_tick(); // SAFETY: query has unique world access @@ -665,7 +661,7 @@ impl QueryState { where EntityList::Item: Borrow, { - self.update_archetypes(world); + self.update_archetypes(world.as_unsafe_world_cell_readonly()); // SAFETY: query is read only unsafe { self.as_readonly().iter_many_unchecked_manual( @@ -700,7 +696,7 @@ impl QueryState { where EntityList::Item: Borrow, { - self.validate_world(world); + self.validate_world(world.id()); // SAFETY: query is read only, world id is validated unsafe { self.as_readonly().iter_many_unchecked_manual( @@ -725,7 +721,7 @@ impl QueryState { where EntityList::Item: Borrow, { - self.update_archetypes(&*world); + self.update_archetypes(world.as_unsafe_world_cell()); let change_tick = world.change_tick(); let last_change_tick = world.last_change_tick(); // SAFETY: Query has unique world access. @@ -844,7 +840,7 @@ impl QueryState { /// This can only be called for read-only queries, see [`Self::for_each_mut`] for write-queries. #[inline] pub fn for_each<'w, FN: FnMut(ROQueryItem<'w, Q>)>(&mut self, world: &'w World, func: FN) { - self.update_archetypes(world); + self.update_archetypes(world.as_unsafe_world_cell_readonly()); // SAFETY: query is read only unsafe { self.as_readonly().for_each_unchecked_manual( @@ -860,7 +856,7 @@ impl QueryState { /// `iter_mut()` method, but cannot be chained like a normal [`Iterator`]. #[inline] pub fn for_each_mut<'w, FN: FnMut(Q::Item<'w>)>(&mut self, world: &'w mut World, func: FN) { - self.update_archetypes(&*world); + self.update_archetypes(world.as_unsafe_world_cell()); let change_tick = world.change_tick(); let last_change_tick = world.last_change_tick(); // SAFETY: query has unique world access @@ -898,7 +894,7 @@ impl QueryState { /// [`par_iter_mut`]: Self::par_iter_mut #[inline] pub fn par_iter<'w, 's>(&'s mut self, world: &'w World) -> QueryParIter<'w, 's, Q, F> { - self.update_archetypes(world); + self.update_archetypes(world.as_unsafe_world_cell_readonly()); QueryParIter { world: world.as_unsafe_world_cell_readonly(), state: self, @@ -915,7 +911,7 @@ impl QueryState { /// [`par_iter`]: Self::par_iter #[inline] pub fn par_iter_mut<'w, 's>(&'s mut self, world: &'w mut World) -> QueryParIter<'w, 's, Q, F> { - self.update_archetypes(&*world); + self.update_archetypes(world.as_unsafe_world_cell()); let this_run = world.change_tick(); let last_run = world.last_change_tick(); QueryParIter { @@ -1156,7 +1152,7 @@ impl QueryState { &mut self, world: &'w World, ) -> Result, QuerySingleError> { - self.update_archetypes(world); + self.update_archetypes(world.as_unsafe_world_cell_readonly()); // SAFETY: query is read only unsafe { @@ -1192,7 +1188,7 @@ impl QueryState { &mut self, world: &'w mut World, ) -> Result, QuerySingleError> { - self.update_archetypes(&*world); + self.update_archetypes(world.as_unsafe_world_cell()); let change_tick = world.change_tick(); let last_change_tick = world.last_change_tick(); diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index ff8848b809fdf..f47d7ae6e9830 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -317,7 +317,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { this_run: Tick, force_read_only_component_access: bool, ) -> Self { - state.validate_world(world); + state.validate_world(world.id()); Self { force_read_only_component_access, diff --git a/crates/bevy_ecs/src/world/unsafe_world_cell.rs b/crates/bevy_ecs/src/world/unsafe_world_cell.rs index 69e15202b5938..1f54eaabbf2d7 100644 --- a/crates/bevy_ecs/src/world/unsafe_world_cell.rs +++ b/crates/bevy_ecs/src/world/unsafe_world_cell.rs @@ -18,27 +18,6 @@ use crate::{ use bevy_ptr::Ptr; use std::{any::TypeId, cell::UnsafeCell, marker::PhantomData}; -/// Types that can be safely coerced into an [`UnsafeWorldCell`]. -pub trait AsUnsafeWorldCell<'w> { - /// Returns an [`UnsafeWorldCell`] that can be used to access world metadata. - #[allow(clippy::wrong_self_convention)] - fn as_metadata(self) -> UnsafeWorldCell<'w>; -} - -impl<'w> AsUnsafeWorldCell<'w> for UnsafeWorldCell<'w> { - #[inline] - fn as_metadata(self) -> UnsafeWorldCell<'w> { - self - } -} - -impl<'w> AsUnsafeWorldCell<'w> for &'w World { - #[inline] - fn as_metadata(self) -> UnsafeWorldCell<'w> { - self.as_unsafe_world_cell_readonly() - } -} - // Note: We intentionally do not implement this for `&mut World`. // Since reborrowing does not work for `impl AsUnsafeWorldCell` parameters, // implementing this trait would cause mutable world references to be moved, From 05a1f235336a3560508fc20c24dce10e03a8a00e Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Mon, 12 Jun 2023 19:40:32 -0400 Subject: [PATCH 14/22] update rendering crates --- .../src/contrast_adaptive_sharpening/node.rs | 2 +- .../bevy_core_pipeline/src/core_2d/main_pass_2d_node.rs | 2 +- crates/bevy_core_pipeline/src/msaa_writeback.rs | 2 +- crates/bevy_ecs/src/system/function_system.rs | 6 +++--- crates/bevy_pbr/src/render/light.rs | 6 ++++-- crates/bevy_render/src/camera/camera_driver_node.rs | 2 +- crates/bevy_render/src/render_graph/node.rs | 3 ++- crates/bevy_render/src/render_phase/draw.rs | 9 ++++++--- crates/bevy_ui/src/render/render_pass.rs | 6 ++++-- 9 files changed, 23 insertions(+), 15 deletions(-) diff --git a/crates/bevy_core_pipeline/src/contrast_adaptive_sharpening/node.rs b/crates/bevy_core_pipeline/src/contrast_adaptive_sharpening/node.rs index c49d876f2214d..451c9d37c7ac0 100644 --- a/crates/bevy_core_pipeline/src/contrast_adaptive_sharpening/node.rs +++ b/crates/bevy_core_pipeline/src/contrast_adaptive_sharpening/node.rs @@ -39,7 +39,7 @@ impl FromWorld for CASNode { impl Node for CASNode { fn update(&mut self, world: &mut World) { - self.query.update_archetypes(world); + self.query.update_archetypes(world.as_unsafe_world_cell()); } fn run( diff --git a/crates/bevy_core_pipeline/src/core_2d/main_pass_2d_node.rs b/crates/bevy_core_pipeline/src/core_2d/main_pass_2d_node.rs index 708193524b70a..506d74f928272 100644 --- a/crates/bevy_core_pipeline/src/core_2d/main_pass_2d_node.rs +++ b/crates/bevy_core_pipeline/src/core_2d/main_pass_2d_node.rs @@ -36,7 +36,7 @@ impl FromWorld for MainPass2dNode { impl Node for MainPass2dNode { fn update(&mut self, world: &mut World) { - self.query.update_archetypes(world); + self.query.update_archetypes(world.as_unsafe_world_cell()); } fn run( diff --git a/crates/bevy_core_pipeline/src/msaa_writeback.rs b/crates/bevy_core_pipeline/src/msaa_writeback.rs index cf881b07b608e..f610e3eb9cfe3 100644 --- a/crates/bevy_core_pipeline/src/msaa_writeback.rs +++ b/crates/bevy_core_pipeline/src/msaa_writeback.rs @@ -57,7 +57,7 @@ impl FromWorld for MsaaWritebackNode { impl Node for MsaaWritebackNode { fn update(&mut self, world: &mut World) { - self.cameras.update_archetypes(world); + self.cameras.update_archetypes(world.as_unsafe_world_cell()); } fn run( &self, diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index e1df722da835e..82dfbd8e61ee2 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -184,7 +184,7 @@ impl SystemState { Param: ReadOnlySystemParam, { self.validate_world(world); - self.update_archetypes(world); + self.update_archetypes(world.as_unsafe_world_cell_readonly()); // SAFETY: Param is read-only and doesn't allow mutable access to World. It also matches the World this SystemState was created with. unsafe { self.get_unchecked_manual(world.as_unsafe_world_cell_readonly()) } } @@ -193,7 +193,7 @@ impl SystemState { #[inline] pub fn get_mut<'w, 's>(&'s mut self, world: &'w mut World) -> SystemParamItem<'w, 's, Param> { self.validate_world(world); - self.update_archetypes(world); + self.update_archetypes(world.as_unsafe_world_cell_readonly()); // SAFETY: World is uniquely borrowed and matches the World this SystemState was created with. unsafe { self.get_unchecked_manual(world.as_unsafe_world_cell()) } } @@ -226,7 +226,7 @@ impl SystemState { /// be called if the `world` has been structurally mutated (i.e. added/removed a component or resource). Users using /// [`SystemState::get`] or [`SystemState::get_mut`] do not need to call this as it will be automatically called for them. #[inline] - pub fn update_archetypes(&mut self, world: &World) { + pub fn update_archetypes(&mut self, world: UnsafeWorldCell) { let archetypes = world.archetypes(); let new_generation = archetypes.generation(); let old_generation = std::mem::replace(&mut self.archetype_generation, new_generation); diff --git a/crates/bevy_pbr/src/render/light.rs b/crates/bevy_pbr/src/render/light.rs index c9308982088d9..d7a1ef56c41df 100644 --- a/crates/bevy_pbr/src/render/light.rs +++ b/crates/bevy_pbr/src/render/light.rs @@ -1704,8 +1704,10 @@ impl ShadowPassNode { impl Node for ShadowPassNode { fn update(&mut self, world: &mut World) { - self.main_view_query.update_archetypes(world); - self.view_light_query.update_archetypes(world); + self.main_view_query + .update_archetypes(world.as_unsafe_world_cell()); + self.view_light_query + .update_archetypes(world.as_unsafe_world_cell()); } fn run( diff --git a/crates/bevy_render/src/camera/camera_driver_node.rs b/crates/bevy_render/src/camera/camera_driver_node.rs index 38ba4ac185749..03a7f14f2c27e 100644 --- a/crates/bevy_render/src/camera/camera_driver_node.rs +++ b/crates/bevy_render/src/camera/camera_driver_node.rs @@ -22,7 +22,7 @@ impl CameraDriverNode { impl Node for CameraDriverNode { fn update(&mut self, world: &mut World) { - self.cameras.update_archetypes(world); + self.cameras.update_archetypes(world.as_unsafe_world_cell()); } fn run( &self, diff --git a/crates/bevy_render/src/render_graph/node.rs b/crates/bevy_render/src/render_graph/node.rs index 9307c235a062e..a7108577c4e3b 100644 --- a/crates/bevy_render/src/render_graph/node.rs +++ b/crates/bevy_render/src/render_graph/node.rs @@ -388,7 +388,8 @@ where T: ViewNode + Send + Sync + 'static, { fn update(&mut self, world: &mut World) { - self.view_query.update_archetypes(world); + self.view_query + .update_archetypes(world.as_unsafe_world_cell()); self.node.update(world); } diff --git a/crates/bevy_render/src/render_phase/draw.rs b/crates/bevy_render/src/render_phase/draw.rs index 405467ebeccc0..d93473b82b1bd 100644 --- a/crates/bevy_render/src/render_phase/draw.rs +++ b/crates/bevy_render/src/render_phase/draw.rs @@ -251,9 +251,12 @@ where /// Prepares the render command to be used. This is called once and only once before the phase /// begins. There may be zero or more [`draw`](RenderCommandState::draw) calls following a call to this function. fn prepare(&mut self, world: &'_ World) { - self.state.update_archetypes(world); - self.view.update_archetypes(world); - self.entity.update_archetypes(world); + self.state + .update_archetypes(world.as_unsafe_world_cell_readonly()); + self.view + .update_archetypes(world.as_unsafe_world_cell_readonly()); + self.entity + .update_archetypes(world.as_unsafe_world_cell_readonly()); } /// Fetches the ECS parameters for the wrapped [`RenderCommand`] and then renders it. diff --git a/crates/bevy_ui/src/render/render_pass.rs b/crates/bevy_ui/src/render/render_pass.rs index 90e7b6059cecc..aa1ed0841e949 100644 --- a/crates/bevy_ui/src/render/render_pass.rs +++ b/crates/bevy_ui/src/render/render_pass.rs @@ -36,8 +36,10 @@ impl UiPassNode { impl Node for UiPassNode { fn update(&mut self, world: &mut World) { - self.ui_view_query.update_archetypes(world); - self.default_camera_view_query.update_archetypes(world); + self.ui_view_query + .update_archetypes(world.as_unsafe_world_cell()); + self.default_camera_view_query + .update_archetypes(world.as_unsafe_world_cell()); } fn run( From e885d4150ed2328ca0226135907bcc92387de154 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Mon, 12 Jun 2023 19:43:26 -0400 Subject: [PATCH 15/22] update shader example --- examples/shader/post_processing.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/shader/post_processing.rs b/examples/shader/post_processing.rs index 1480e699179e8..650137145dde8 100644 --- a/examples/shader/post_processing.rs +++ b/examples/shader/post_processing.rs @@ -136,7 +136,7 @@ impl Node for PostProcessNode { // Since this is not a system we need to update the query manually. // This is mostly boilerplate. There are plans to remove this in the future. // For now, you can just copy it. - self.query.update_archetypes(world); + self.query.update_archetypes(world.as_unsafe_world_cell()); } // Runs the node logic From 86605f1c9141aa2c7b03c245fe1ce8a242a81a66 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Mon, 12 Jun 2023 19:50:25 -0400 Subject: [PATCH 16/22] update `SystemState::matches_world` --- crates/bevy_ecs/src/system/function_system.rs | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 82dfbd8e61ee2..7af1669960e7a 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -183,7 +183,7 @@ impl SystemState { where Param: ReadOnlySystemParam, { - self.validate_world(world); + self.validate_world(world.id()); self.update_archetypes(world.as_unsafe_world_cell_readonly()); // SAFETY: Param is read-only and doesn't allow mutable access to World. It also matches the World this SystemState was created with. unsafe { self.get_unchecked_manual(world.as_unsafe_world_cell_readonly()) } @@ -192,7 +192,7 @@ impl SystemState { /// Retrieve the mutable [`SystemParam`] values. #[inline] pub fn get_mut<'w, 's>(&'s mut self, world: &'w mut World) -> SystemParamItem<'w, 's, Param> { - self.validate_world(world); + self.validate_world(world.id()); self.update_archetypes(world.as_unsafe_world_cell_readonly()); // SAFETY: World is uniquely borrowed and matches the World this SystemState was created with. unsafe { self.get_unchecked_manual(world.as_unsafe_world_cell()) } @@ -206,17 +206,17 @@ impl SystemState { Param::apply(&mut self.param_state, &self.meta, world); } - /// Returns `true` if `world` is the same one that was used to call [`SystemState::new`]. + /// Returns `true` if `world_id` matches the [`World`] that was used to call [`SystemState::new`]. /// Otherwise, this returns false. #[inline] - pub fn matches_world(&self, world: &World) -> bool { - self.world_id == world.id() + pub fn matches_world(&self, world_id: WorldId) -> bool { + self.world_id == world_id } - /// Asserts that the [`SystemState`] matches the provided [`World`]. + /// Asserts that the [`SystemState`] matches the provided world. #[inline] - fn validate_world(&self, world: &World) { - assert!(self.matches_world(world), "Encountered a mismatched World. A SystemState cannot be used with Worlds other than the one it was created with."); + fn validate_world(&self, world_id: WorldId) { + assert!(self.matches_world(world_id), "Encountered a mismatched World. A SystemState cannot be used with Worlds other than the one it was created with."); } /// Updates the state's internal view of the `world`'s archetypes. If this is not called before fetching the parameters, @@ -254,7 +254,7 @@ impl SystemState { where Param: ReadOnlySystemParam, { - self.validate_world(world); + self.validate_world(world.id()); let change_tick = world.read_change_tick(); // SAFETY: Param is read-only and doesn't allow mutable access to World. // It also matches the World this SystemState was created with. @@ -273,7 +273,7 @@ impl SystemState { &'s mut self, world: &'w mut World, ) -> SystemParamItem<'w, 's, Param> { - self.validate_world(world); + self.validate_world(world.id()); let change_tick = world.change_tick(); // SAFETY: World is uniquely borrowed and matches the World this SystemState was created with. unsafe { self.fetch(world.as_unsafe_world_cell(), change_tick) } From d2c083d5cad60c59893cf345221cd5516425b6e2 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Mon, 12 Jun 2023 20:00:03 -0400 Subject: [PATCH 17/22] remove a leftover comment --- crates/bevy_ecs/src/world/unsafe_world_cell.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/crates/bevy_ecs/src/world/unsafe_world_cell.rs b/crates/bevy_ecs/src/world/unsafe_world_cell.rs index 1f54eaabbf2d7..997862981ae99 100644 --- a/crates/bevy_ecs/src/world/unsafe_world_cell.rs +++ b/crates/bevy_ecs/src/world/unsafe_world_cell.rs @@ -18,11 +18,6 @@ use crate::{ use bevy_ptr::Ptr; use std::{any::TypeId, cell::UnsafeCell, marker::PhantomData}; -// Note: We intentionally do not implement this for `&mut World`. -// Since reborrowing does not work for `impl AsUnsafeWorldCell` parameters, -// implementing this trait would cause mutable world references to be moved, -// which is an ergonomic footgun. - /// Variant of the [`World`] where resource and component accesses take `&self`, and the responsibility to avoid /// aliasing violations are given to the caller instead of being checked at compile-time by rust's unique XOR shared rule. /// From adb4116a2cf5fa53ca8c56095032f99afe280839 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Mon, 12 Jun 2023 21:30:23 -0400 Subject: [PATCH 18/22] fix docs --- crates/bevy_ecs/src/query/fetch.rs | 4 ++-- crates/bevy_ecs/src/system/query.rs | 4 ++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 5fc02c5ab4d5e..cd0bb152d32ce 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -376,7 +376,7 @@ pub unsafe trait WorldQuery { /// - `archetype` and `tables` must be from the same [`World`] that [`WorldQuery::init_state`] was called on. /// - [`Self::update_archetype_component_access`] must have been previously called with `archetype`. /// - `table` must correspond to `archetype`. - /// - `state` must be the [`State`](Self::state) that `fetch` was initialized with. + /// - `state` must be the [`State`](Self::State) that `fetch` was initialized with. unsafe fn set_archetype<'w>( fetch: &mut Self::Fetch<'w>, state: &Self::State, @@ -392,7 +392,7 @@ pub unsafe trait WorldQuery { /// - `table` must be from the same [`World`] that [`WorldQuery::init_state`] was called on. /// - `table` must belong to an archetype that was previously registered with /// [`Self::update_archetype_component_access`]. - /// - `state` must be the [`State`](Self::state) that `fetch` was initialized with. + /// - `state` must be the [`State`](Self::State) that `fetch` was initialized with. unsafe fn set_table<'w>(fetch: &mut Self::Fetch<'w>, state: &Self::State, table: &'w Table); /// Fetch [`Self::Item`](`WorldQuery::Item`) for either the given `entity` in the current [`Table`], diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index f47d7ae6e9830..21d334338711e 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -24,6 +24,8 @@ use std::{any::TypeId, borrow::Borrow, fmt::Debug}; /// A set of conditions that determines whether query items should be kept or discarded. /// This type parameter is optional. /// +/// [`World`]: crate::world::World +/// /// # System parameter declaration /// /// A query should always be declared as a system parameter. @@ -734,6 +736,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// This can only be called for read-only queries, see [`par_iter_mut`] for write-queries. /// /// [`par_iter_mut`]: Self::par_iter_mut + /// [`World`]: crate::world::World #[inline] pub fn par_iter(&self) -> QueryParIter<'_, '_, Q::ReadOnly, F::ReadOnly> { QueryParIter { @@ -750,6 +753,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// This can only be called for mutable queries, see [`par_iter`] for read-only-queries. /// /// [`par_iter`]: Self::par_iter + /// [`World`]: crate::world::World #[inline] pub fn par_iter_mut(&mut self) -> QueryParIter<'_, '_, Q, F> { QueryParIter { From 83420eb045693971bec82025dfa5bd2c8daa1716 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Mon, 12 Jun 2023 21:34:44 -0400 Subject: [PATCH 19/22] fix a benchmark --- benches/benches/bevy_ecs/world/world_get.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/benches/benches/bevy_ecs/world/world_get.rs b/benches/benches/bevy_ecs/world/world_get.rs index ee63498a095f1..725e9b2cf39f5 100644 --- a/benches/benches/bevy_ecs/world/world_get.rs +++ b/benches/benches/bevy_ecs/world/world_get.rs @@ -271,9 +271,10 @@ pub fn query_get_component_simple(criterion: &mut Criterion) { let entity = world.spawn(A(0.0)).id(); let mut query = world.query::<&mut A>(); + let world_cell = world.as_unsafe_world_cell(); bencher.iter(|| { for _x in 0..100000 { - let mut a = unsafe { query.get_unchecked(&world, entity).unwrap() }; + let mut a = unsafe { query.get_unchecked(world_cell, entity).unwrap() }; a.0 += 1.0; } }); From fbac88b900617320cbb490ef5719b4169e7930cc Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Tue, 13 Jun 2023 11:47:24 -0400 Subject: [PATCH 20/22] add `update_archetypes_unsafe_world_cell` --- crates/bevy_ecs/src/query/state.rs | 75 ++++++++++++------- crates/bevy_ecs/src/system/function_system.rs | 24 +++++- 2 files changed, 70 insertions(+), 29 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 29e922178ca02..267c7903893ba 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -124,7 +124,7 @@ impl QueryState { matched_archetypes: Default::default(), archetype_component_access: Default::default(), }; - state.update_archetypes(world.as_unsafe_world_cell()); + state.update_archetypes(world); state } @@ -140,13 +140,38 @@ impl QueryState { } } - /// Takes a query for the given [`World`], checks if the given world is the same as the query, and - /// updates the [`QueryState`]'s view of the [`World`] with any newly-added archetypes. + /// Updates the state's internal view of the [`World`]'s archetypes. If this is not called before querying data, + /// the results may not accurately reflect what is in the `world`. + /// + /// This is only required if a `manual` method (such as [`Self::get_manual`]) is being called, and it only needs to + /// be called if the `world` has been structurally mutated (i.e. added/removed a component or resource). Users using + /// non-`manual` methods such as [`QueryState::get`] do not need to call this as it will be automatically called for them. + /// + /// If you have an [`UnsafeWorldCell`] instead of `&World`, consider using [`QueryState::update_archetypes_unsafe_world_cell`]. /// /// # Panics /// /// If `world` does not match the one used to call `QueryState::new` for this instance. - pub fn update_archetypes(&mut self, world: UnsafeWorldCell) { + #[inline] + pub fn update_archetypes(&mut self, world: &World) { + self.update_archetypes_unsafe_world_cell(world.as_unsafe_world_cell_readonly()); + } + + /// Updates the state's internal view of the `world`'s archetypes. If this is not called before querying data, + /// the results may not accurately reflect what is in the `world`. + /// + /// This is only required if a `manual` method (such as [`Self::get_manual`]) is being called, and it only needs to + /// be called if the `world` has been structurally mutated (i.e. added/removed a component or resource). Users using + /// non-`manual` methods such as [`QueryState::get`] do not need to call this as it will be automatically called for them. + /// + /// # Note + /// + /// This method only accesses world metadata. + /// + /// # Panics + /// + /// If `world` does not match the one used to call `QueryState::new` for this instance. + pub fn update_archetypes_unsafe_world_cell(&mut self, world: UnsafeWorldCell) { self.validate_world(world.id()); let archetypes = world.archetypes(); let new_generation = archetypes.generation(); @@ -213,7 +238,7 @@ impl QueryState { world: &'w World, entity: Entity, ) -> Result, QueryEntityError> { - self.update_archetypes(world.as_unsafe_world_cell_readonly()); + self.update_archetypes(world); // SAFETY: query is read only unsafe { self.as_readonly().get_unchecked_manual( @@ -263,7 +288,7 @@ impl QueryState { world: &'w World, entities: [Entity; N], ) -> Result<[ROQueryItem<'w, Q>; N], QueryEntityError> { - self.update_archetypes(world.as_unsafe_world_cell_readonly()); + self.update_archetypes(world); // SAFETY: update_archetypes validates the `World` matches unsafe { @@ -283,7 +308,7 @@ impl QueryState { world: &'w mut World, entity: Entity, ) -> Result, QueryEntityError> { - self.update_archetypes(world.as_unsafe_world_cell()); + self.update_archetypes(world); let change_tick = world.change_tick(); let last_change_tick = world.last_change_tick(); // SAFETY: query has unique world access @@ -341,7 +366,7 @@ impl QueryState { world: &'w mut World, entities: [Entity; N], ) -> Result<[Q::Item<'w>; N], QueryEntityError> { - self.update_archetypes(world.as_unsafe_world_cell()); + self.update_archetypes(world); let change_tick = world.change_tick(); let last_change_tick = world.last_change_tick(); @@ -398,7 +423,7 @@ impl QueryState { world: UnsafeWorldCell<'w>, entity: Entity, ) -> Result, QueryEntityError> { - self.update_archetypes(world); + self.update_archetypes_unsafe_world_cell(world); self.get_unchecked_manual(world, entity, world.last_change_tick(), world.change_tick()) } @@ -529,7 +554,7 @@ impl QueryState { &'s mut self, world: &'w World, ) -> QueryIter<'w, 's, Q::ReadOnly, F::ReadOnly> { - self.update_archetypes(world.as_unsafe_world_cell_readonly()); + self.update_archetypes(world); // SAFETY: query is read only unsafe { self.as_readonly().iter_unchecked_manual( @@ -543,7 +568,7 @@ impl QueryState { /// Returns an [`Iterator`] over the query results for the given [`World`]. #[inline] pub fn iter_mut<'w, 's>(&'s mut self, world: &'w mut World) -> QueryIter<'w, 's, Q, F> { - self.update_archetypes(world.as_unsafe_world_cell()); + self.update_archetypes(world); let change_tick = world.change_tick(); let last_change_tick = world.last_change_tick(); // SAFETY: query has unique world access @@ -598,7 +623,7 @@ impl QueryState { &'s mut self, world: &'w World, ) -> QueryCombinationIter<'w, 's, Q::ReadOnly, F::ReadOnly, K> { - self.update_archetypes(world.as_unsafe_world_cell_readonly()); + self.update_archetypes(world); // SAFETY: query is read only unsafe { self.as_readonly().iter_combinations_unchecked_manual( @@ -631,7 +656,7 @@ impl QueryState { &'s mut self, world: &'w mut World, ) -> QueryCombinationIter<'w, 's, Q, F, K> { - self.update_archetypes(world.as_unsafe_world_cell()); + self.update_archetypes(world); let change_tick = world.change_tick(); let last_change_tick = world.last_change_tick(); // SAFETY: query has unique world access @@ -661,7 +686,7 @@ impl QueryState { where EntityList::Item: Borrow, { - self.update_archetypes(world.as_unsafe_world_cell_readonly()); + self.update_archetypes(world); // SAFETY: query is read only unsafe { self.as_readonly().iter_many_unchecked_manual( @@ -721,7 +746,7 @@ impl QueryState { where EntityList::Item: Borrow, { - self.update_archetypes(world.as_unsafe_world_cell()); + self.update_archetypes(world); let change_tick = world.change_tick(); let last_change_tick = world.last_change_tick(); // SAFETY: Query has unique world access. @@ -746,7 +771,7 @@ impl QueryState { &'s mut self, world: UnsafeWorldCell<'w>, ) -> QueryIter<'w, 's, Q, F> { - self.update_archetypes(world); + self.update_archetypes_unsafe_world_cell(world); self.iter_unchecked_manual(world, world.last_change_tick(), world.change_tick()) } @@ -763,7 +788,7 @@ impl QueryState { &'s mut self, world: UnsafeWorldCell<'w>, ) -> QueryCombinationIter<'w, 's, Q, F, K> { - self.update_archetypes(world); + self.update_archetypes_unsafe_world_cell(world); self.iter_combinations_unchecked_manual( world, world.last_change_tick(), @@ -840,7 +865,7 @@ impl QueryState { /// This can only be called for read-only queries, see [`Self::for_each_mut`] for write-queries. #[inline] pub fn for_each<'w, FN: FnMut(ROQueryItem<'w, Q>)>(&mut self, world: &'w World, func: FN) { - self.update_archetypes(world.as_unsafe_world_cell_readonly()); + self.update_archetypes(world); // SAFETY: query is read only unsafe { self.as_readonly().for_each_unchecked_manual( @@ -856,7 +881,7 @@ impl QueryState { /// `iter_mut()` method, but cannot be chained like a normal [`Iterator`]. #[inline] pub fn for_each_mut<'w, FN: FnMut(Q::Item<'w>)>(&mut self, world: &'w mut World, func: FN) { - self.update_archetypes(world.as_unsafe_world_cell()); + self.update_archetypes(world); let change_tick = world.change_tick(); let last_change_tick = world.last_change_tick(); // SAFETY: query has unique world access @@ -883,7 +908,7 @@ impl QueryState { world: UnsafeWorldCell<'w>, func: FN, ) { - self.update_archetypes(world); + self.update_archetypes_unsafe_world_cell(world); self.for_each_unchecked_manual(world, func, world.last_change_tick(), world.change_tick()); } @@ -897,7 +922,7 @@ impl QueryState { &'s mut self, world: &'w World, ) -> QueryParIter<'w, 's, Q::ReadOnly, F::ReadOnly> { - self.update_archetypes(world.as_unsafe_world_cell_readonly()); + self.update_archetypes(world); QueryParIter { world: world.as_unsafe_world_cell_readonly(), state: self.as_readonly(), @@ -914,7 +939,7 @@ impl QueryState { /// [`par_iter`]: Self::par_iter #[inline] pub fn par_iter_mut<'w, 's>(&'s mut self, world: &'w mut World) -> QueryParIter<'w, 's, Q, F> { - self.update_archetypes(world.as_unsafe_world_cell()); + self.update_archetypes(world); let this_run = world.change_tick(); let last_run = world.last_change_tick(); QueryParIter { @@ -1155,7 +1180,7 @@ impl QueryState { &mut self, world: &'w World, ) -> Result, QuerySingleError> { - self.update_archetypes(world.as_unsafe_world_cell_readonly()); + self.update_archetypes(world); // SAFETY: query is read only unsafe { @@ -1191,7 +1216,7 @@ impl QueryState { &mut self, world: &'w mut World, ) -> Result, QuerySingleError> { - self.update_archetypes(world.as_unsafe_world_cell()); + self.update_archetypes(world); let change_tick = world.change_tick(); let last_change_tick = world.last_change_tick(); @@ -1219,7 +1244,7 @@ impl QueryState { &mut self, world: UnsafeWorldCell<'w>, ) -> Result, QuerySingleError> { - self.update_archetypes(world); + self.update_archetypes_unsafe_world_cell(world); self.get_single_unchecked_manual(world, world.last_change_tick(), world.change_tick()) } diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 7af1669960e7a..e245f93716d78 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -184,8 +184,9 @@ impl SystemState { Param: ReadOnlySystemParam, { self.validate_world(world.id()); - self.update_archetypes(world.as_unsafe_world_cell_readonly()); - // SAFETY: Param is read-only and doesn't allow mutable access to World. It also matches the World this SystemState was created with. + self.update_archetypes(world); + // SAFETY: Param is read-only and doesn't allow mutable access to World. + // It also matches the World this SystemState was created with. unsafe { self.get_unchecked_manual(world.as_unsafe_world_cell_readonly()) } } @@ -193,7 +194,7 @@ impl SystemState { #[inline] pub fn get_mut<'w, 's>(&'s mut self, world: &'w mut World) -> SystemParamItem<'w, 's, Param> { self.validate_world(world.id()); - self.update_archetypes(world.as_unsafe_world_cell_readonly()); + self.update_archetypes(world); // SAFETY: World is uniquely borrowed and matches the World this SystemState was created with. unsafe { self.get_unchecked_manual(world.as_unsafe_world_cell()) } } @@ -219,14 +220,29 @@ impl SystemState { assert!(self.matches_world(world_id), "Encountered a mismatched World. A SystemState cannot be used with Worlds other than the one it was created with."); } + /// Updates the state's internal view of the [`World`]'s archetypes. If this is not called before fetching the parameters, + /// the results may not accurately reflect what is in the `world`. + /// + /// This is only required if [`SystemState::get_manual`] or [`SystemState::get_manual_mut`] is being called, and it only needs to + /// be called if the `world` has been structurally mutated (i.e. added/removed a component or resource). Users using + /// [`SystemState::get`] or [`SystemState::get_mut`] do not need to call this as it will be automatically called for them. + #[inline] + pub fn update_archetypes(&mut self, world: &World) { + self.update_archetypes_unsafe_world_cell(world.as_unsafe_world_cell_readonly()); + } + /// Updates the state's internal view of the `world`'s archetypes. If this is not called before fetching the parameters, /// the results may not accurately reflect what is in the `world`. /// /// This is only required if [`SystemState::get_manual`] or [`SystemState::get_manual_mut`] is being called, and it only needs to /// be called if the `world` has been structurally mutated (i.e. added/removed a component or resource). Users using /// [`SystemState::get`] or [`SystemState::get_mut`] do not need to call this as it will be automatically called for them. + /// + /// # Note + /// + /// This method only accesses world metadata. #[inline] - pub fn update_archetypes(&mut self, world: UnsafeWorldCell) { + pub fn update_archetypes_unsafe_world_cell(&mut self, world: UnsafeWorldCell) { let archetypes = world.archetypes(); let new_generation = archetypes.generation(); let old_generation = std::mem::replace(&mut self.archetype_generation, new_generation); From 9e4da2b0e0c493785d75b5020464c9888d9fcd31 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Tue, 13 Jun 2023 11:50:10 -0400 Subject: [PATCH 21/22] update rendering crates --- .../src/contrast_adaptive_sharpening/node.rs | 2 +- .../bevy_core_pipeline/src/core_2d/main_pass_2d_node.rs | 2 +- crates/bevy_core_pipeline/src/msaa_writeback.rs | 2 +- crates/bevy_pbr/src/render/light.rs | 6 ++---- crates/bevy_render/src/camera/camera_driver_node.rs | 2 +- crates/bevy_render/src/render_graph/node.rs | 3 +-- crates/bevy_render/src/render_phase/draw.rs | 9 +++------ crates/bevy_ui/src/render/render_pass.rs | 6 ++---- examples/shader/post_processing.rs | 2 +- 9 files changed, 13 insertions(+), 21 deletions(-) diff --git a/crates/bevy_core_pipeline/src/contrast_adaptive_sharpening/node.rs b/crates/bevy_core_pipeline/src/contrast_adaptive_sharpening/node.rs index 451c9d37c7ac0..c49d876f2214d 100644 --- a/crates/bevy_core_pipeline/src/contrast_adaptive_sharpening/node.rs +++ b/crates/bevy_core_pipeline/src/contrast_adaptive_sharpening/node.rs @@ -39,7 +39,7 @@ impl FromWorld for CASNode { impl Node for CASNode { fn update(&mut self, world: &mut World) { - self.query.update_archetypes(world.as_unsafe_world_cell()); + self.query.update_archetypes(world); } fn run( diff --git a/crates/bevy_core_pipeline/src/core_2d/main_pass_2d_node.rs b/crates/bevy_core_pipeline/src/core_2d/main_pass_2d_node.rs index 506d74f928272..708193524b70a 100644 --- a/crates/bevy_core_pipeline/src/core_2d/main_pass_2d_node.rs +++ b/crates/bevy_core_pipeline/src/core_2d/main_pass_2d_node.rs @@ -36,7 +36,7 @@ impl FromWorld for MainPass2dNode { impl Node for MainPass2dNode { fn update(&mut self, world: &mut World) { - self.query.update_archetypes(world.as_unsafe_world_cell()); + self.query.update_archetypes(world); } fn run( diff --git a/crates/bevy_core_pipeline/src/msaa_writeback.rs b/crates/bevy_core_pipeline/src/msaa_writeback.rs index f610e3eb9cfe3..cf881b07b608e 100644 --- a/crates/bevy_core_pipeline/src/msaa_writeback.rs +++ b/crates/bevy_core_pipeline/src/msaa_writeback.rs @@ -57,7 +57,7 @@ impl FromWorld for MsaaWritebackNode { impl Node for MsaaWritebackNode { fn update(&mut self, world: &mut World) { - self.cameras.update_archetypes(world.as_unsafe_world_cell()); + self.cameras.update_archetypes(world); } fn run( &self, diff --git a/crates/bevy_pbr/src/render/light.rs b/crates/bevy_pbr/src/render/light.rs index d7a1ef56c41df..c9308982088d9 100644 --- a/crates/bevy_pbr/src/render/light.rs +++ b/crates/bevy_pbr/src/render/light.rs @@ -1704,10 +1704,8 @@ impl ShadowPassNode { impl Node for ShadowPassNode { fn update(&mut self, world: &mut World) { - self.main_view_query - .update_archetypes(world.as_unsafe_world_cell()); - self.view_light_query - .update_archetypes(world.as_unsafe_world_cell()); + self.main_view_query.update_archetypes(world); + self.view_light_query.update_archetypes(world); } fn run( diff --git a/crates/bevy_render/src/camera/camera_driver_node.rs b/crates/bevy_render/src/camera/camera_driver_node.rs index 03a7f14f2c27e..38ba4ac185749 100644 --- a/crates/bevy_render/src/camera/camera_driver_node.rs +++ b/crates/bevy_render/src/camera/camera_driver_node.rs @@ -22,7 +22,7 @@ impl CameraDriverNode { impl Node for CameraDriverNode { fn update(&mut self, world: &mut World) { - self.cameras.update_archetypes(world.as_unsafe_world_cell()); + self.cameras.update_archetypes(world); } fn run( &self, diff --git a/crates/bevy_render/src/render_graph/node.rs b/crates/bevy_render/src/render_graph/node.rs index a7108577c4e3b..9307c235a062e 100644 --- a/crates/bevy_render/src/render_graph/node.rs +++ b/crates/bevy_render/src/render_graph/node.rs @@ -388,8 +388,7 @@ where T: ViewNode + Send + Sync + 'static, { fn update(&mut self, world: &mut World) { - self.view_query - .update_archetypes(world.as_unsafe_world_cell()); + self.view_query.update_archetypes(world); self.node.update(world); } diff --git a/crates/bevy_render/src/render_phase/draw.rs b/crates/bevy_render/src/render_phase/draw.rs index d93473b82b1bd..405467ebeccc0 100644 --- a/crates/bevy_render/src/render_phase/draw.rs +++ b/crates/bevy_render/src/render_phase/draw.rs @@ -251,12 +251,9 @@ where /// Prepares the render command to be used. This is called once and only once before the phase /// begins. There may be zero or more [`draw`](RenderCommandState::draw) calls following a call to this function. fn prepare(&mut self, world: &'_ World) { - self.state - .update_archetypes(world.as_unsafe_world_cell_readonly()); - self.view - .update_archetypes(world.as_unsafe_world_cell_readonly()); - self.entity - .update_archetypes(world.as_unsafe_world_cell_readonly()); + self.state.update_archetypes(world); + self.view.update_archetypes(world); + self.entity.update_archetypes(world); } /// Fetches the ECS parameters for the wrapped [`RenderCommand`] and then renders it. diff --git a/crates/bevy_ui/src/render/render_pass.rs b/crates/bevy_ui/src/render/render_pass.rs index aa1ed0841e949..90e7b6059cecc 100644 --- a/crates/bevy_ui/src/render/render_pass.rs +++ b/crates/bevy_ui/src/render/render_pass.rs @@ -36,10 +36,8 @@ impl UiPassNode { impl Node for UiPassNode { fn update(&mut self, world: &mut World) { - self.ui_view_query - .update_archetypes(world.as_unsafe_world_cell()); - self.default_camera_view_query - .update_archetypes(world.as_unsafe_world_cell()); + self.ui_view_query.update_archetypes(world); + self.default_camera_view_query.update_archetypes(world); } fn run( diff --git a/examples/shader/post_processing.rs b/examples/shader/post_processing.rs index 650137145dde8..1480e699179e8 100644 --- a/examples/shader/post_processing.rs +++ b/examples/shader/post_processing.rs @@ -136,7 +136,7 @@ impl Node for PostProcessNode { // Since this is not a system we need to update the query manually. // This is mostly boilerplate. There are plans to remove this in the future. // For now, you can just copy it. - self.query.update_archetypes(world.as_unsafe_world_cell()); + self.query.update_archetypes(world); } // Runs the node logic From 36f65e6d5b7f0154d5737eeacd86c976b44e7c88 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Tue, 13 Jun 2023 23:46:47 -0400 Subject: [PATCH 22/22] fix an incorrect safety comment --- crates/bevy_ecs/src/world/unsafe_world_cell.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/world/unsafe_world_cell.rs b/crates/bevy_ecs/src/world/unsafe_world_cell.rs index 997862981ae99..82ab7f1c1badb 100644 --- a/crates/bevy_ecs/src/world/unsafe_world_cell.rs +++ b/crates/bevy_ecs/src/world/unsafe_world_cell.rs @@ -443,7 +443,9 @@ impl<'w> UnsafeWorldCell<'w> { }; Some(MutUntyped { - // SAFETY: This function has exclusive access to the world so nothing aliases `ptr`. + // SAFETY: + // - caller ensures that `self` has permission to access the resource + // - caller ensures that the resource is unaliased value: unsafe { ptr.assert_unique() }, ticks, })