Skip to content

Commit

Permalink
Mark mutable APIs under ECS storage as pub(crate) (bevyengine#5065)
Browse files Browse the repository at this point in the history 
# Objective
Closes bevyengine#1557. Partially addresses bevyengine#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.
  • Loading branch information
@james7132
james7132 committed Jun 22, 2022
1 parent cbda1cb commit 69d8da7
Show file tree
Hide file tree
Showing 6 changed files with 82 additions and 119 deletions.
51 changes: 32 additions & 19 deletions crates/bevy_ecs/src/archetype.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<ComponentStatus>,
pub(crate) bundle_status: Vec<ComponentStatus>,
}

/// 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<BundleId, AddBundle>,
pub remove_bundle: SparseArray<BundleId, Option<ArchetypeId>>,
pub remove_bundle_intersection: SparseArray<BundleId, Option<ArchetypeId>>,
add_bundle: SparseArray<BundleId, AddBundle>,
remove_bundle: SparseArray<BundleId, Option<ArchetypeId>>,
remove_bundle_intersection: SparseArray<BundleId, Option<ArchetypeId>>,
}

impl Edges {
Expand All @@ -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,
Expand All @@ -77,7 +91,11 @@ impl Edges {
}

#[inline]
pub fn insert_remove_bundle(&mut self, bundle_id: BundleId, archetype_id: Option<ArchetypeId>) {
pub(crate) fn insert_remove_bundle(
&mut self,
bundle_id: BundleId,
archetype_id: Option<ArchetypeId>,
) {
self.remove_bundle.insert(bundle_id, archetype_id);
}

Expand All @@ -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<ArchetypeId>,
Expand Down Expand Up @@ -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);

Expand All @@ -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);
}
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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,
Expand Down Expand Up @@ -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();
}
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/src/storage/blob_vec.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 0 additions & 1 deletion crates/bevy_ecs/src/storage/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ mod blob_vec;
mod sparse_set;
mod table;

pub use blob_vec::*;
pub use sparse_set::*;
pub use table::*;

Expand Down
53 changes: 6 additions & 47 deletions crates/bevy_ecs/src/storage/sparse_set.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use std::{cell::UnsafeCell, hash::Hash, marker::PhantomData};
type EntityId = u32;

#[derive(Debug)]
pub struct SparseArray<I, V = I> {
pub(crate) struct SparseArray<I, V = I> {
values: Vec<Option<V>>,
marker: PhantomData<I>,
}
Expand All @@ -31,13 +31,6 @@ impl<I, V> SparseArray<I, V> {
}

impl<I: SparseSetIndex, V> SparseArray<I, V> {
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();
Expand Down Expand Up @@ -74,18 +67,6 @@ impl<I: SparseSetIndex, V> SparseArray<I, V> {
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();
}
Expand All @@ -108,15 +89,15 @@ 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),
sparse: Default::default(),
}
}

pub fn clear(&mut self) {
pub(crate) fn clear(&mut self) {
self.dense.clear();
self.entities.clear();
self.sparse.clear();
Expand All @@ -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]);
Expand Down Expand Up @@ -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<OwningPtr<'_>> {
pub(crate) fn remove_and_forget(&mut self, entity: Entity) -> Option<OwningPtr<'_>> {
self.sparse.remove(entity.id()).map(|dense_index| {
let dense_index = dense_index as usize;
#[cfg(debug_assertions)]
Expand All @@ -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)]
Expand Down Expand Up @@ -308,28 +289,6 @@ impl<I: SparseSetIndex, V> SparseSet<I, V> {
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 {
Expand Down
Loading

0 comments on commit 69d8da7

Please sign in to comment.