diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index 6ec6094ec28649..00f3bac9e527e4 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -140,20 +140,20 @@ impl BundleInfo { // bundle_info.component_ids are also in "bundle order" let mut bundle_component = 0; bundle.get_components(|component_ptr| { - // SAFE: component_id was initialized by get_dynamic_bundle_info let component_id = *self.component_ids.get_unchecked(bundle_component); - let component_status = bundle_status.get_unchecked(bundle_component); match self.storage_types[bundle_component] { StorageType::Table => { let column = table.get_column_mut(component_id).unwrap(); - column.set_data_unchecked(table_row, component_ptr); - let column_status = column.get_ticks_unchecked_mut(table_row); - match component_status { + match bundle_status.get_unchecked(bundle_component) { ComponentStatus::Added => { - *column_status = ComponentTicks::new(change_tick); + column.initialize( + table_row, + component_ptr, + ComponentTicks::new(change_tick), + ); } ComponentStatus::Mutated => { - column_status.set_changed(change_tick); + column.replace(table_row, component_ptr, change_tick); } } } diff --git a/crates/bevy_ecs/src/component/mod.rs b/crates/bevy_ecs/src/component/mod.rs index 3ee166d9c0aeeb..4a392bbe3a8982 100644 --- a/crates/bevy_ecs/src/component/mod.rs +++ b/crates/bevy_ecs/src/component/mod.rs @@ -306,7 +306,7 @@ impl Components { } } -#[derive(Copy, Clone, Debug)] +#[derive(Clone, Debug)] pub struct ComponentTicks { pub(crate) added: u32, pub(crate) changed: u32, diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index f5e01120c1ba7d..a6fdbced0894d9 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -50,13 +50,34 @@ mod tests { }; use bevy_tasks::TaskPool; use parking_lot::Mutex; - use std::{any::TypeId, sync::Arc}; + use std::{ + any::TypeId, + sync::{ + atomic::{AtomicUsize, Ordering}, + Arc, + }, + }; #[derive(Debug, PartialEq, Eq)] struct A(usize); struct B(usize); struct C; + #[derive(Clone, Debug)] + struct DropCk(Arc); + impl DropCk { + fn new_pair() -> (Self, Arc) { + let atomic = Arc::new(AtomicUsize::new(0)); + (DropCk(atomic.clone()), atomic) + } + } + + impl Drop for DropCk { + fn drop(&mut self) { + self.0.as_ref().fetch_add(1, Ordering::Relaxed); + } + } + #[test] fn random_access() { let mut world = World::new(); @@ -1176,4 +1197,33 @@ mod tests { }); assert_eq!(*world.get_resource::().unwrap(), 1); } + + #[test] + fn insert_overwrite_drop() { + let (dropck1, dropped1) = DropCk::new_pair(); + let (dropck2, dropped2) = DropCk::new_pair(); + let mut world = World::default(); + world.spawn().insert(dropck1).insert(dropck2); + assert_eq!(dropped1.load(Ordering::Relaxed), 1); + assert_eq!(dropped2.load(Ordering::Relaxed), 0); + drop(world); + assert_eq!(dropped1.load(Ordering::Relaxed), 1); + assert_eq!(dropped2.load(Ordering::Relaxed), 1); + } + + #[test] + fn insert_overwrite_drop_sparse() { + let (dropck1, dropped1) = DropCk::new_pair(); + let (dropck2, dropped2) = DropCk::new_pair(); + let mut world = World::default(); + world + .register_component(ComponentDescriptor::new::(StorageType::SparseSet)) + .unwrap(); + world.spawn().insert(dropck1).insert(dropck2); + assert_eq!(dropped1.load(Ordering::Relaxed), 1); + assert_eq!(dropped2.load(Ordering::Relaxed), 0); + drop(world); + assert_eq!(dropped1.load(Ordering::Relaxed), 1); + assert_eq!(dropped2.load(Ordering::Relaxed), 1); + } } diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index b24a9e8b900090..2764f897310f27 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -9,6 +9,7 @@ use crate::{ }; use bevy_ecs_macros::all_tuples; use std::{ + cell::UnsafeCell, marker::PhantomData, ptr::{self, NonNull}, }; @@ -343,7 +344,7 @@ impl<'w, T: Component> Fetch<'w> for ReadFetch { let column = tables[archetype.table_id()] .get_column(state.component_id) .unwrap(); - self.table_components = column.get_ptr().cast::(); + self.table_components = column.get_data_ptr().cast::(); } StorageType::SparseSet => self.entities = archetype.entities().as_ptr(), } @@ -354,7 +355,7 @@ impl<'w, T: Component> Fetch<'w> for ReadFetch { self.table_components = table .get_column(state.component_id) .unwrap() - .get_ptr() + .get_data_ptr() .cast::(); } @@ -387,7 +388,7 @@ impl WorldQuery for &mut T { pub struct WriteFetch { storage_type: StorageType, table_components: NonNull, - table_ticks: *mut ComponentTicks, + table_ticks: *const UnsafeCell, entities: *const Entity, entity_table_rows: *const usize, sparse_set: *const ComponentSparseSet, @@ -482,7 +483,7 @@ impl<'w, T: Component> Fetch<'w> for WriteFetch { entities: ptr::null::(), entity_table_rows: ptr::null::(), sparse_set: ptr::null::(), - table_ticks: ptr::null_mut::(), + table_ticks: ptr::null::>(), last_change_tick, change_tick, }; @@ -509,8 +510,8 @@ impl<'w, T: Component> Fetch<'w> for WriteFetch { let column = tables[archetype.table_id()] .get_column(state.component_id) .unwrap(); - self.table_components = column.get_ptr().cast::(); - self.table_ticks = column.get_ticks_mut_ptr(); + self.table_components = column.get_data_ptr().cast::(); + self.table_ticks = column.get_ticks_ptr(); } StorageType::SparseSet => self.entities = archetype.entities().as_ptr(), } @@ -519,8 +520,8 @@ impl<'w, T: Component> Fetch<'w> for WriteFetch { #[inline] unsafe fn set_table(&mut self, state: &Self::State, table: &Table) { let column = table.get_column(state.component_id).unwrap(); - self.table_components = column.get_ptr().cast::(); - self.table_ticks = column.get_ticks_mut_ptr(); + self.table_components = column.get_data_ptr().cast::(); + self.table_ticks = column.get_ticks_ptr(); } #[inline] @@ -531,7 +532,7 @@ impl<'w, T: Component> Fetch<'w> for WriteFetch { Mut { value: &mut *self.table_components.as_ptr().add(table_row), ticks: Ticks { - component_ticks: &mut *self.table_ticks.add(table_row), + component_ticks: &mut *(&*self.table_ticks.add(table_row)).get(), change_tick: self.change_tick, last_change_tick: self.last_change_tick, }, @@ -558,7 +559,7 @@ impl<'w, T: Component> Fetch<'w> for WriteFetch { Mut { value: &mut *self.table_components.as_ptr().add(table_row), ticks: Ticks { - component_ticks: &mut *self.table_ticks.add(table_row), + component_ticks: &mut *(&*self.table_ticks.add(table_row)).get(), change_tick: self.change_tick, last_change_tick: self.last_change_tick, }, @@ -860,7 +861,7 @@ impl<'w, T: Component> Fetch<'w> for ChangeTrackersFetch { let column = tables[archetype.table_id()] .get_column(state.component_id) .unwrap(); - self.table_ticks = column.get_ticks_mut_ptr().cast::(); + self.table_ticks = column.get_ticks_const_ptr(); } StorageType::SparseSet => self.entities = archetype.entities().as_ptr(), } @@ -871,8 +872,7 @@ impl<'w, T: Component> Fetch<'w> for ChangeTrackersFetch { self.table_ticks = table .get_column(state.component_id) .unwrap() - .get_ticks_mut_ptr() - .cast::(); + .get_ticks_const_ptr(); } #[inline] @@ -881,7 +881,7 @@ impl<'w, T: Component> Fetch<'w> for ChangeTrackersFetch { StorageType::Table => { let table_row = *self.entity_table_rows.add(archetype_index); ChangeTrackers { - component_ticks: *self.table_ticks.add(table_row), + component_ticks: (&*self.table_ticks.add(table_row)).clone(), marker: PhantomData, last_change_tick: self.last_change_tick, change_tick: self.change_tick, @@ -890,7 +890,7 @@ impl<'w, T: Component> Fetch<'w> for ChangeTrackersFetch { StorageType::SparseSet => { let entity = *self.entities.add(archetype_index); ChangeTrackers { - component_ticks: *(*self.sparse_set).get_ticks(entity).unwrap(), + component_ticks: (&*self.sparse_set).get_ticks(entity).cloned().unwrap(), marker: PhantomData, last_change_tick: self.last_change_tick, change_tick: self.change_tick, @@ -902,7 +902,7 @@ impl<'w, T: Component> Fetch<'w> for ChangeTrackersFetch { #[inline] unsafe fn table_fetch(&mut self, table_row: usize) -> Self::Item { ChangeTrackers { - component_ticks: *self.table_ticks.add(table_row), + component_ticks: (&*self.table_ticks.add(table_row)).clone(), marker: PhantomData, last_change_tick: self.last_change_tick, change_tick: self.change_tick, diff --git a/crates/bevy_ecs/src/query/filter.rs b/crates/bevy_ecs/src/query/filter.rs index 2d51028311674e..77d23c2cb3986b 100644 --- a/crates/bevy_ecs/src/query/filter.rs +++ b/crates/bevy_ecs/src/query/filter.rs @@ -8,7 +8,7 @@ use crate::{ world::World, }; use bevy_ecs_macros::all_tuples; -use std::{marker::PhantomData, ptr}; +use std::{cell::UnsafeCell, marker::PhantomData, ptr}; // TODO: uncomment this and use as shorthand (remove where F::Fetch: FilterFetch everywhere) when // this bug is fixed in Rust 1.51: https://github.com/rust-lang/rust/pull/81671 @@ -561,7 +561,7 @@ macro_rules! impl_tick_filter { $(#[$fetch_meta])* pub struct $fetch_name { storage_type: StorageType, - table_ticks: *mut ComponentTicks, + table_ticks: *const UnsafeCell, entity_table_rows: *const usize, marker: PhantomData, entities: *const Entity, @@ -630,7 +630,7 @@ macro_rules! impl_tick_filter { unsafe fn init(world: &World, state: &Self::State, last_change_tick: u32, change_tick: u32) -> Self { let mut value = Self { storage_type: state.storage_type, - table_ticks: ptr::null_mut::(), + table_ticks: ptr::null::>(), entities: ptr::null::(), entity_table_rows: ptr::null::(), sparse_set: ptr::null::(), @@ -655,7 +655,7 @@ macro_rules! impl_tick_filter { unsafe fn set_table(&mut self, state: &Self::State, table: &Table) { self.table_ticks = table .get_column(state.component_id).unwrap() - .get_ticks_mut_ptr(); + .get_ticks_ptr(); } unsafe fn set_archetype(&mut self, state: &Self::State, archetype: &Archetype, tables: &Tables) { @@ -665,25 +665,25 @@ macro_rules! impl_tick_filter { let table = &tables[archetype.table_id()]; self.table_ticks = table .get_column(state.component_id).unwrap() - .get_ticks_mut_ptr(); + .get_ticks_ptr(); } StorageType::SparseSet => self.entities = archetype.entities().as_ptr(), } } unsafe fn table_fetch(&mut self, table_row: usize) -> bool { - $is_detected(&*self.table_ticks.add(table_row), self.last_change_tick, self.change_tick) + $is_detected(&*(&*self.table_ticks.add(table_row)).get(), self.last_change_tick, self.change_tick) } unsafe fn archetype_fetch(&mut self, archetype_index: usize) -> bool { match self.storage_type { StorageType::Table => { let table_row = *self.entity_table_rows.add(archetype_index); - $is_detected(&*self.table_ticks.add(table_row), self.last_change_tick, self.change_tick) + $is_detected(&*(&*self.table_ticks.add(table_row)).get(), self.last_change_tick, self.change_tick) } StorageType::SparseSet => { let entity = *self.entities.add(archetype_index); - let ticks = (*(*self.sparse_set).get_ticks(entity).unwrap()); + let ticks = (&*self.sparse_set).get_ticks(entity).cloned().unwrap(); $is_detected(&ticks, self.last_change_tick, self.change_tick) } } diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs index 0c0fd4ac2218fc..7469355cb78c64 100644 --- a/crates/bevy_ecs/src/storage/blob_vec.rs +++ b/crates/bevy_ecs/src/storage/blob_vec.rs @@ -86,15 +86,25 @@ impl BlobVec { } /// # Safety - /// `index` must be in bounds - /// Allows aliased mutable access to `index`'s data. Caller must ensure this does not happen + /// - index must be in bounds + /// - memory must be reserved and uninitialized #[inline] - pub unsafe fn set_unchecked(&self, index: usize, value: *mut u8) { + pub unsafe fn initialize_unchecked(&mut self, index: usize, value: *mut u8) { debug_assert!(index < self.len()); let ptr = self.get_unchecked(index); std::ptr::copy_nonoverlapping(value, ptr, self.item_layout.size()); } + /// # Safety + /// - index must be in-bounds + // - memory must be previously initialized + pub unsafe fn replace_unchecked(&mut self, index: usize, value: *mut u8) { + debug_assert!(index < self.len()); + let ptr = self.get_unchecked(index); + (self.drop)(ptr); + std::ptr::copy_nonoverlapping(value, ptr, self.item_layout.size()); + } + /// increases the length by one (and grows the vec if needed) with uninitialized memory and /// returns the index /// @@ -267,7 +277,7 @@ mod tests { /// `blob_vec` must have a layout that matches Layout::new::() unsafe fn push(blob_vec: &mut BlobVec, mut value: T) { let index = blob_vec.push_uninit(); - blob_vec.set_unchecked(index, (&mut value as *mut T).cast::()); + blob_vec.initialize_unchecked(index, (&mut value as *mut T).cast::()); std::mem::forget(value); } diff --git a/crates/bevy_ecs/src/storage/sparse_set.rs b/crates/bevy_ecs/src/storage/sparse_set.rs index 8d6d9edc1825bd..6d0b85ffbbe902 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -87,7 +87,7 @@ impl SparseArray { #[derive(Debug)] pub struct ComponentSparseSet { dense: BlobVec, - ticks: UnsafeCell>, + ticks: Vec>, entities: Vec, sparse: SparseArray, } @@ -96,7 +96,7 @@ impl ComponentSparseSet { pub fn new(component_info: &ComponentInfo, capacity: usize) -> Self { Self { dense: BlobVec::new(component_info.layout(), component_info.drop(), capacity), - ticks: UnsafeCell::new(Vec::with_capacity(capacity)), + ticks: Vec::with_capacity(capacity), entities: Vec::with_capacity(capacity), sparse: Default::default(), } @@ -120,17 +120,20 @@ impl ComponentSparseSet { /// The `value` pointer must point to a valid address that matches the `Layout` /// inside the `ComponentInfo` given when constructing this sparse set. pub unsafe fn insert(&mut self, entity: Entity, value: *mut u8, change_tick: u32) { - let dense = &mut self.dense; - let entities = &mut self.entities; - let ticks_list = self.ticks.get_mut(); - let dense_index = *self.sparse.get_or_insert_with(entity, move || { - ticks_list.push(ComponentTicks::new(change_tick)); - entities.push(entity); - dense.push_uninit() - }); - // SAFE: dense_index exists thanks to the call above - self.dense.set_unchecked(dense_index, value); - ((*self.ticks.get()).get_unchecked_mut(dense_index)).set_changed(change_tick); + if let Some(&dense_index) = self.sparse.get(entity) { + self.dense.replace_unchecked(dense_index, value); + *self.ticks.get_unchecked_mut(dense_index) = + UnsafeCell::new(ComponentTicks::new(change_tick)); + } else { + let dense_index = self.dense.push_uninit(); + self.dense.initialize_unchecked(dense_index, value); + self.sparse.insert(entity, dense_index); + debug_assert_eq!(self.ticks.len(), dense_index); + debug_assert_eq!(self.entities.len(), dense_index); + self.ticks + .push(UnsafeCell::new(ComponentTicks::new(change_tick))); + self.entities.push(entity); + } } #[inline] @@ -152,27 +155,19 @@ impl ComponentSparseSet { /// ensure the same entity is not accessed twice at the same time #[inline] pub unsafe fn get_with_ticks(&self, entity: Entity) -> Option<(*mut u8, *mut ComponentTicks)> { - let ticks = &mut *self.ticks.get(); - self.sparse.get(entity).map(move |dense_index| { - let dense_index = *dense_index; - // SAFE: if the sparse index points to something in the dense vec, it exists - ( - self.dense.get_unchecked(dense_index), - ticks.get_unchecked_mut(dense_index) as *mut ComponentTicks, - ) - }) + let dense_index = *self.sparse.get(entity)?; + // SAFE: if the sparse index points to something in the dense vec, it exists + Some(( + self.dense.get_unchecked(dense_index), + self.ticks.get_unchecked(dense_index).get(), + )) } - /// # Safety - /// ensure the same entity is not accessed twice at the same time #[inline] - pub unsafe fn get_ticks(&self, entity: Entity) -> Option<&mut ComponentTicks> { - let ticks = &mut *self.ticks.get(); - self.sparse.get(entity).map(move |dense_index| { - let dense_index = *dense_index; - // SAFE: if the sparse index points to something in the dense vec, it exists - ticks.get_unchecked_mut(dense_index) - }) + pub fn get_ticks(&self, entity: Entity) -> Option<&ComponentTicks> { + let dense_index = *self.sparse.get(entity)?; + // SAFE: if the sparse index points to something in the dense vec, it exists + unsafe { Some(&*self.ticks.get_unchecked(dense_index).get()) } } /// Removes the `entity` from this sparse set and returns a pointer to the associated value (if @@ -180,7 +175,7 @@ impl ComponentSparseSet { /// returned). pub fn remove_and_forget(&mut self, entity: Entity) -> Option<*mut u8> { self.sparse.remove(entity).map(|dense_index| { - self.ticks.get_mut().swap_remove(dense_index); + self.ticks.swap_remove(dense_index); self.entities.swap_remove(dense_index); let is_last = dense_index == self.dense.len() - 1; // SAFE: dense_index was just removed from `sparse`, which ensures that it is valid @@ -195,7 +190,7 @@ impl ComponentSparseSet { pub fn remove(&mut self, entity: Entity) -> bool { if let Some(dense_index) = self.sparse.remove(entity) { - self.ticks.get_mut().swap_remove(dense_index); + self.ticks.swap_remove(dense_index); self.entities.swap_remove(dense_index); let is_last = dense_index == self.dense.len() - 1; // SAFE: if the sparse index points to something in the dense vec, it exists @@ -211,9 +206,8 @@ impl ComponentSparseSet { } pub(crate) fn check_change_ticks(&mut self, change_tick: u32) { - let ticks = self.ticks.get_mut().iter_mut(); - for component_ticks in ticks { - component_ticks.check_ticks(change_tick); + for component_ticks in &mut self.ticks { + component_ticks.get_mut().check_ticks(change_tick); } } } diff --git a/crates/bevy_ecs/src/storage/table.rs b/crates/bevy_ecs/src/storage/table.rs index 9d95d234cb3941..8fc334b56975a3 100644 --- a/crates/bevy_ecs/src/storage/table.rs +++ b/crates/bevy_ecs/src/storage/table.rs @@ -34,7 +34,7 @@ impl TableId { pub struct Column { pub(crate) component_id: ComponentId, pub(crate) data: BlobVec, - pub(crate) ticks: UnsafeCell>, + pub(crate) ticks: Vec>, } impl Column { @@ -43,29 +43,44 @@ impl Column { Column { component_id: component_info.id(), data: BlobVec::new(component_info.layout(), component_info.drop(), capacity), - ticks: UnsafeCell::new(Vec::with_capacity(capacity)), + ticks: Vec::with_capacity(capacity), } } + /// 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. + /// + /// # Safety + /// Assumes data has already been allocated for the given row. #[inline] - fn ticks_mut(&mut self) -> &mut Vec { - self.ticks.get_mut() + pub unsafe fn initialize(&mut self, row: usize, data: *mut u8, ticks: ComponentTicks) { + debug_assert!(row < self.len()); + self.data.initialize_unchecked(row, data); + *self.ticks.get_unchecked_mut(row).get_mut() = ticks; } + /// Writes component data to the column at given row. + /// Assumes the slot is initialized, calls drop. + /// /// # Safety /// Assumes data has already been allocated for the given row. #[inline] - pub unsafe fn set_unchecked(&mut self, row: usize, data: *mut u8, ticks: ComponentTicks) { - self.set_data_unchecked(row, data); - *self.ticks_mut().get_unchecked_mut(row) = ticks; + pub unsafe fn replace(&mut self, row: usize, data: *mut u8, change_tick: u32) { + debug_assert!(row < self.len()); + self.data.replace_unchecked(row, data); + self.ticks + .get_unchecked_mut(row) + .get_mut() + .set_changed(change_tick); } /// # Safety /// Assumes data has already been allocated for the given row. #[inline] - pub unsafe fn set_data_unchecked(&mut self, row: usize, data: *mut u8) { + pub unsafe fn initialize_data(&mut self, row: usize, data: *mut u8) { debug_assert!(row < self.len()); - self.data.set_unchecked(row, data); + self.data.initialize_unchecked(row, data); } #[inline] @@ -79,11 +94,11 @@ impl Column { } /// # Safety - /// Assumes data has already been allocated for the given row. + /// index must be in-bounds #[inline] pub unsafe fn get_ticks_unchecked_mut(&mut self, row: usize) -> &mut ComponentTicks { debug_assert!(row < self.len()); - self.ticks_mut().get_unchecked_mut(row) + self.ticks.get_unchecked_mut(row).get_mut() } /// # Safety @@ -91,7 +106,7 @@ impl Column { #[inline] pub(crate) unsafe fn swap_remove_unchecked(&mut self, row: usize) { self.data.swap_remove_and_drop_unchecked(row); - self.ticks_mut().swap_remove(row); + self.ticks.swap_remove(row); } #[inline] @@ -100,7 +115,7 @@ impl Column { row: usize, ) -> (*mut u8, ComponentTicks) { let data = self.data.swap_remove_and_forget_unchecked(row); - let ticks = self.ticks_mut().swap_remove(row); + let ticks = self.ticks.swap_remove(row).into_inner(); (data, ticks) } @@ -108,50 +123,66 @@ impl Column { // - ptr must point to valid data of this column's component type pub(crate) unsafe fn push(&mut self, ptr: *mut u8, ticks: ComponentTicks) { let row = self.data.push_uninit(); - self.data.set_unchecked(row, ptr); - self.ticks_mut().push(ticks); + self.data.initialize_unchecked(row, ptr); + self.ticks.push(UnsafeCell::new(ticks)); } #[inline] pub(crate) fn reserve_exact(&mut self, additional: usize) { self.data.reserve_exact(additional); - self.ticks_mut().reserve_exact(additional); + self.ticks.reserve_exact(additional); } /// # Safety /// must ensure rust mutability rules are not violated #[inline] - pub unsafe fn get_ptr(&self) -> NonNull { + pub unsafe fn get_data_ptr(&self) -> NonNull { self.data.get_ptr() } - /// # Safety - /// must ensure rust mutability rules are not violated #[inline] - pub unsafe fn get_ticks_mut_ptr(&self) -> *mut ComponentTicks { - (*self.ticks.get()).as_mut_ptr() + pub fn get_ticks_ptr(&self) -> *const UnsafeCell { + self.ticks.as_ptr() + } + + #[inline] + pub fn get_ticks_const_ptr(&self) -> *const ComponentTicks { + // cast is valid, because UnsafeCell is repr(transparent) + self.get_ticks_ptr() as *const ComponentTicks } /// # Safety - /// must ensure rust mutability rules are not violated + /// - index must be in-bounds + /// - no other reference to the data of the same row can exist at the same time + /// - pointer cannot be dereferenced after mutable reference to this `Column` was live #[inline] - pub unsafe fn get_unchecked(&self, row: usize) -> *mut u8 { + pub unsafe fn get_data_unchecked(&self, row: usize) -> *mut u8 { debug_assert!(row < self.data.len()); self.data.get_unchecked(row) } /// # Safety - /// must ensure rust mutability rules are not violated + /// index must be in-bounds + #[inline] + pub unsafe fn get_ticks_unchecked(&self, row: usize) -> &ComponentTicks { + debug_assert!(row < self.ticks.len()); + &*self.ticks.get_unchecked(row).get() + } + + /// # Safety + /// - index must be in-bounds + /// - no other reference to the ticks of the same row can exist at the same time + /// - pointer cannot be dereferenced after mutable reference to this column was live #[inline] - pub unsafe fn get_ticks_unchecked(&self, row: usize) -> *mut ComponentTicks { - debug_assert!(row < (*self.ticks.get()).len()); - self.get_ticks_mut_ptr().add(row) + pub unsafe fn get_ticks_mut_ptr_unchecked(&self, row: usize) -> *mut ComponentTicks { + debug_assert!(row < self.ticks.len()); + self.ticks.get_unchecked(row).get() } #[inline] pub(crate) fn check_change_ticks(&mut self, change_tick: u32) { - for component_ticks in self.ticks_mut() { - component_ticks.check_ticks(change_tick); + for component_ticks in &mut self.ticks { + component_ticks.get_mut().check_ticks(change_tick); } } } @@ -224,7 +255,7 @@ impl Table { for column in self.columns.values_mut() { let (data, ticks) = column.swap_remove_and_forget_unchecked(row); if let Some(new_column) = new_table.get_column_mut(column.component_id) { - new_column.set_unchecked(new_row, data, ticks); + new_column.initialize(new_row, data, ticks); } } TableMoveResult { @@ -254,7 +285,7 @@ impl Table { for column in self.columns.values_mut() { if let Some(new_column) = new_table.get_column_mut(column.component_id) { let (data, ticks) = column.swap_remove_and_forget_unchecked(row); - new_column.set_unchecked(new_row, data, ticks); + new_column.initialize(new_row, data, ticks); } else { column.swap_remove_unchecked(row); } @@ -286,7 +317,7 @@ impl Table { for column in self.columns.values_mut() { let new_column = new_table.get_column_mut(column.component_id).unwrap(); let (data, ticks) = column.swap_remove_and_forget_unchecked(row); - new_column.set_unchecked(new_row, data, ticks); + new_column.initialize(new_row, data, ticks); } TableMoveResult { new_row, @@ -336,7 +367,7 @@ impl Table { self.entities.push(entity); for column in self.columns.values_mut() { column.data.set_len(self.entities.len()); - (*column.ticks.get()).push(ComponentTicks::new(0)); + column.ticks.push(UnsafeCell::new(ComponentTicks::new(0))); } index } @@ -493,7 +524,7 @@ mod tests { table .get_column_mut(component_id) .unwrap() - .set_data_unchecked(row, value_ptr); + .initialize_data(row, value_ptr); }; } diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index eb7bbcc1a64c8b..07a8499971396f 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -289,8 +289,8 @@ impl<'a, T: Component> SystemParamFetch<'a> for ResState { ) }); Res { - value: &*column.get_ptr().as_ptr().cast::(), - ticks: &*column.get_ticks_mut_ptr(), + value: &*column.get_data_ptr().cast::().as_ptr(), + ticks: column.get_ticks_unchecked(0), last_change_tick: system_state.last_change_tick, change_tick, } @@ -327,8 +327,8 @@ impl<'a, T: Component> SystemParamFetch<'a> for OptionResState { world .get_populated_resource_column(state.0.component_id) .map(|column| Res { - value: &*column.get_ptr().as_ptr().cast::(), - ticks: &*column.get_ticks_mut_ptr(), + value: &*column.get_data_ptr().cast::().as_ptr(), + ticks: column.get_ticks_unchecked(0), last_change_tick: system_state.last_change_tick, change_tick, }) @@ -756,9 +756,10 @@ impl<'a, T: 'static> SystemParamFetch<'a> for NonSendState { std::any::type_name::() ) }); + NonSend { - value: &*column.get_ptr().as_ptr().cast::(), - ticks: *column.get_ticks_mut_ptr(), + value: &*column.get_data_ptr().cast::().as_ptr(), + ticks: column.get_ticks_unchecked(0).clone(), last_change_tick: system_state.last_change_tick, change_tick, } @@ -888,8 +889,8 @@ impl<'a, T: 'static> SystemParamFetch<'a> for NonSendMutState { ) }); NonSendMut { - value: &mut *column.get_ptr().as_ptr().cast::(), - ticks: &mut *column.get_ticks_mut_ptr(), + value: &mut *column.get_data_ptr().cast::().as_ptr(), + ticks: &mut *column.get_ticks_mut_ptr_unchecked(0), last_change_tick: system_state.last_change_tick, change_tick, } diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index cb4a10b66b1186..1815d15798f433 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -191,16 +191,6 @@ impl<'w> EntityMut<'w> { // TODO: move relevant methods to World (add/remove bundle) pub fn insert_bundle(&mut self, bundle: T) -> &mut Self { - let entity = self.entity; - let change_tick = self.world.change_tick(); - let entities = &mut self.world.entities; - let archetypes = &mut self.world.archetypes; - let components = &mut self.world.components; - let storages = &mut self.world.storages; - - let bundle_info = self.world.bundles.init_info::(components); - let current_location = self.location; - // Use a non-generic function to cut down on monomorphization unsafe fn get_insert_bundle_info<'a>( entities: &mut Entities, @@ -269,26 +259,32 @@ impl<'w> EntityMut<'w> { } } + let change_tick = self.world.change_tick(); + let bundle_info = self + .world + .bundles + .init_info::(&mut self.world.components); + let (archetype, bundle_status, new_location) = unsafe { get_insert_bundle_info( - entities, - archetypes, - components, - storages, + &mut self.world.entities, + &mut self.world.archetypes, + &mut self.world.components, + &mut self.world.storages, bundle_info, - current_location, - entity, + self.location, + self.entity, ) }; self.location = new_location; - let table = &mut storages.tables[archetype.table_id()]; + let table = &mut self.world.storages.tables[archetype.table_id()]; let table_row = archetype.entity_table_row(new_location.index); // SAFE: table row is valid unsafe { bundle_info.write_components( - &mut storages.sparse_sets, - entity, + &mut self.world.storages.sparse_sets, + self.entity, table, table_row, bundle_status, @@ -554,7 +550,7 @@ unsafe fn get_component( let components = table.get_column(component_id)?; let table_row = archetype.entity_table_row(location.index); // SAFE: archetypes only store valid table_rows and the stored component type is T - Some(components.get_unchecked(table_row)) + Some(components.get_data_unchecked(table_row)) } StorageType::SparseSet => world .storages @@ -582,8 +578,8 @@ unsafe fn get_component_and_ticks( let table_row = archetype.entity_table_row(location.index); // SAFE: archetypes only store valid table_rows and the stored component type is T Some(( - components.get_unchecked(table_row), - components.get_ticks_unchecked(table_row), + components.get_data_unchecked(table_row), + components.get_ticks_mut_ptr_unchecked(table_row), )) } StorageType::SparseSet => world @@ -624,7 +620,7 @@ unsafe fn take_component( let components = table.get_column(component_id).unwrap(); let table_row = archetype.entity_table_row(location.index); // SAFE: archetypes only store valid table_rows and the stored component type is T - components.get_unchecked(table_row) + components.get_data_unchecked(table_row) } StorageType::SparseSet => storages .sparse_sets diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index f549ff791afa68..5068fdc4e50745 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -594,14 +594,16 @@ impl World { pub fn is_resource_added(&self) -> bool { let component_id = self.components.get_resource_id(TypeId::of::()).unwrap(); let column = self.get_populated_resource_column(component_id).unwrap(); - let ticks = unsafe { &*column.get_ticks_mut_ptr() }; + // SAFE: resources table always have row 0 + let ticks = unsafe { column.get_ticks_unchecked(0) }; ticks.is_added(self.last_change_tick(), self.read_change_tick()) } pub fn is_resource_changed(&self) -> bool { let component_id = self.components.get_resource_id(TypeId::of::()).unwrap(); let column = self.get_populated_resource_column(component_id).unwrap(); - let ticks = unsafe { &*column.get_ticks_mut_ptr() }; + // SAFE: resources table always have row 0 + let ticks = unsafe { column.get_ticks_unchecked(0) }; ticks.is_changed(self.last_change_tick(), self.read_change_tick()) } @@ -736,7 +738,7 @@ impl World { component_id: ComponentId, ) -> Option<&T> { let column = self.get_populated_resource_column(component_id)?; - Some(&*column.get_ptr().as_ptr().cast::()) + Some(&*column.get_data_ptr().as_ptr().cast::()) } /// # Safety @@ -749,9 +751,9 @@ impl World { ) -> Option> { let column = self.get_populated_resource_column(component_id)?; Some(Mut { - value: &mut *column.get_ptr().as_ptr().cast::(), + value: &mut *column.get_data_ptr().cast::().as_ptr(), ticks: Ticks { - component_ticks: &mut *column.get_ticks_mut_ptr(), + component_ticks: &mut *column.get_ticks_mut_ptr_unchecked(0), last_change_tick: self.last_change_tick(), change_tick: self.read_change_tick(), }, @@ -794,7 +796,7 @@ impl World { column.push(data, ComponentTicks::new(change_tick)); } else { // SAFE: column is of type T and has already been allocated - *column.get_unchecked(0).cast::() = value; + *column.get_data_unchecked(0).cast::() = value; column.get_ticks_unchecked_mut(0).set_changed(change_tick); } } diff --git a/crates/bevy_transform/src/hierarchy/child_builder.rs b/crates/bevy_transform/src/hierarchy/child_builder.rs index 0a97c4a8503822..1c1ab208eefa4e 100644 --- a/crates/bevy_transform/src/hierarchy/child_builder.rs +++ b/crates/bevy_transform/src/hierarchy/child_builder.rs @@ -23,15 +23,9 @@ impl Command for InsertChildren { .insert_bundle((Parent(self.parent), PreviousParent(self.parent))); } { - let mut added = false; if let Some(mut children) = world.get_mut::(self.parent) { children.0.insert_from_slice(self.index, &self.children); - added = true; - } - - // NOTE: ideally this is just an else statement, but currently that _incorrectly_ fails - // borrow-checking - if !added { + } else { world .entity_mut(self.parent) .insert(Children(self.children));