From 6760b6e8a5376583ddc4b6ee0b12fc95250ea5f1 Mon Sep 17 00:00:00 2001 From: James Liu Date: Sun, 17 Mar 2024 23:28:31 -0700 Subject: [PATCH] Remove WorldCell (#12551) # Objective Fixes #12549. WorldCell's support of everything a World can do is incomplete, and represents an alternative, potentially confusing, and less performant way of pulling multiple fetches from a `World`. The typical approach is to use `SystemState` for a runtime cached and safe way, or `UnsafeWorldCell` if the use of `unsafe` is tolerable. ## Solution Remove it! --- ## Changelog Removed: `WorldCell` Removed: `World::cell` ## Migration Guide `WorldCell` has been removed. If you were using it to fetch multiple distinct values from a `&mut World`, use `SystemState` by calling `SystemState::get` instead. Alternatively, if `SystemState` cannot be used, `UnsafeWorldCell` can instead be used in unsafe contexts. --- crates/bevy_ecs/src/world/mod.rs | 12 - .../bevy_ecs/src/world/unsafe_world_cell.rs | 32 +- crates/bevy_ecs/src/world/world_cell.rs | 479 ------------------ 3 files changed, 1 insertion(+), 522 deletions(-) delete mode 100644 crates/bevy_ecs/src/world/world_cell.rs diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 74c7ffb7f5cdb..3f87d8026db27 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -6,7 +6,6 @@ mod entity_ref; pub mod error; mod spawn_batch; pub mod unsafe_world_cell; -mod world_cell; pub use crate::change_detection::{Mut, Ref, CHECK_TICK_THRESHOLD}; pub use crate::world::command_queue::CommandQueue; @@ -16,7 +15,6 @@ pub use entity_ref::{ OccupiedEntry, VacantEntry, }; pub use spawn_batch::*; -pub use world_cell::*; use crate::{ archetype::{ArchetypeComponentId, ArchetypeId, ArchetypeRow, Archetypes}, @@ -111,8 +109,6 @@ pub struct World { pub(crate) storages: Storages, pub(crate) bundles: Bundles, pub(crate) removed_components: RemovedComponentEvents, - /// Access cache used by [`WorldCell`]. Is only accessed in the `Drop` impl of `WorldCell`. - pub(crate) archetype_component_access: ArchetypeComponentAccess, pub(crate) change_tick: AtomicU32, pub(crate) last_change_tick: Tick, pub(crate) last_check_tick: Tick, @@ -129,7 +125,6 @@ impl Default for World { storages: Default::default(), bundles: Default::default(), removed_components: Default::default(), - archetype_component_access: Default::default(), // Default value is `1`, and `last_change_tick`s default to `0`, such that changes // are detected on first system runs and for direct world queries. change_tick: AtomicU32::new(1), @@ -217,13 +212,6 @@ impl World { &self.removed_components } - /// Retrieves a [`WorldCell`], which safely enables multiple mutable World accesses at the same - /// time, provided those accesses do not conflict with each other. - #[inline] - pub fn cell(&mut self) -> WorldCell<'_> { - WorldCell::new(self) - } - /// Creates a new [`Commands`] instance that writes to the world's command queue /// Use [`World::flush_commands`] to apply all queued commands #[inline] diff --git a/crates/bevy_ecs/src/world/unsafe_world_cell.rs b/crates/bevy_ecs/src/world/unsafe_world_cell.rs index 9fbb6e52de668..429a9658fdf78 100644 --- a/crates/bevy_ecs/src/world/unsafe_world_cell.rs +++ b/crates/bevy_ecs/src/world/unsafe_world_cell.rs @@ -4,7 +4,7 @@ use super::{command_queue::CommandQueue, Mut, Ref, World, WorldId}; use crate::{ - archetype::{Archetype, ArchetypeComponentId, Archetypes}, + archetype::{Archetype, Archetypes}, bundle::Bundles, change_detection::{MutUntyped, Ticks, TicksMut}, component::{ComponentId, ComponentTicks, Components, StorageType, Tick, TickCells}, @@ -284,36 +284,6 @@ impl<'w> UnsafeWorldCell<'w> { &unsafe { self.unsafe_world() }.storages } - /// Shorthand helper function for getting the [`ArchetypeComponentId`] for a resource. - #[inline] - pub(crate) fn get_resource_archetype_component_id( - self, - component_id: ComponentId, - ) -> Option { - // SAFETY: - // - we only access world metadata - let resource = unsafe { self.world_metadata() } - .storages - .resources - .get(component_id)?; - Some(resource.id()) - } - - /// Shorthand helper function for getting the [`ArchetypeComponentId`] for a resource. - #[inline] - pub(crate) fn get_non_send_archetype_component_id( - self, - component_id: ComponentId, - ) -> Option { - // SAFETY: - // - we only access world metadata - let resource = unsafe { self.world_metadata() } - .storages - .non_send_resources - .get(component_id)?; - Some(resource.id()) - } - /// Retrieves an [`UnsafeEntityCell`] that exposes read and write operations for the given `entity`. /// Similar to the [`UnsafeWorldCell`], you are in charge of making sure that no aliasing rules are violated. #[inline] diff --git a/crates/bevy_ecs/src/world/world_cell.rs b/crates/bevy_ecs/src/world/world_cell.rs deleted file mode 100644 index 2ae79b32306e6..0000000000000 --- a/crates/bevy_ecs/src/world/world_cell.rs +++ /dev/null @@ -1,479 +0,0 @@ -use bevy_utils::tracing::error; - -use crate::{ - archetype::ArchetypeComponentId, - event::{Event, Events}, - storage::SparseSet, - system::Resource, - world::{Mut, World}, -}; -use std::{ - any::TypeId, - cell::RefCell, - ops::{Deref, DerefMut}, - rc::Rc, -}; - -use super::unsafe_world_cell::UnsafeWorldCell; - -/// Exposes safe mutable access to multiple resources at a time in a World. Attempting to access -/// World in a way that violates Rust's mutability rules will panic thanks to runtime checks. -pub struct WorldCell<'w> { - pub(crate) world: UnsafeWorldCell<'w>, - pub(crate) access: Rc>, -} - -pub(crate) struct ArchetypeComponentAccess { - access: SparseSet, -} - -impl Default for ArchetypeComponentAccess { - fn default() -> Self { - Self { - access: SparseSet::new(), - } - } -} - -const UNIQUE_ACCESS: usize = 0; -const BASE_ACCESS: usize = 1; -impl ArchetypeComponentAccess { - const fn new() -> Self { - Self { - access: SparseSet::new(), - } - } - - fn read(&mut self, id: ArchetypeComponentId) -> bool { - let id_access = self.access.get_or_insert_with(id, || BASE_ACCESS); - if *id_access == UNIQUE_ACCESS { - false - } else { - *id_access += 1; - true - } - } - - fn drop_read(&mut self, id: ArchetypeComponentId) { - let id_access = self.access.get_or_insert_with(id, || BASE_ACCESS); - *id_access -= 1; - } - - fn write(&mut self, id: ArchetypeComponentId) -> bool { - let id_access = self.access.get_or_insert_with(id, || BASE_ACCESS); - if *id_access == BASE_ACCESS { - *id_access = UNIQUE_ACCESS; - true - } else { - false - } - } - - fn drop_write(&mut self, id: ArchetypeComponentId) { - let id_access = self.access.get_or_insert_with(id, || BASE_ACCESS); - *id_access = BASE_ACCESS; - } -} - -impl<'w> Drop for WorldCell<'w> { - fn drop(&mut self) { - let mut access = self.access.borrow_mut(); - - { - // SAFETY: `WorldCell` does not hand out `UnsafeWorldCell` to anywhere else so this is the only - // `UnsafeWorldCell` and we have exclusive access to it. - let world = unsafe { self.world.world_mut() }; - let world_cached_access = &mut world.archetype_component_access; - - // give world ArchetypeComponentAccess back to reuse allocations - std::mem::swap(world_cached_access, &mut *access); - } - } -} - -/// A read-only borrow of some data stored in a [`World`]. This type is returned by [`WorldCell`], -/// which uses run-time checks to ensure that the borrow does not violate Rust's aliasing rules. -pub struct WorldBorrow<'w, T> { - value: &'w T, - archetype_component_id: ArchetypeComponentId, - access: Rc>, -} - -impl<'w, T> WorldBorrow<'w, T> { - fn try_new( - value: impl FnOnce() -> Option<&'w T>, - archetype_component_id: ArchetypeComponentId, - access: Rc>, - ) -> Option { - assert!( - access.borrow_mut().read(archetype_component_id), - "Attempted to immutably access {}, but it is already mutably borrowed", - std::any::type_name::(), - ); - let value = value()?; - Some(Self { - value, - archetype_component_id, - access, - }) - } -} - -impl<'w, T> Deref for WorldBorrow<'w, T> { - type Target = T; - - #[inline] - fn deref(&self) -> &Self::Target { - self.value - } -} - -impl<'w, T> Drop for WorldBorrow<'w, T> { - fn drop(&mut self) { - let mut access = self.access.borrow_mut(); - access.drop_read(self.archetype_component_id); - } -} - -/// A mutable borrow of some data stored in a [`World`]. This type is returned by [`WorldCell`], -/// which uses run-time checks to ensure that the borrow does not violate Rust's aliasing rules. -pub struct WorldBorrowMut<'w, T> { - value: Mut<'w, T>, - archetype_component_id: ArchetypeComponentId, - access: Rc>, -} - -impl<'w, T> WorldBorrowMut<'w, T> { - fn try_new( - value: impl FnOnce() -> Option>, - archetype_component_id: ArchetypeComponentId, - access: Rc>, - ) -> Option { - assert!( - access.borrow_mut().write(archetype_component_id), - "Attempted to mutably access {}, but it is already mutably borrowed", - std::any::type_name::(), - ); - let value = value()?; - Some(Self { - value, - archetype_component_id, - access, - }) - } -} - -impl<'w, T> Deref for WorldBorrowMut<'w, T> { - type Target = T; - - #[inline] - fn deref(&self) -> &Self::Target { - self.value.deref() - } -} - -impl<'w, T> DerefMut for WorldBorrowMut<'w, T> { - fn deref_mut(&mut self) -> &mut Self::Target { - &mut self.value - } -} - -impl<'w, T> Drop for WorldBorrowMut<'w, T> { - fn drop(&mut self) { - let mut access = self.access.borrow_mut(); - access.drop_write(self.archetype_component_id); - } -} - -impl<'w> WorldCell<'w> { - pub(crate) fn new(world: &'w mut World) -> Self { - // this is cheap because ArchetypeComponentAccess::new() is const / allocation free - let access = std::mem::replace( - &mut world.archetype_component_access, - ArchetypeComponentAccess::new(), - ); - // world's ArchetypeComponentAccess is recycled to cut down on allocations - Self { - world: world.as_unsafe_world_cell(), - access: Rc::new(RefCell::new(access)), - } - } - - /// Gets a reference to the resource of the given type - pub fn get_resource(&self) -> Option> { - let component_id = self.world.components().get_resource_id(TypeId::of::())?; - - let archetype_component_id = self - .world - .get_resource_archetype_component_id(component_id)?; - - WorldBorrow::try_new( - // SAFETY: access is checked by WorldBorrow - || unsafe { self.world.get_resource::() }, - archetype_component_id, - self.access.clone(), - ) - } - - /// Gets a reference to the resource of the given type - /// - /// # Panics - /// - /// Panics if the resource does not exist. Use [`get_resource`](WorldCell::get_resource) instead - /// if you want to handle this case. - pub fn resource(&self) -> WorldBorrow<'_, T> { - match self.get_resource() { - Some(x) => x, - None => panic!( - "Requested resource {} does not exist in the `World`. - Did you forget to add it using `app.insert_resource` / `app.init_resource`? - Resources are also implicitly added via `app.add_event`, - and can be added by plugins.", - std::any::type_name::() - ), - } - } - - /// Gets a mutable reference to the resource of the given type - pub fn get_resource_mut(&self) -> Option> { - let component_id = self.world.components().get_resource_id(TypeId::of::())?; - - let archetype_component_id = self - .world - .get_resource_archetype_component_id(component_id)?; - WorldBorrowMut::try_new( - // SAFETY: access is checked by WorldBorrowMut - || unsafe { self.world.get_resource_mut::() }, - archetype_component_id, - self.access.clone(), - ) - } - - /// Gets a mutable reference to the resource of the given type - /// - /// # Panics - /// - /// Panics if the resource does not exist. Use [`get_resource_mut`](WorldCell::get_resource_mut) - /// instead if you want to handle this case. - pub fn resource_mut(&self) -> WorldBorrowMut<'_, T> { - match self.get_resource_mut() { - Some(x) => x, - None => panic!( - "Requested resource {} does not exist in the `World`. - Did you forget to add it using `app.insert_resource` / `app.init_resource`? - Resources are also implicitly added via `app.add_event`, - and can be added by plugins.", - std::any::type_name::() - ), - } - } - - /// Gets an immutable reference to the non-send resource of the given type, if it exists. - pub fn get_non_send_resource(&self) -> Option> { - let component_id = self.world.components().get_resource_id(TypeId::of::())?; - - let archetype_component_id = self - .world - .get_non_send_archetype_component_id(component_id)?; - WorldBorrow::try_new( - // SAFETY: access is checked by WorldBorrowMut - || unsafe { self.world.get_non_send_resource::() }, - archetype_component_id, - self.access.clone(), - ) - } - - /// Gets an immutable reference to the non-send resource of the given type, if it exists. - /// - /// # Panics - /// - /// Panics if the resource does not exist. Use - /// [`get_non_send_resource`](WorldCell::get_non_send_resource) instead if you want to handle - /// this case. - pub fn non_send_resource(&self) -> WorldBorrow<'_, T> { - match self.get_non_send_resource() { - Some(x) => x, - None => panic!( - "Requested non-send resource {} does not exist in the `World`. - Did you forget to add it using `app.insert_non_send_resource` / `app.init_non_send_resource`? - Non-send resources can also be added by plugins.", - std::any::type_name::() - ), - } - } - - /// Gets a mutable reference to the non-send resource of the given type, if it exists. - pub fn get_non_send_resource_mut(&self) -> Option> { - let component_id = self.world.components().get_resource_id(TypeId::of::())?; - - let archetype_component_id = self - .world - .get_non_send_archetype_component_id(component_id)?; - WorldBorrowMut::try_new( - // SAFETY: access is checked by WorldBorrowMut - || unsafe { self.world.get_non_send_resource_mut::() }, - archetype_component_id, - self.access.clone(), - ) - } - - /// Gets a mutable reference to the non-send resource of the given type, if it exists. - /// - /// # Panics - /// - /// Panics if the resource does not exist. Use - /// [`get_non_send_resource_mut`](WorldCell::get_non_send_resource_mut) instead if you want to - /// handle this case. - pub fn non_send_resource_mut(&self) -> WorldBorrowMut<'_, T> { - match self.get_non_send_resource_mut() { - Some(x) => x, - None => panic!( - "Requested non-send resource {} does not exist in the `World`. - Did you forget to add it using `app.insert_non_send_resource` / `app.init_non_send_resource`? - Non-send resources can also be added by plugins.", - std::any::type_name::() - ), - } - } - - /// Sends an [`Event`]. - #[inline] - pub fn send_event(&self, event: E) { - self.send_event_batch(std::iter::once(event)); - } - - /// Sends the default value of the [`Event`] of type `E`. - #[inline] - pub fn send_event_default(&self) { - self.send_event_batch(std::iter::once(E::default())); - } - - /// Sends a batch of [`Event`]s from an iterator. - #[inline] - pub fn send_event_batch(&self, events: impl Iterator) { - match self.get_resource_mut::>() { - Some(mut events_resource) => events_resource.extend(events), - None => error!( - "Unable to send event `{}`\n\tEvent must be added to the app with `add_event()`\n\thttps://docs.rs/bevy/*/bevy/app/struct.App.html#method.add_event ", - std::any::type_name::() - ), - } - } -} - -#[cfg(test)] -mod tests { - use super::BASE_ACCESS; - use crate as bevy_ecs; - use crate::{system::Resource, world::World}; - use std::any::TypeId; - - #[derive(Resource)] - struct A(u32); - - #[derive(Resource)] - struct B(u64); - - #[test] - fn world_cell() { - let mut world = World::default(); - world.insert_resource(A(1)); - world.insert_resource(B(1)); - let cell = world.cell(); - { - let mut a = cell.resource_mut::(); - assert_eq!(1, a.0); - a.0 = 2; - } - { - let a = cell.resource::(); - assert_eq!(2, a.0, "ensure access is dropped"); - - let a2 = cell.resource::(); - assert_eq!( - 2, a2.0, - "ensure multiple immutable accesses can occur at the same time" - ); - } - { - let a = cell.resource_mut::(); - assert_eq!( - 2, a.0, - "ensure both immutable accesses are dropped, enabling a new mutable access" - ); - - let b = cell.resource::(); - assert_eq!( - 1, b.0, - "ensure multiple non-conflicting mutable accesses can occur at the same time" - ); - } - } - - #[test] - fn world_access_reused() { - let mut world = World::default(); - world.insert_resource(A(1)); - { - let cell = world.cell(); - { - let mut a = cell.resource_mut::(); - assert_eq!(1, a.0); - a.0 = 2; - } - } - - let u32_component_id = world.components.get_resource_id(TypeId::of::()).unwrap(); - let u32_archetype_component_id = world - .get_resource_archetype_component_id(u32_component_id) - .unwrap(); - assert_eq!(world.archetype_component_access.access.len(), 1); - assert_eq!( - world - .archetype_component_access - .access - .get(u32_archetype_component_id), - Some(&BASE_ACCESS), - "reused access count is 'base'" - ); - } - - #[test] - #[should_panic] - fn world_cell_double_mut() { - let mut world = World::default(); - world.insert_resource(A(1)); - let cell = world.cell(); - let _value_a = cell.resource_mut::(); - let _value_b = cell.resource_mut::(); - } - - #[test] - #[should_panic] - fn world_cell_ref_and_mut() { - let mut world = World::default(); - world.insert_resource(A(1)); - let cell = world.cell(); - let _value_a = cell.resource::(); - let _value_b = cell.resource_mut::(); - } - - #[test] - #[should_panic] - fn world_cell_mut_and_ref() { - let mut world = World::default(); - world.insert_resource(A(1)); - let cell = world.cell(); - let _value_a = cell.resource_mut::(); - let _value_b = cell.resource::(); - } - - #[test] - fn world_cell_ref_and_ref() { - let mut world = World::default(); - world.insert_resource(A(1)); - let cell = world.cell(); - let _value_a = cell.resource::(); - let _value_b = cell.resource::(); - } -}