From 37b4997ed94301367aa2e994de2471df73181b1a Mon Sep 17 00:00:00 2001 From: James Liu Date: Tue, 21 Jun 2022 20:35:26 +0000 Subject: [PATCH] Mark mutable APIs under ECS storage as pub(crate) (#5065) # Objective Closes #1557. Partially addresses #3362. Cleanup the public facing API for storage types. Most of these APIs are difficult to use safely when directly interfacing with these types, and is also currently impossible to interact with in normal ECS use as there is no `World::storages_mut`. The majority of these types should be easy enough to read, and perhaps mutate the contents, but never structurally altered without the same checks in the rest of bevy_ecs code. This both cleans up the public facing types and helps use unused code detection to remove a few of the APIs we're not using internally. ## Solution - Mark all APIs that take `&mut T` under `bevy_ecs::storage` as `pub(crate)` or `pub(super)` - Cleanup after it all. Entire type visibility changes: - `BlobVec` is `pub(super)`, only storage code should be directly interacting with it. - `SparseArray` is now `pub(crate)` for the entire type. It's an implementation detail for `Table` and `(Component)SparseSet`. - `TableMoveResult` is now `pub(crate) --- ## Changelog TODO ## Migration Guide Dear God, I hope not. --- crates/bevy_ecs/src/archetype.rs | 51 ++++++++----- crates/bevy_ecs/src/storage/blob_vec.rs | 2 +- crates/bevy_ecs/src/storage/mod.rs | 1 - crates/bevy_ecs/src/storage/sparse_set.rs | 53 ++----------- crates/bevy_ecs/src/storage/table.rs | 92 +++++++++++------------ crates/bevy_ecs/src/world/mod.rs | 2 +- 6 files changed, 82 insertions(+), 119 deletions(-) diff --git a/crates/bevy_ecs/src/archetype.rs b/crates/bevy_ecs/src/archetype.rs index ab33119a93be61..2c01f8c1bb4b2c 100644 --- a/crates/bevy_ecs/src/archetype.rs +++ b/crates/bevy_ecs/src/archetype.rs @@ -32,21 +32,35 @@ impl ArchetypeId { } } -pub enum ComponentStatus { +pub(crate) enum ComponentStatus { Added, Mutated, } pub struct AddBundle { pub archetype_id: ArchetypeId, - pub bundle_status: Vec, + pub(crate) bundle_status: Vec, } +/// Archetypes and bundles form a graph. Adding or removing a bundle moves +/// an [`Entity`] to a new [`Archetype`]. +/// +/// [`Edges`] caches the results of these moves. Each archetype caches +/// the result of a structural alteration. This can be used to monitor the +/// state of the archetype graph. +/// +/// Note: This type only contains edges the [`World`] has already traversed. +/// If any of functions return `None`, it doesn't mean there is guarenteed +/// not to be a result of adding or removing that bundle, but rather that +/// operation that has moved an entity along that edge has not been performed +/// yet. +/// +/// [`World`]: crate::world::World #[derive(Default)] pub struct Edges { - pub add_bundle: SparseArray, - pub remove_bundle: SparseArray>, - pub remove_bundle_intersection: SparseArray>, + add_bundle: SparseArray, + remove_bundle: SparseArray>, + remove_bundle_intersection: SparseArray>, } impl Edges { @@ -56,7 +70,7 @@ impl Edges { } #[inline] - pub fn insert_add_bundle( + pub(crate) fn insert_add_bundle( &mut self, bundle_id: BundleId, archetype_id: ArchetypeId, @@ -77,7 +91,11 @@ impl Edges { } #[inline] - pub fn insert_remove_bundle(&mut self, bundle_id: BundleId, archetype_id: Option) { + pub(crate) fn insert_remove_bundle( + &mut self, + bundle_id: BundleId, + archetype_id: Option, + ) { self.remove_bundle.insert(bundle_id, archetype_id); } @@ -90,7 +108,7 @@ impl Edges { } #[inline] - pub fn insert_remove_bundle_intersection( + pub(crate) fn insert_remove_bundle_intersection( &mut self, bundle_id: BundleId, archetype_id: Option, @@ -237,14 +255,14 @@ impl Archetype { } #[inline] - pub fn set_entity_table_row(&mut self, index: usize, table_row: usize) { + pub(crate) fn set_entity_table_row(&mut self, index: usize, table_row: usize) { self.table_info.entity_rows[index] = table_row; } /// # Safety /// valid component values must be immediately written to the relevant storages /// `table_row` must be valid - pub unsafe fn allocate(&mut self, entity: Entity, table_row: usize) -> EntityLocation { + pub(crate) unsafe fn allocate(&mut self, entity: Entity, table_row: usize) -> EntityLocation { self.entities.push(entity); self.table_info.entity_rows.push(table_row); @@ -254,7 +272,7 @@ impl Archetype { } } - pub fn reserve(&mut self, additional: usize) { + pub(crate) fn reserve(&mut self, additional: usize) { self.entities.reserve(additional); self.table_info.entity_rows.reserve(additional); } @@ -407,7 +425,7 @@ impl Archetypes { } #[inline] - pub fn empty_mut(&mut self) -> &mut Archetype { + pub(crate) fn empty_mut(&mut self) -> &mut Archetype { // SAFE: empty archetype always exists unsafe { self.archetypes @@ -422,7 +440,7 @@ impl Archetypes { } #[inline] - pub fn resource_mut(&mut self) -> &mut Archetype { + pub(crate) fn resource_mut(&mut self) -> &mut Archetype { // SAFE: resource archetype always exists unsafe { self.archetypes @@ -440,11 +458,6 @@ impl Archetypes { self.archetypes.get(id.index()) } - #[inline] - pub fn get_mut(&mut self, id: ArchetypeId) -> Option<&mut Archetype> { - self.archetypes.get_mut(id.index()) - } - #[inline] pub(crate) fn get_2_mut( &mut self, @@ -518,7 +531,7 @@ impl Archetypes { self.archetype_component_count } - pub fn clear_entities(&mut self) { + pub(crate) fn clear_entities(&mut self) { for archetype in &mut self.archetypes { archetype.clear_entities(); } diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs index 7a3991d35e48e2..8ce77d73a64a7d 100644 --- a/crates/bevy_ecs/src/storage/blob_vec.rs +++ b/crates/bevy_ecs/src/storage/blob_vec.rs @@ -9,7 +9,7 @@ use bevy_ptr::{OwningPtr, Ptr, PtrMut}; /// A flat, type-erased data storage type /// /// Used to densely store homogeneous ECS data. -pub struct BlobVec { +pub(super) struct BlobVec { item_layout: Layout, capacity: usize, /// Number of elements, not bytes diff --git a/crates/bevy_ecs/src/storage/mod.rs b/crates/bevy_ecs/src/storage/mod.rs index f54a2275bfe074..571f05184cdbd0 100644 --- a/crates/bevy_ecs/src/storage/mod.rs +++ b/crates/bevy_ecs/src/storage/mod.rs @@ -4,7 +4,6 @@ mod blob_vec; mod sparse_set; mod table; -pub use blob_vec::*; pub use sparse_set::*; pub use table::*; diff --git a/crates/bevy_ecs/src/storage/sparse_set.rs b/crates/bevy_ecs/src/storage/sparse_set.rs index f62999a2a9d702..1d5172b64a6245 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -9,7 +9,7 @@ use std::{cell::UnsafeCell, hash::Hash, marker::PhantomData}; type EntityId = u32; #[derive(Debug)] -pub struct SparseArray { +pub(crate) struct SparseArray { values: Vec>, marker: PhantomData, } @@ -31,13 +31,6 @@ impl SparseArray { } impl SparseArray { - pub fn with_capacity(capacity: usize) -> Self { - Self { - values: Vec::with_capacity(capacity), - marker: PhantomData, - } - } - #[inline] pub fn insert(&mut self, index: I, value: V) { let index = index.sparse_set_index(); @@ -74,18 +67,6 @@ impl SparseArray { self.values.get_mut(index).and_then(|value| value.take()) } - #[inline] - pub fn get_or_insert_with(&mut self, index: I, func: impl FnOnce() -> V) -> &mut V { - let index = index.sparse_set_index(); - if index < self.values.len() { - return self.values[index].get_or_insert_with(func); - } - self.values.resize_with(index + 1, || None); - let value = &mut self.values[index]; - *value = Some(func()); - value.as_mut().unwrap() - } - pub fn clear(&mut self) { self.values.clear(); } @@ -108,7 +89,7 @@ pub struct ComponentSparseSet { } impl ComponentSparseSet { - pub fn new(component_info: &ComponentInfo, capacity: usize) -> Self { + pub(crate) fn new(component_info: &ComponentInfo, capacity: usize) -> Self { Self { dense: Column::with_capacity(component_info, capacity), entities: Vec::with_capacity(capacity), @@ -116,7 +97,7 @@ impl ComponentSparseSet { } } - pub fn clear(&mut self) { + pub(crate) fn clear(&mut self) { self.dense.clear(); self.entities.clear(); self.sparse.clear(); @@ -138,7 +119,7 @@ impl ComponentSparseSet { /// # Safety /// The `value` pointer must point to a valid address that matches the [`Layout`](std::alloc::Layout) /// inside the [`ComponentInfo`] given when constructing this sparse set. - pub unsafe fn insert(&mut self, entity: Entity, value: OwningPtr<'_>, change_tick: u32) { + pub(crate) unsafe fn insert(&mut self, entity: Entity, value: OwningPtr<'_>, change_tick: u32) { if let Some(&dense_index) = self.sparse.get(entity.id()) { #[cfg(debug_assertions)] assert_eq!(entity, self.entities[dense_index as usize]); @@ -209,7 +190,7 @@ impl ComponentSparseSet { /// Removes the `entity` from this sparse set and returns a pointer to the associated value (if /// it exists). #[must_use = "The returned pointer must be used to drop the removed component."] - pub fn remove_and_forget(&mut self, entity: Entity) -> Option> { + pub(crate) fn remove_and_forget(&mut self, entity: Entity) -> Option> { self.sparse.remove(entity.id()).map(|dense_index| { let dense_index = dense_index as usize; #[cfg(debug_assertions)] @@ -230,7 +211,7 @@ impl ComponentSparseSet { }) } - pub fn remove(&mut self, entity: Entity) -> bool { + pub(crate) fn remove(&mut self, entity: Entity) -> bool { if let Some(dense_index) = self.sparse.remove(entity.id()) { let dense_index = dense_index as usize; #[cfg(debug_assertions)] @@ -308,28 +289,6 @@ impl SparseSet { self.indices.push(index); self.dense.push(value); } - - // PERF: switch to this. it's faster but it has an invalid memory access on - // table_add_remove_many let dense = &mut self.dense; - // let indices = &mut self.indices; - // let dense_index = *self.sparse.get_or_insert_with(index.clone(), move || { - // if dense.len() == dense.capacity() { - // dense.reserve(64); - // indices.reserve(64); - // } - // let len = dense.len(); - // // SAFE: we set the index immediately - // unsafe { - // dense.set_len(len + 1); - // indices.set_len(len + 1); - // } - // len - // }); - // // SAFE: index either already existed or was just allocated - // unsafe { - // *self.dense.get_unchecked_mut(dense_index) = value; - // *self.indices.get_unchecked_mut(dense_index) = index; - // } } pub fn get_or_insert_with(&mut self, index: I, func: impl FnOnce() -> V) -> &mut V { diff --git a/crates/bevy_ecs/src/storage/table.rs b/crates/bevy_ecs/src/storage/table.rs index 6150c679c88a9a..37ee9498656c0d 100644 --- a/crates/bevy_ecs/src/storage/table.rs +++ b/crates/bevy_ecs/src/storage/table.rs @@ -1,10 +1,11 @@ use crate::{ component::{ComponentId, ComponentInfo, ComponentTicks, Components}, entity::Entity, - storage::{BlobVec, SparseSet}, + storage::{blob_vec::BlobVec, SparseSet}, }; use bevy_ptr::{OwningPtr, Ptr, PtrMut}; use bevy_utils::HashMap; +use std::alloc::Layout; use std::{ cell::UnsafeCell, ops::{Index, IndexMut}, @@ -32,13 +33,13 @@ impl TableId { #[derive(Debug)] pub struct Column { - pub(crate) data: BlobVec, - pub(crate) ticks: Vec>, + data: BlobVec, + ticks: Vec>, } impl Column { #[inline] - pub fn with_capacity(component_info: &ComponentInfo, capacity: usize) -> Self { + pub(crate) fn with_capacity(component_info: &ComponentInfo, capacity: usize) -> Self { Column { // SAFE: component_info.drop() is valid for the types that will be inserted. data: unsafe { BlobVec::new(component_info.layout(), component_info.drop(), capacity) }, @@ -46,6 +47,11 @@ impl Column { } } + #[inline] + pub fn item_layout(&self) -> Layout { + self.data.layout() + } + /// Writes component data to the column at given row. /// Assumes the slot is uninitialized, drop is not called. /// To overwrite existing initialized value, use `replace` instead. @@ -53,7 +59,12 @@ impl Column { /// # Safety /// Assumes data has already been allocated for the given row. #[inline] - pub unsafe fn initialize(&mut self, row: usize, data: OwningPtr<'_>, ticks: ComponentTicks) { + pub(crate) unsafe fn initialize( + &mut self, + row: usize, + data: OwningPtr<'_>, + ticks: ComponentTicks, + ) { debug_assert!(row < self.len()); self.data.initialize_unchecked(row, data); *self.ticks.get_unchecked_mut(row).get_mut() = ticks; @@ -65,7 +76,7 @@ impl Column { /// # Safety /// Assumes data has already been allocated for the given row. #[inline] - pub unsafe fn replace(&mut self, row: usize, data: OwningPtr<'_>, change_tick: u32) { + pub(crate) unsafe fn replace(&mut self, row: usize, data: OwningPtr<'_>, change_tick: u32) { debug_assert!(row < self.len()); self.data.replace_unchecked(row, data); self.ticks @@ -74,14 +85,6 @@ impl Column { .set_changed(change_tick); } - /// # Safety - /// Assumes data has already been allocated for the given row. - #[inline] - pub unsafe fn initialize_data(&mut self, row: usize, data: OwningPtr<'_>) { - debug_assert!(row < self.len()); - self.data.initialize_unchecked(row, data); - } - #[inline] pub fn len(&self) -> usize { self.data.len() @@ -95,7 +98,7 @@ impl Column { /// # Safety /// index must be in-bounds #[inline] - pub unsafe fn get_ticks_unchecked_mut(&mut self, row: usize) -> &mut ComponentTicks { + pub(crate) unsafe fn get_ticks_unchecked_mut(&mut self, row: usize) -> &mut ComponentTicks { debug_assert!(row < self.len()); self.ticks.get_unchecked_mut(row).get_mut() } @@ -161,7 +164,7 @@ impl Column { /// - index must be in-bounds /// - no other reference to the data of the same row can exist at the same time #[inline] - pub unsafe fn get_data_unchecked_mut(&mut self, row: usize) -> PtrMut<'_> { + pub(crate) unsafe fn get_data_unchecked_mut(&mut self, row: usize) -> PtrMut<'_> { debug_assert!(row < self.data.len()); self.data.get_unchecked_mut(row) } @@ -193,14 +196,7 @@ pub struct Table { } impl Table { - pub const fn new() -> Table { - Self { - columns: SparseSet::new(), - entities: Vec::new(), - } - } - - pub fn with_capacity(capacity: usize, column_capacity: usize) -> Table { + pub(crate) fn with_capacity(capacity: usize, column_capacity: usize) -> Table { Self { columns: SparseSet::with_capacity(column_capacity), entities: Vec::with_capacity(capacity), @@ -212,7 +208,7 @@ impl Table { &self.entities } - pub fn add_column(&mut self, component_info: &ComponentInfo) { + pub(crate) fn add_column(&mut self, component_info: &ComponentInfo) { self.columns.insert( component_info.id(), Column::with_capacity(component_info, self.entities.capacity()), @@ -224,7 +220,7 @@ impl Table { /// /// # Safety /// `row` must be in-bounds - pub unsafe fn swap_remove_unchecked(&mut self, row: usize) -> Option { + pub(crate) unsafe fn swap_remove_unchecked(&mut self, row: usize) -> Option { for column in self.columns.values_mut() { column.swap_remove_unchecked(row); } @@ -244,7 +240,7 @@ impl Table { /// /// # Safety /// Row must be in-bounds - pub unsafe fn move_to_and_forget_missing_unchecked( + pub(crate) unsafe fn move_to_and_forget_missing_unchecked( &mut self, row: usize, new_table: &mut Table, @@ -274,7 +270,7 @@ impl Table { /// /// # Safety /// row must be in-bounds - pub unsafe fn move_to_and_drop_missing_unchecked( + pub(crate) unsafe fn move_to_and_drop_missing_unchecked( &mut self, row: usize, new_table: &mut Table, @@ -306,7 +302,7 @@ impl Table { /// /// # Safety /// `row` must be in-bounds. `new_table` must contain every component this table has - pub unsafe fn move_to_superset_unchecked( + pub(crate) unsafe fn move_to_superset_unchecked( &mut self, row: usize, new_table: &mut Table, @@ -335,7 +331,7 @@ impl Table { } #[inline] - pub fn get_column_mut(&mut self, component_id: ComponentId) -> Option<&mut Column> { + pub(crate) fn get_column_mut(&mut self, component_id: ComponentId) -> Option<&mut Column> { self.columns.get_mut(component_id) } @@ -344,7 +340,7 @@ impl Table { self.columns.contains(component_id) } - pub fn reserve(&mut self, additional: usize) { + pub(crate) fn reserve(&mut self, additional: usize) { if self.entities.capacity() - self.entities.len() < additional { self.entities.reserve(additional); @@ -361,7 +357,7 @@ impl Table { /// /// # Safety /// the allocated row must be written to immediately with valid values in each column - pub unsafe fn allocate(&mut self, entity: Entity) -> usize { + pub(crate) unsafe fn allocate(&mut self, entity: Entity) -> usize { self.reserve(1); let index = self.entities.len(); self.entities.push(entity); @@ -397,7 +393,7 @@ impl Table { self.columns.values() } - pub fn clear(&mut self) { + pub(crate) fn clear(&mut self) { self.entities.clear(); for column in self.columns.values_mut() { column.clear(); @@ -423,7 +419,7 @@ impl Default for Tables { } } -pub struct TableMoveResult { +pub(crate) struct TableMoveResult { pub swapped_entity: Option, pub new_row: usize, } @@ -444,11 +440,6 @@ impl Tables { self.tables.get(id.index()) } - #[inline] - pub fn get_mut(&mut self, id: TableId) -> Option<&mut Table> { - self.tables.get_mut(id.index()) - } - #[inline] pub(crate) fn get_2_mut(&mut self, a: TableId, b: TableId) -> (&mut Table, &mut Table) { if a.index() > b.index() { @@ -462,7 +453,7 @@ impl Tables { /// # Safety /// `component_ids` must contain components that exist in `components` - pub unsafe fn get_id_or_insert( + pub(crate) unsafe fn get_id_or_insert( &mut self, component_ids: &[ComponentId], components: &Components, @@ -488,11 +479,7 @@ impl Tables { self.tables.iter() } - pub fn iter_mut(&mut self) -> std::slice::IterMut<'_, Table> { - self.tables.iter_mut() - } - - pub fn clear(&mut self) { + pub(crate) fn clear(&mut self) { for table in &mut self.tables { table.clear(); } @@ -527,7 +514,11 @@ mod tests { use crate::component::Component; use crate::ptr::OwningPtr; use crate::storage::Storages; - use crate::{component::Components, entity::Entity, storage::Table}; + use crate::{ + component::{ComponentTicks, Components}, + entity::Entity, + storage::Table, + }; #[derive(Component)] struct W(T); @@ -546,10 +537,11 @@ mod tests { let row = table.allocate(*entity); let value: W = W(row); OwningPtr::make(value, |value_ptr| { - table - .get_column_mut(component_id) - .unwrap() - .initialize_data(row, value_ptr); + table.get_column_mut(component_id).unwrap().initialize( + row, + value_ptr, + ComponentTicks::new(0), + ); }); }; } diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 71ee4c7a3716be..b6e447e8dcd84c 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -1196,7 +1196,7 @@ impl World { std::ptr::copy_nonoverlapping::( value.as_ptr(), ptr.as_ptr(), - column.data.layout().size(), + column.item_layout().size(), ); column.get_ticks_unchecked_mut(0).set_changed(change_tick); }