From 530be10e72aa2b8191fe680e9ffef5c4576ce961 Mon Sep 17 00:00:00 2001 From: James Liu Date: Tue, 6 Dec 2022 01:38:21 +0000 Subject: [PATCH] Newtype ArchetypeRow and TableRow (#4878) # Objective Prevent future unsoundness that was seen in #6623. ## Solution Newtype both indexes in `Archetype` and `Table` as `ArchetypeRow` and `TableRow`. This avoids weird numerical manipulation on the indices, and can be stored and treated opaquely. Also enforces the source and destination of where these indices at a type level. --- ## Changelog Changed: `Archetype` indices and `Table` rows have been newtyped as `ArchetypeRow` and `TableRow`. --- crates/bevy_ecs/macros/src/fetch.rs | 4 +- crates/bevy_ecs/src/archetype.rs | 63 +++++-- crates/bevy_ecs/src/bundle.rs | 20 +- crates/bevy_ecs/src/entity/mod.rs | 9 +- crates/bevy_ecs/src/query/fetch.rs | 42 +++-- crates/bevy_ecs/src/query/filter.rs | 16 +- crates/bevy_ecs/src/query/iter.rs | 59 +++--- crates/bevy_ecs/src/query/state.rs | 10 +- crates/bevy_ecs/src/storage/resource.rs | 24 ++- crates/bevy_ecs/src/storage/sparse_set.rs | 36 ++-- crates/bevy_ecs/src/storage/table.rs | 211 +++++++++++++--------- crates/bevy_ecs/src/world/entity_ref.rs | 19 +- crates/bevy_ecs/src/world/mod.rs | 10 +- 13 files changed, 310 insertions(+), 213 deletions(-) diff --git a/crates/bevy_ecs/macros/src/fetch.rs b/crates/bevy_ecs/macros/src/fetch.rs index 4d04d1812b..a610205acc 100644 --- a/crates/bevy_ecs/macros/src/fetch.rs +++ b/crates/bevy_ecs/macros/src/fetch.rs @@ -283,7 +283,7 @@ pub fn derive_world_query_impl(ast: DeriveInput) -> TokenStream { unsafe fn fetch<'__w>( _fetch: &mut ::Fetch<'__w>, _entity: #path::entity::Entity, - _table_row: usize + _table_row: #path::storage::TableRow, ) -> ::Item<'__w> { Self::Item { #(#field_idents: <#field_types>::fetch(&mut _fetch.#field_idents, _entity, _table_row),)* @@ -296,7 +296,7 @@ pub fn derive_world_query_impl(ast: DeriveInput) -> TokenStream { unsafe fn filter_fetch<'__w>( _fetch: &mut ::Fetch<'__w>, _entity: #path::entity::Entity, - _table_row: usize + _table_row: #path::storage::TableRow, ) -> bool { true #(&& <#field_types>::filter_fetch(&mut _fetch.#field_idents, _entity, _table_row))* } diff --git a/crates/bevy_ecs/src/archetype.rs b/crates/bevy_ecs/src/archetype.rs index 2cb0b47eef..705ca64a7f 100644 --- a/crates/bevy_ecs/src/archetype.rs +++ b/crates/bevy_ecs/src/archetype.rs @@ -23,7 +23,7 @@ use crate::{ bundle::BundleId, component::{ComponentId, StorageType}, entity::{Entity, EntityLocation}, - storage::{ImmutableSparseSet, SparseArray, SparseSet, SparseSetIndex, TableId}, + storage::{ImmutableSparseSet, SparseArray, SparseSet, SparseSetIndex, TableId, TableRow}, }; use std::{ collections::HashMap, @@ -31,6 +31,33 @@ use std::{ ops::{Index, IndexMut}, }; +/// An opaque location within a [`Archetype`]. +/// +/// This can be used in conjunction with [`ArchetypeId`] to find the exact location +/// of an [`Entity`] within a [`World`]. An entity's archetype and index can be +/// retrieved via [`Entities::get`]. +/// +/// [`World`]: crate::world::World +/// [`Entities::get`]: crate::entity::Entities +#[derive(Debug, Copy, Clone, Eq, PartialEq)] +#[repr(transparent)] +pub struct ArchetypeRow(usize); + +impl ArchetypeRow { + pub const INVALID: ArchetypeRow = ArchetypeRow(usize::MAX); + + /// Creates a `ArchetypeRow`. + pub const fn new(index: usize) -> Self { + Self(index) + } + + /// Gets the index of the row. + #[inline] + pub const fn index(self) -> usize { + self.0 + } +} + /// An opaque unique ID for a single [`Archetype`] within a [`World`]. /// /// Archetype IDs are only valid for a given World, and are not globally unique. @@ -226,7 +253,7 @@ impl Edges { /// Metadata about an [`Entity`] in a [`Archetype`]. pub struct ArchetypeEntity { entity: Entity, - table_row: usize, + table_row: TableRow, } impl ArchetypeEntity { @@ -240,14 +267,14 @@ impl ArchetypeEntity { /// /// [`Table`]: crate::storage::Table #[inline] - pub const fn table_row(&self) -> usize { + pub const fn table_row(&self) -> TableRow { self.table_row } } pub(crate) struct ArchetypeSwapRemoveResult { pub(crate) swapped_entity: Option, - pub(crate) table_row: usize, + pub(crate) table_row: TableRow, } /// Internal metadata for a [`Component`] within a given [`Archetype`]. @@ -380,18 +407,18 @@ impl Archetype { /// Fetches the row in the [`Table`] where the components for the entity at `index` /// is stored. /// - /// An entity's archetype index can be fetched from [`EntityLocation::index`], which + /// An entity's archetype index can be fetched from [`EntityLocation::archetype_row`], which /// can be retrieved from [`Entities::get`]. /// /// # Panics /// This function will panic if `index >= self.len()`. /// /// [`Table`]: crate::storage::Table - /// [`EntityLocation`]: crate::entity::EntityLocation::index + /// [`EntityLocation`]: crate::entity::EntityLocation::archetype_row /// [`Entities::get`]: crate::entity::Entities::get #[inline] - pub fn entity_table_row(&self, index: usize) -> usize { - self.entities[index].table_row + pub fn entity_table_row(&self, index: ArchetypeRow) -> TableRow { + self.entities[index.0].table_row } /// Updates if the components for the entity at `index` can be found @@ -400,8 +427,8 @@ impl Archetype { /// # Panics /// This function will panic if `index >= self.len()`. #[inline] - pub(crate) fn set_entity_table_row(&mut self, index: usize, table_row: usize) { - self.entities[index].table_row = table_row; + pub(crate) fn set_entity_table_row(&mut self, index: ArchetypeRow, table_row: TableRow) { + self.entities[index.0].table_row = table_row; } /// Allocates an entity to the archetype. @@ -409,12 +436,16 @@ impl Archetype { /// # Safety /// valid component values must be immediately written to the relevant storages /// `table_row` must be valid - pub(crate) unsafe fn allocate(&mut self, entity: Entity, table_row: usize) -> EntityLocation { + pub(crate) unsafe fn allocate( + &mut self, + entity: Entity, + table_row: TableRow, + ) -> EntityLocation { self.entities.push(ArchetypeEntity { entity, table_row }); EntityLocation { archetype_id: self.id, - index: self.entities.len() - 1, + archetype_row: ArchetypeRow(self.entities.len() - 1), } } @@ -427,14 +458,14 @@ impl Archetype { /// /// # Panics /// This function will panic if `index >= self.len()` - pub(crate) fn swap_remove(&mut self, index: usize) -> ArchetypeSwapRemoveResult { - let is_last = index == self.entities.len() - 1; - let entity = self.entities.swap_remove(index); + pub(crate) fn swap_remove(&mut self, index: ArchetypeRow) -> ArchetypeSwapRemoveResult { + let is_last = index.0 == self.entities.len() - 1; + let entity = self.entities.swap_remove(index.0); ArchetypeSwapRemoveResult { swapped_entity: if is_last { None } else { - Some(self.entities[index].entity) + Some(self.entities[index.0].entity) }, table_row: entity.table_row, } diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index cce7493e22..e0f9b341b3 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -6,12 +6,12 @@ pub use bevy_ecs_macros::Bundle; use crate::{ archetype::{ - Archetype, ArchetypeId, Archetypes, BundleComponentStatus, ComponentStatus, + Archetype, ArchetypeId, ArchetypeRow, Archetypes, BundleComponentStatus, ComponentStatus, SpawnBundleStatus, }, component::{Component, ComponentId, Components, StorageType, Tick}, entity::{Entities, Entity, EntityLocation}, - storage::{SparseSetIndex, SparseSets, Storages, Table}, + storage::{SparseSetIndex, SparseSets, Storages, Table, TableRow}, }; use bevy_ecs_macros::all_tuples; use bevy_ptr::OwningPtr; @@ -379,7 +379,7 @@ impl BundleInfo { sparse_sets: &mut SparseSets, bundle_component_status: &S, entity: Entity, - table_row: usize, + table_row: TableRow, change_tick: u32, bundle: T, ) { @@ -518,17 +518,17 @@ pub(crate) enum InsertBundleResult<'a> { impl<'a, 'b> BundleInserter<'a, 'b> { /// # Safety - /// `entity` must currently exist in the source archetype for this inserter. `archetype_index` + /// `entity` must currently exist in the source archetype for this inserter. `archetype_row` /// must be `entity`'s location in the archetype. `T` must match this [`BundleInfo`]'s type #[inline] pub unsafe fn insert( &mut self, entity: Entity, - archetype_index: usize, + archetype_row: ArchetypeRow, bundle: T, ) -> EntityLocation { let location = EntityLocation { - index: archetype_index, + archetype_row, archetype_id: self.archetype.id(), }; match &mut self.result { @@ -544,14 +544,14 @@ impl<'a, 'b> BundleInserter<'a, 'b> { self.sparse_sets, add_bundle, entity, - self.archetype.entity_table_row(archetype_index), + self.archetype.entity_table_row(archetype_row), self.change_tick, bundle, ); location } InsertBundleResult::NewArchetypeSameTable { new_archetype } => { - let result = self.archetype.swap_remove(location.index); + let result = self.archetype.swap_remove(location.archetype_row); if let Some(swapped_entity) = result.swapped_entity { self.entities.set(swapped_entity.index(), location); } @@ -579,7 +579,7 @@ impl<'a, 'b> BundleInserter<'a, 'b> { new_archetype, new_table, } => { - let result = self.archetype.swap_remove(location.index); + let result = self.archetype.swap_remove(location.archetype_row); if let Some(swapped_entity) = result.swapped_entity { self.entities.set(swapped_entity.index(), location); } @@ -607,7 +607,7 @@ impl<'a, 'b> BundleInserter<'a, 'b> { }; swapped_archetype - .set_entity_table_row(swapped_location.index, result.table_row); + .set_entity_table_row(swapped_location.archetype_row, result.table_row); } // PERF: this could be looked up during Inserter construction and stored (but borrowing makes this nasty) diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index 7404b5345a..82614aeafc 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -34,7 +34,10 @@ mod map_entities; pub use map_entities::*; -use crate::{archetype::ArchetypeId, storage::SparseSetIndex}; +use crate::{ + archetype::{ArchetypeId, ArchetypeRow}, + storage::SparseSetIndex, +}; use serde::{Deserialize, Serialize}; use std::{convert::TryFrom, fmt, mem, sync::atomic::Ordering}; @@ -727,7 +730,7 @@ impl EntityMeta { generation: 0, location: EntityLocation { archetype_id: ArchetypeId::INVALID, - index: usize::MAX, // dummy value, to be filled in + archetype_row: ArchetypeRow::INVALID, // dummy value, to be filled in }, }; } @@ -740,7 +743,7 @@ pub struct EntityLocation { pub archetype_id: ArchetypeId, /// The index of the entity in the archetype - pub index: usize, + pub archetype_row: ArchetypeRow, } #[cfg(test)] diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 9e62594e3c..6ca7ddd32e 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -4,7 +4,7 @@ use crate::{ component::{Component, ComponentId, ComponentStorage, ComponentTicks, StorageType, Tick}, entity::Entity, query::{Access, DebugCheckedUnwrap, FilteredAccess}, - storage::{ComponentSparseSet, Table}, + storage::{ComponentSparseSet, Table, TableRow}, world::{Mut, World}, }; use bevy_ecs_macros::all_tuples; @@ -342,7 +342,7 @@ pub unsafe trait WorldQuery { /// # Safety /// While calling this method on its own cannot cause UB it is marked `unsafe` as the caller must ensure /// that the returned value is not used in any way that would cause two `QueryItem` for the same - /// `archetype_index` or `table_row` to be alive at the same time. + /// `archetype_row` or `table_row` to be alive at the same time. unsafe fn clone_fetch<'w>(fetch: &Self::Fetch<'w>) -> Self::Fetch<'w>; /// Returns true if (and only if) every table of every archetype matched by this fetch contains @@ -395,7 +395,7 @@ pub unsafe trait WorldQuery { unsafe fn fetch<'w>( fetch: &mut Self::Fetch<'w>, entity: Entity, - table_row: usize, + table_row: TableRow, ) -> Self::Item<'w>; /// # Safety @@ -404,7 +404,11 @@ pub unsafe trait WorldQuery { /// `table_row` must be in the range of the current table and archetype. #[allow(unused_variables)] #[inline(always)] - unsafe fn filter_fetch(fetch: &mut Self::Fetch<'_>, entity: Entity, table_row: usize) -> bool { + unsafe fn filter_fetch( + fetch: &mut Self::Fetch<'_>, + entity: Entity, + table_row: TableRow, + ) -> bool { true } @@ -484,7 +488,7 @@ unsafe impl WorldQuery for Entity { unsafe fn fetch<'w>( _fetch: &mut Self::Fetch<'w>, entity: Entity, - _table_row: usize, + _table_row: TableRow, ) -> Self::Item<'w> { entity } @@ -595,13 +599,13 @@ unsafe impl WorldQuery for &T { unsafe fn fetch<'w>( fetch: &mut Self::Fetch<'w>, entity: Entity, - table_row: usize, + table_row: TableRow, ) -> Self::Item<'w> { match T::Storage::STORAGE_TYPE { StorageType::Table => fetch .table_components .debug_checked_unwrap() - .get(table_row) + .get(table_row.index()) .deref(), StorageType::SparseSet => fetch .sparse_set @@ -743,17 +747,17 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T { unsafe fn fetch<'w>( fetch: &mut Self::Fetch<'w>, entity: Entity, - table_row: usize, + table_row: TableRow, ) -> Self::Item<'w> { match T::Storage::STORAGE_TYPE { StorageType::Table => { let (table_components, added_ticks, changed_ticks) = fetch.table_data.debug_checked_unwrap(); Mut { - value: table_components.get(table_row).deref_mut(), + value: table_components.get(table_row.index()).deref_mut(), ticks: Ticks { - added: added_ticks.get(table_row).deref_mut(), - changed: changed_ticks.get(table_row).deref_mut(), + added: added_ticks.get(table_row.index()).deref_mut(), + changed: changed_ticks.get(table_row.index()).deref_mut(), change_tick: fetch.change_tick, last_change_tick: fetch.last_change_tick, }, @@ -872,7 +876,7 @@ unsafe impl WorldQuery for Option { unsafe fn fetch<'w>( fetch: &mut Self::Fetch<'w>, entity: Entity, - table_row: usize, + table_row: TableRow, ) -> Self::Item<'w> { fetch .matches @@ -1082,7 +1086,7 @@ unsafe impl WorldQuery for ChangeTrackers { unsafe fn fetch<'w>( fetch: &mut Self::Fetch<'w>, entity: Entity, - table_row: usize, + table_row: TableRow, ) -> Self::Item<'w> { match T::Storage::STORAGE_TYPE { StorageType::Table => ChangeTrackers { @@ -1091,12 +1095,12 @@ unsafe impl WorldQuery for ChangeTrackers { added: fetch .table_added .debug_checked_unwrap() - .get(table_row) + .get(table_row.index()) .read(), changed: fetch .table_changed .debug_checked_unwrap() - .get(table_row) + .get(table_row.index()) .read(), } }, @@ -1210,7 +1214,7 @@ macro_rules! impl_tuple_fetch { unsafe fn fetch<'w>( _fetch: &mut Self::Fetch<'w>, _entity: Entity, - _table_row: usize + _table_row: TableRow ) -> Self::Item<'w> { let ($($name,)*) = _fetch; ($($name::fetch($name, _entity, _table_row),)*) @@ -1220,7 +1224,7 @@ macro_rules! impl_tuple_fetch { unsafe fn filter_fetch<'w>( _fetch: &mut Self::Fetch<'w>, _entity: Entity, - _table_row: usize + _table_row: TableRow ) -> bool { let ($($name,)*) = _fetch; true $(&& $name::filter_fetch($name, _entity, _table_row))* @@ -1329,7 +1333,7 @@ macro_rules! impl_anytuple_fetch { unsafe fn fetch<'w>( _fetch: &mut Self::Fetch<'w>, _entity: Entity, - _table_row: usize + _table_row: TableRow ) -> Self::Item<'w> { let ($($name,)*) = _fetch; ($( @@ -1443,7 +1447,7 @@ unsafe impl WorldQuery for NopWorldQuery { unsafe fn fetch<'w>( _fetch: &mut Self::Fetch<'w>, _entity: Entity, - _table_row: usize, + _table_row: TableRow, ) -> Self::Item<'w> { } diff --git a/crates/bevy_ecs/src/query/filter.rs b/crates/bevy_ecs/src/query/filter.rs index a84d74f807..a067acbd89 100644 --- a/crates/bevy_ecs/src/query/filter.rs +++ b/crates/bevy_ecs/src/query/filter.rs @@ -3,7 +3,7 @@ use crate::{ component::{Component, ComponentId, ComponentStorage, StorageType, Tick}, entity::Entity, query::{Access, DebugCheckedUnwrap, FilteredAccess, WorldQuery}, - storage::{Column, ComponentSparseSet, Table}, + storage::{Column, ComponentSparseSet, Table, TableRow}, world::World, }; use bevy_ecs_macros::all_tuples; @@ -85,7 +85,7 @@ unsafe impl WorldQuery for With { unsafe fn fetch<'w>( _fetch: &mut Self::Fetch<'w>, _entity: Entity, - _table_row: usize, + _table_row: TableRow, ) -> Self::Item<'w> { } @@ -187,7 +187,7 @@ unsafe impl WorldQuery for Without { unsafe fn fetch<'w>( _fetch: &mut Self::Fetch<'w>, _entity: Entity, - _table_row: usize, + _table_row: TableRow, ) -> Self::Item<'w> { } @@ -330,7 +330,7 @@ macro_rules! impl_query_filter_tuple { unsafe fn fetch<'w>( fetch: &mut Self::Fetch<'w>, _entity: Entity, - _table_row: usize + _table_row: TableRow ) -> Self::Item<'w> { let ($($filter,)*) = fetch; false $(|| ($filter.matches && $filter::filter_fetch(&mut $filter.fetch, _entity, _table_row)))* @@ -340,7 +340,7 @@ macro_rules! impl_query_filter_tuple { unsafe fn filter_fetch<'w>( fetch: &mut Self::Fetch<'w>, entity: Entity, - table_row: usize + table_row: TableRow ) -> bool { Self::fetch(fetch, entity, table_row) } @@ -500,14 +500,14 @@ macro_rules! impl_tick_filter { unsafe fn fetch<'w>( fetch: &mut Self::Fetch<'w>, entity: Entity, - table_row: usize + table_row: TableRow ) -> Self::Item<'w> { match T::Storage::STORAGE_TYPE { StorageType::Table => { fetch .table_ticks .debug_checked_unwrap() - .get(table_row) + .get(table_row.index()) .deref() .is_older_than(fetch.last_change_tick, fetch.change_tick) } @@ -527,7 +527,7 @@ macro_rules! impl_tick_filter { unsafe fn filter_fetch<'w>( fetch: &mut Self::Fetch<'w>, entity: Entity, - table_row: usize + table_row: TableRow ) -> bool { Self::fetch(fetch, entity, table_row) } diff --git a/crates/bevy_ecs/src/query/iter.rs b/crates/bevy_ecs/src/query/iter.rs index c5f1bab220..3844dcb74a 100644 --- a/crates/bevy_ecs/src/query/iter.rs +++ b/crates/bevy_ecs/src/query/iter.rs @@ -3,7 +3,7 @@ use crate::{ entity::{Entities, Entity}, prelude::World, query::{ArchetypeFilter, DebugCheckedUnwrap, QueryState, WorldQuery}, - storage::{TableId, Tables}, + storage::{TableId, TableRow, Tables}, }; use std::{borrow::Borrow, iter::FusedIterator, marker::PhantomData, mem::MaybeUninit}; @@ -170,11 +170,11 @@ where table, ); - let table_row = archetype.entity_table_row(location.index); + let table_row = archetype.entity_table_row(location.archetype_row); // SAFETY: set_archetype was called prior. - // `location.index` is an archetype index row in range of the current archetype, because if it was not, the match above would have `continue`d + // `location.archetype_row` is an archetype index row in range of the current archetype, because if it was not, the match above would have `continue`d if F::filter_fetch(&mut self.filter, entity, table_row) { - // SAFETY: set_archetype was called prior, `location.index` is an archetype index in range of the current archetype + // SAFETY: set_archetype was called prior, `location.archetype_row` is an archetype index in range of the current archetype return Some(Q::fetch(&mut self.fetch, entity, table_row)); } } @@ -464,7 +464,7 @@ struct QueryIterationCursor<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> { // length of the table table or length of the archetype, depending on whether both `Q`'s and `F`'s fetches are dense current_len: usize, // either table row or archetype index, depending on whether both `Q`'s and `F`'s fetches are dense - current_index: usize, + current_row: usize, phantom: PhantomData, } @@ -474,7 +474,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> QueryIterationCursor<'w, 's, /// # Safety /// While calling this method on its own cannot cause UB it is marked `unsafe` as the caller must ensure /// that the returned value is not used in any way that would cause two `QueryItem` for the same - /// `archetype_index` or `table_row` to be alive at the same time. + /// `archetype_row` or `table_row` to be alive at the same time. unsafe fn clone_cursor(&self) -> Self { Self { table_id_iter: self.table_id_iter.clone(), @@ -485,7 +485,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> QueryIterationCursor<'w, 's, fetch: Q::clone_fetch(&self.fetch), filter: F::clone_fetch(&self.filter), current_len: self.current_len, - current_index: self.current_index, + current_row: self.current_row, phantom: PhantomData, } } @@ -533,7 +533,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> QueryIterationCursor<'w, 's, table_id_iter: query_state.matched_table_ids.iter(), archetype_id_iter: query_state.matched_archetype_ids.iter(), current_len: 0, - current_index: 0, + current_row: 0, phantom: PhantomData, } } @@ -541,11 +541,11 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> QueryIterationCursor<'w, 's, /// retrieve item returned from most recent `next` call again. #[inline] unsafe fn peek_last(&mut self) -> Option> { - if self.current_index > 0 { - let index = self.current_index - 1; + if self.current_row > 0 { + let index = self.current_row - 1; if Self::IS_DENSE { let entity = self.table_entities.get_unchecked(index); - Some(Q::fetch(&mut self.fetch, *entity, index)) + Some(Q::fetch(&mut self.fetch, *entity, TableRow::new(index))) } else { let archetype_entity = self.archetype_entities.get_unchecked(index); Some(Q::fetch( @@ -571,7 +571,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> QueryIterationCursor<'w, 's, let ids = self.archetype_id_iter.clone(); ids.map(|id| archetypes[*id].len()).sum() }; - remaining_matched + self.current_len - self.current_index + remaining_matched + self.current_len - self.current_row } // NOTE: If you are changing query iteration code, remember to update the following places, where relevant: @@ -590,7 +590,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> QueryIterationCursor<'w, 's, if Self::IS_DENSE { loop { // we are on the beginning of the query, or finished processing a table, so skip to the next - if self.current_index == self.current_len { + if self.current_row == self.current_len { let table_id = self.table_id_iter.next()?; let table = tables.get(*table_id).debug_checked_unwrap(); // SAFETY: `table` is from the world that `fetch/filter` were created for, @@ -599,28 +599,29 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> QueryIterationCursor<'w, 's, F::set_table(&mut self.filter, &query_state.filter_state, table); self.table_entities = table.entities(); self.current_len = table.entity_count(); - self.current_index = 0; + self.current_row = 0; continue; } // SAFETY: set_table was called prior. - // `current_index` is a table row in range of the current table, because if it was not, then the if above would have been executed. - let entity = self.table_entities.get_unchecked(self.current_index); - if !F::filter_fetch(&mut self.filter, *entity, self.current_index) { - self.current_index += 1; + // `current_row` is a table row in range of the current table, because if it was not, then the if above would have been executed. + let entity = self.table_entities.get_unchecked(self.current_row); + let row = TableRow::new(self.current_row); + if !F::filter_fetch(&mut self.filter, *entity, row) { + self.current_row += 1; continue; } // SAFETY: set_table was called prior. - // `current_index` is a table row in range of the current table, because if it was not, then the if above would have been executed. - let item = Q::fetch(&mut self.fetch, *entity, self.current_index); + // `current_row` is a table row in range of the current table, because if it was not, then the if above would have been executed. + let item = Q::fetch(&mut self.fetch, *entity, row); - self.current_index += 1; + self.current_row += 1; return Some(item); } } else { loop { - if self.current_index == self.current_len { + if self.current_row == self.current_len { let archetype_id = self.archetype_id_iter.next()?; let archetype = archetypes.get(*archetype_id).debug_checked_unwrap(); // SAFETY: `archetype` and `tables` are from the world that `fetch/filter` were created for, @@ -635,30 +636,30 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> QueryIterationCursor<'w, 's, ); self.archetype_entities = archetype.entities(); self.current_len = archetype.len(); - self.current_index = 0; + self.current_row = 0; continue; } // SAFETY: set_archetype was called prior. - // `current_index` is an archetype index row in range of the current archetype, because if it was not, then the if above would have been executed. - let archetype_entity = self.archetype_entities.get_unchecked(self.current_index); + // `current_row` is an archetype index row in range of the current archetype, because if it was not, then the if above would have been executed. + let archetype_entity = self.archetype_entities.get_unchecked(self.current_row); if !F::filter_fetch( &mut self.filter, archetype_entity.entity(), archetype_entity.table_row(), ) { - self.current_index += 1; + self.current_row += 1; continue; } - // SAFETY: set_archetype was called prior, `current_index` is an archetype index in range of the current archetype - // `current_index` is an archetype index row in range of the current archetype, because if it was not, then the if above would have been executed. + // SAFETY: set_archetype was called prior, `current_row` is an archetype index in range of the current archetype + // `current_row` is an archetype index row in range of the current archetype, because if it was not, then the if above would have been executed. let item = Q::fetch( &mut self.fetch, archetype_entity.entity(), archetype_entity.table_row(), ); - self.current_index += 1; + self.current_row += 1; return Some(item); } } diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index ca956c271e..b0f263e7fd 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -6,7 +6,7 @@ use crate::{ query::{ Access, DebugCheckedUnwrap, FilteredAccess, QueryCombinationIter, QueryIter, WorldQuery, }, - storage::TableId, + storage::{TableId, TableRow}, world::{World, WorldId}, }; use bevy_tasks::ComputeTaskPool; @@ -408,7 +408,7 @@ impl QueryState { let mut fetch = Q::init_fetch(world, &self.fetch_state, last_change_tick, change_tick); let mut filter = F::init_fetch(world, &self.filter_state, last_change_tick, change_tick); - let table_row = archetype.entity_table_row(location.index); + let table_row = archetype.entity_table_row(location.archetype_row); let table = world .storages() .tables @@ -928,6 +928,7 @@ impl QueryState { let entities = table.entities(); for row in 0..table.entity_count() { let entity = entities.get_unchecked(row); + let row = TableRow::new(row); if !F::filter_fetch(&mut filter, *entity, row) { continue; } @@ -1022,6 +1023,7 @@ impl QueryState { F::set_table(&mut filter, &self.filter_state, table); for row in offset..offset + len { let entity = entities.get_unchecked(row); + let row = TableRow::new(row); if !F::filter_fetch(&mut filter, *entity, row) { continue; } @@ -1074,8 +1076,8 @@ impl QueryState { F::set_archetype(&mut filter, &self.filter_state, archetype, table); let entities = archetype.entities(); - for archetype_index in offset..offset + len { - let archetype_entity = entities.get_unchecked(archetype_index); + for archetype_row in offset..offset + len { + let archetype_entity = entities.get_unchecked(archetype_row); if !F::filter_fetch( &mut filter, archetype_entity.entity(), diff --git a/crates/bevy_ecs/src/storage/resource.rs b/crates/bevy_ecs/src/storage/resource.rs index 2d852ba73e..01fa742e06 100644 --- a/crates/bevy_ecs/src/storage/resource.rs +++ b/crates/bevy_ecs/src/storage/resource.rs @@ -1,6 +1,6 @@ use crate::archetype::ArchetypeComponentId; use crate::component::{ComponentId, ComponentTicks, Components, TickCells}; -use crate::storage::{Column, SparseSet}; +use crate::storage::{Column, SparseSet, TableRow}; use bevy_ptr::{OwningPtr, Ptr, UnsafeCellDeref}; /// The type-erased backing storage and metadata for a single resource within a [`World`]. @@ -12,6 +12,9 @@ pub struct ResourceData { } impl ResourceData { + /// The only row in the underlying column. + const ROW: TableRow = TableRow::new(0); + /// Returns true if the resource is populated. #[inline] pub fn is_present(&self) -> bool { @@ -27,18 +30,18 @@ impl ResourceData { /// Gets a read-only pointer to the underlying resource, if available. #[inline] pub fn get_data(&self) -> Option> { - self.column.get_data(0) + self.column.get_data(Self::ROW) } /// Gets a read-only reference to the change ticks of the underlying resource, if available. #[inline] pub fn get_ticks(&self) -> Option { - self.column.get_ticks(0) + self.column.get_ticks(Self::ROW) } #[inline] pub(crate) fn get_with_ticks(&self) -> Option<(Ptr<'_>, TickCells<'_>)> { - self.column.get(0) + self.column.get(Self::ROW) } /// Inserts a value into the resource. If a value is already present @@ -54,7 +57,7 @@ impl ResourceData { #[inline] pub(crate) unsafe fn insert(&mut self, value: OwningPtr<'_>, change_tick: u32) { if self.is_present() { - self.column.replace(0, value, change_tick); + self.column.replace(Self::ROW, value, change_tick); } else { self.column.push(value, ComponentTicks::new(change_tick)); } @@ -77,9 +80,12 @@ impl ResourceData { change_ticks: ComponentTicks, ) { if self.is_present() { - self.column.replace_untracked(0, value); - *self.column.get_added_ticks_unchecked(0).deref_mut() = change_ticks.added; - *self.column.get_changed_ticks_unchecked(0).deref_mut() = change_ticks.changed; + self.column.replace_untracked(Self::ROW, value); + *self.column.get_added_ticks_unchecked(Self::ROW).deref_mut() = change_ticks.added; + *self + .column + .get_changed_ticks_unchecked(Self::ROW) + .deref_mut() = change_ticks.changed; } else { self.column.push(value, change_ticks); } @@ -97,7 +103,7 @@ impl ResourceData { #[inline] #[must_use = "The returned pointer to the removed component should be used or dropped"] pub(crate) unsafe fn remove(&mut self) -> Option<(OwningPtr<'_>, ComponentTicks)> { - self.column.swap_remove_and_forget(0) + self.column.swap_remove_and_forget(Self::ROW) } /// Removes a value from the resource, if present, and drops it. diff --git a/crates/bevy_ecs/src/storage/sparse_set.rs b/crates/bevy_ecs/src/storage/sparse_set.rs index 810a4fe569..cc3da14682 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -1,7 +1,7 @@ use crate::{ component::{ComponentId, ComponentInfo, ComponentTicks, Tick, TickCells}, entity::Entity, - storage::Column, + storage::{Column, TableRow}, }; use bevy_ptr::{OwningPtr, Ptr}; use std::{cell::UnsafeCell, hash::Hash, marker::PhantomData}; @@ -147,7 +147,8 @@ impl ComponentSparseSet { if let Some(&dense_index) = self.sparse.get(entity.index()) { #[cfg(debug_assertions)] assert_eq!(entity, self.entities[dense_index as usize]); - self.dense.replace(dense_index as usize, value, change_tick); + self.dense + .replace(TableRow::new(dense_index as usize), value, change_tick); } else { let dense_index = self.dense.len(); self.dense.push(value, ComponentTicks::new(change_tick)); @@ -180,19 +181,19 @@ impl ComponentSparseSet { #[inline] pub fn get(&self, entity: Entity) -> Option> { self.sparse.get(entity.index()).map(|dense_index| { - let dense_index = *dense_index as usize; + let dense_index = (*dense_index) as usize; #[cfg(debug_assertions)] assert_eq!(entity, self.entities[dense_index]); // SAFETY: if the sparse index points to something in the dense vec, it exists - unsafe { self.dense.get_data_unchecked(dense_index) } + unsafe { self.dense.get_data_unchecked(TableRow::new(dense_index)) } }) } #[inline] pub fn get_with_ticks(&self, entity: Entity) -> Option<(Ptr<'_>, TickCells<'_>)> { - let dense_index = *self.sparse.get(entity.index())? as usize; + let dense_index = TableRow::new(*self.sparse.get(entity.index())? as usize); #[cfg(debug_assertions)] - assert_eq!(entity, self.entities[dense_index]); + assert_eq!(entity, self.entities[dense_index.index()]); // SAFETY: if the sparse index points to something in the dense vec, it exists unsafe { Some(( @@ -211,7 +212,12 @@ impl ComponentSparseSet { #[cfg(debug_assertions)] assert_eq!(entity, self.entities[dense_index]); // SAFETY: if the sparse index points to something in the dense vec, it exists - unsafe { Some(self.dense.get_added_ticks_unchecked(dense_index)) } + unsafe { + Some( + self.dense + .get_added_ticks_unchecked(TableRow::new(dense_index)), + ) + } } #[inline] @@ -220,7 +226,12 @@ impl ComponentSparseSet { #[cfg(debug_assertions)] assert_eq!(entity, self.entities[dense_index]); // SAFETY: if the sparse index points to something in the dense vec, it exists - unsafe { Some(self.dense.get_changed_ticks_unchecked(dense_index)) } + unsafe { + Some( + self.dense + .get_changed_ticks_unchecked(TableRow::new(dense_index)), + ) + } } #[inline] @@ -229,7 +240,7 @@ impl ComponentSparseSet { #[cfg(debug_assertions)] assert_eq!(entity, self.entities[dense_index]); // SAFETY: if the sparse index points to something in the dense vec, it exists - unsafe { Some(self.dense.get_ticks_unchecked(dense_index)) } + unsafe { Some(self.dense.get_ticks_unchecked(TableRow::new(dense_index))) } } /// Removes the `entity` from this sparse set and returns a pointer to the associated value (if @@ -243,7 +254,10 @@ impl ComponentSparseSet { self.entities.swap_remove(dense_index); let is_last = dense_index == self.dense.len() - 1; // SAFETY: dense_index was just removed from `sparse`, which ensures that it is valid - let (value, _) = unsafe { self.dense.swap_remove_and_forget_unchecked(dense_index) }; + let (value, _) = unsafe { + self.dense + .swap_remove_and_forget_unchecked(TableRow::new(dense_index)) + }; if !is_last { let swapped_entity = self.entities[dense_index]; #[cfg(not(debug_assertions))] @@ -264,7 +278,7 @@ impl ComponentSparseSet { self.entities.swap_remove(dense_index); let is_last = dense_index == self.dense.len() - 1; // SAFETY: if the sparse index points to something in the dense vec, it exists - unsafe { self.dense.swap_remove_unchecked(dense_index) } + unsafe { self.dense.swap_remove_unchecked(TableRow::new(dense_index)) } if !is_last { let swapped_entity = self.entities[dense_index]; #[cfg(not(debug_assertions))] diff --git a/crates/bevy_ecs/src/storage/table.rs b/crates/bevy_ecs/src/storage/table.rs index 9cd773af58..f14dbcedf4 100644 --- a/crates/bevy_ecs/src/storage/table.rs +++ b/crates/bevy_ecs/src/storage/table.rs @@ -32,6 +32,39 @@ impl TableId { } } +/// A opaque newtype for rows in [`Table`]s. Specifies a single row in a specific table. +/// +/// Values of this type are retreivable from [`Archetype::entity_table_row`] and can be +/// used alongside [`Archetype::table_id`] to fetch the exact table and row where an +/// [`Entity`]'s +/// +/// Values of this type are only valid so long as entities have not moved around. +/// Adding and removing components from an entity, or despawning it will invalidate +/// potentially any table row in the table the entity was previously stored in. Users +/// should *always* fetch the approripate row from the entity's [`Archetype`] before +/// fetching the entity's components. +/// +/// [`Archetype`]: crate::archetype::Archetype +/// [`Archetype::entity_table_row`]: crate::archetype::Archetype::entity_table_row +/// [`Archetype::table_id`]: crate::archetype::Archetype::table_id +/// [`Entity`]: crate::entity::Entity +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct TableRow(usize); + +impl TableRow { + /// Creates a `TableRow`. + #[inline] + pub const fn new(index: usize) -> Self { + Self(index) + } + + /// Gets the index of the row. + #[inline] + pub const fn index(self) -> usize { + self.0 + } +} + #[derive(Debug)] pub struct Column { data: BlobVec, @@ -62,11 +95,11 @@ impl Column { /// # Safety /// Assumes data has already been allocated for the given row. #[inline] - pub(crate) unsafe fn initialize(&mut self, row: usize, data: OwningPtr<'_>, tick: Tick) { - debug_assert!(row < self.len()); - self.data.initialize_unchecked(row, data); - *self.added_ticks.get_unchecked_mut(row).get_mut() = tick; - *self.changed_ticks.get_unchecked_mut(row).get_mut() = tick; + pub(crate) unsafe fn initialize(&mut self, row: TableRow, data: OwningPtr<'_>, tick: Tick) { + debug_assert!(row.index() < self.len()); + self.data.initialize_unchecked(row.index(), data); + *self.added_ticks.get_unchecked_mut(row.index()).get_mut() = tick; + *self.changed_ticks.get_unchecked_mut(row.index()).get_mut() = tick; } /// Writes component data to the column at given row. @@ -75,11 +108,11 @@ impl Column { /// # Safety /// Assumes data has already been allocated for the given row. #[inline] - 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); + pub(crate) unsafe fn replace(&mut self, row: TableRow, data: OwningPtr<'_>, change_tick: u32) { + debug_assert!(row.index() < self.len()); + self.data.replace_unchecked(row.index(), data); self.changed_ticks - .get_unchecked_mut(row) + .get_unchecked_mut(row.index()) .get_mut() .set_changed(change_tick); } @@ -91,9 +124,9 @@ impl Column { /// # Safety /// Assumes data has already been allocated for the given row. #[inline] - pub(crate) unsafe fn replace_untracked(&mut self, row: usize, data: OwningPtr<'_>) { - debug_assert!(row < self.len()); - self.data.replace_unchecked(row, data); + pub(crate) unsafe fn replace_untracked(&mut self, row: TableRow, data: OwningPtr<'_>) { + debug_assert!(row.index() < self.len()); + self.data.replace_unchecked(row.index(), data); } #[inline] @@ -109,23 +142,23 @@ impl Column { /// # Safety /// index must be in-bounds #[inline] - pub(crate) unsafe fn swap_remove_unchecked(&mut self, row: usize) { - self.data.swap_remove_and_drop_unchecked(row); - self.added_ticks.swap_remove(row); - self.changed_ticks.swap_remove(row); + pub(crate) unsafe fn swap_remove_unchecked(&mut self, row: TableRow) { + self.data.swap_remove_and_drop_unchecked(row.index()); + self.added_ticks.swap_remove(row.index()); + self.changed_ticks.swap_remove(row.index()); } #[inline] #[must_use = "The returned pointer should be used to drop the removed component"] pub(crate) fn swap_remove_and_forget( &mut self, - row: usize, + row: TableRow, ) -> Option<(OwningPtr<'_>, ComponentTicks)> { - (row < self.data.len()).then(|| { + (row.index() < self.data.len()).then(|| { // SAFETY: The row was length checked before this. - let data = unsafe { self.data.swap_remove_and_forget_unchecked(row) }; - let added = self.added_ticks.swap_remove(row).into_inner(); - let changed = self.changed_ticks.swap_remove(row).into_inner(); + let data = unsafe { self.data.swap_remove_and_forget_unchecked(row.index()) }; + let added = self.added_ticks.swap_remove(row.index()).into_inner(); + let changed = self.changed_ticks.swap_remove(row.index()).into_inner(); (data, ComponentTicks { added, changed }) }) } @@ -136,11 +169,11 @@ impl Column { #[must_use = "The returned pointer should be used to dropped the removed component"] pub(crate) unsafe fn swap_remove_and_forget_unchecked( &mut self, - row: usize, + row: TableRow, ) -> (OwningPtr<'_>, ComponentTicks) { - let data = self.data.swap_remove_and_forget_unchecked(row); - let added = self.added_ticks.swap_remove(row).into_inner(); - let changed = self.changed_ticks.swap_remove(row).into_inner(); + let data = self.data.swap_remove_and_forget_unchecked(row.index()); + let added = self.added_ticks.swap_remove(row.index()).into_inner(); + let changed = self.changed_ticks.swap_remove(row.index()).into_inner(); (data, ComponentTicks { added, changed }) } @@ -159,14 +192,16 @@ impl Column { pub(crate) unsafe fn initialize_from_unchecked( &mut self, other: &mut Column, - src_row: usize, - dst_row: usize, + src_row: TableRow, + dst_row: TableRow, ) { debug_assert!(self.data.layout() == other.data.layout()); - let ptr = self.data.get_unchecked_mut(dst_row); - other.data.swap_remove_unchecked(src_row, ptr); - *self.added_ticks.get_unchecked_mut(dst_row) = other.added_ticks.swap_remove(src_row); - *self.changed_ticks.get_unchecked_mut(dst_row) = other.changed_ticks.swap_remove(src_row); + let ptr = self.data.get_unchecked_mut(dst_row.index()); + other.data.swap_remove_unchecked(src_row.index(), ptr); + *self.added_ticks.get_unchecked_mut(dst_row.index()) = + other.added_ticks.swap_remove(src_row.index()); + *self.changed_ticks.get_unchecked_mut(dst_row.index()) = + other.changed_ticks.swap_remove(src_row.index()); } // # Safety @@ -206,66 +241,66 @@ impl Column { } #[inline] - pub fn get(&self, row: usize) -> Option<(Ptr<'_>, TickCells<'_>)> { - (row < self.data.len()) + pub fn get(&self, row: TableRow) -> Option<(Ptr<'_>, TickCells<'_>)> { + (row.index() < self.data.len()) // SAFETY: The row is length checked before fetching the pointer. This is being // accessed through a read-only reference to the column. .then(|| unsafe { ( - self.data.get_unchecked(row), + self.data.get_unchecked(row.index()), TickCells { - added: self.added_ticks.get_unchecked(row), - changed: self.changed_ticks.get_unchecked(row), + added: self.added_ticks.get_unchecked(row.index()), + changed: self.changed_ticks.get_unchecked(row.index()), }, ) }) } #[inline] - pub fn get_data(&self, row: usize) -> Option> { + pub fn get_data(&self, row: TableRow) -> Option> { // SAFETY: The row is length checked before fetching the pointer. This is being // accessed through a read-only reference to the column. - (row < self.data.len()).then(|| unsafe { self.data.get_unchecked(row) }) + (row.index() < self.data.len()).then(|| unsafe { self.data.get_unchecked(row.index()) }) } /// # Safety /// - 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(&self, row: usize) -> Ptr<'_> { - debug_assert!(row < self.data.len()); - self.data.get_unchecked(row) + pub unsafe fn get_data_unchecked(&self, row: TableRow) -> Ptr<'_> { + debug_assert!(row.index() < self.data.len()); + self.data.get_unchecked(row.index()) } #[inline] - pub fn get_data_mut(&mut self, row: usize) -> Option> { + pub fn get_data_mut(&mut self, row: TableRow) -> Option> { // SAFETY: The row is length checked before fetching the pointer. This is being // accessed through an exclusive reference to the column. - (row < self.data.len()).then(|| unsafe { self.data.get_unchecked_mut(row) }) + (row.index() < self.data.len()).then(|| unsafe { self.data.get_unchecked_mut(row.index()) }) } /// # Safety /// - index must be in-bounds /// - no other reference to the data of the same row can exist at the same time #[inline] - 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) + pub(crate) unsafe fn get_data_unchecked_mut(&mut self, row: TableRow) -> PtrMut<'_> { + debug_assert!(row.index() < self.data.len()); + self.data.get_unchecked_mut(row.index()) } #[inline] - pub fn get_added_ticks(&self, row: usize) -> Option<&UnsafeCell> { - self.added_ticks.get(row) + pub fn get_added_ticks(&self, row: TableRow) -> Option<&UnsafeCell> { + self.added_ticks.get(row.index()) } #[inline] - pub fn get_changed_ticks(&self, row: usize) -> Option<&UnsafeCell> { - self.changed_ticks.get(row) + pub fn get_changed_ticks(&self, row: TableRow) -> Option<&UnsafeCell> { + self.changed_ticks.get(row.index()) } #[inline] - pub fn get_ticks(&self, row: usize) -> Option { - if row < self.data.len() { + pub fn get_ticks(&self, row: TableRow) -> Option { + if row.index() < self.data.len() { // SAFETY: The size of the column has already been checked. Some(unsafe { self.get_ticks_unchecked(row) }) } else { @@ -276,28 +311,28 @@ impl Column { /// # Safety /// index must be in-bounds #[inline] - pub unsafe fn get_added_ticks_unchecked(&self, row: usize) -> &UnsafeCell { - debug_assert!(row < self.added_ticks.len()); - self.added_ticks.get_unchecked(row) + pub unsafe fn get_added_ticks_unchecked(&self, row: TableRow) -> &UnsafeCell { + debug_assert!(row.index() < self.added_ticks.len()); + self.added_ticks.get_unchecked(row.index()) } /// # Safety /// index must be in-bounds #[inline] - pub unsafe fn get_changed_ticks_unchecked(&self, row: usize) -> &UnsafeCell { - debug_assert!(row < self.changed_ticks.len()); - self.changed_ticks.get_unchecked(row) + pub unsafe fn get_changed_ticks_unchecked(&self, row: TableRow) -> &UnsafeCell { + debug_assert!(row.index() < self.changed_ticks.len()); + self.changed_ticks.get_unchecked(row.index()) } /// # Safety /// index must be in-bounds #[inline] - pub unsafe fn get_ticks_unchecked(&self, row: usize) -> ComponentTicks { - debug_assert!(row < self.added_ticks.len()); - debug_assert!(row < self.changed_ticks.len()); + pub unsafe fn get_ticks_unchecked(&self, row: TableRow) -> ComponentTicks { + debug_assert!(row.index() < self.added_ticks.len()); + debug_assert!(row.index() < self.changed_ticks.len()); ComponentTicks { - added: self.added_ticks.get_unchecked(row).read(), - changed: self.changed_ticks.get_unchecked(row).read(), + added: self.added_ticks.get_unchecked(row.index()).read(), + changed: self.changed_ticks.get_unchecked(row.index()).read(), } } @@ -385,16 +420,16 @@ impl Table { /// /// # Safety /// `row` must be in-bounds - pub(crate) unsafe fn swap_remove_unchecked(&mut self, row: usize) -> Option { + pub(crate) unsafe fn swap_remove_unchecked(&mut self, row: TableRow) -> Option { for column in self.columns.values_mut() { column.swap_remove_unchecked(row); } - let is_last = row == self.entities.len() - 1; - self.entities.swap_remove(row); + let is_last = row.index() == self.entities.len() - 1; + self.entities.swap_remove(row.index()); if is_last { None } else { - Some(self.entities[row]) + Some(self.entities[row.index()]) } } @@ -407,12 +442,12 @@ impl Table { /// Row must be in-bounds pub(crate) unsafe fn move_to_and_forget_missing_unchecked( &mut self, - row: usize, + row: TableRow, new_table: &mut Table, ) -> TableMoveResult { - debug_assert!(row < self.entity_count()); - let is_last = row == self.entities.len() - 1; - let new_row = new_table.allocate(self.entities.swap_remove(row)); + debug_assert!(row.index() < self.entity_count()); + let is_last = row.index() == self.entities.len() - 1; + let new_row = new_table.allocate(self.entities.swap_remove(row.index())); for (component_id, column) in self.columns.iter_mut() { if let Some(new_column) = new_table.get_column_mut(*component_id) { new_column.initialize_from_unchecked(column, row, new_row); @@ -426,7 +461,7 @@ impl Table { swapped_entity: if is_last { None } else { - Some(self.entities[row]) + Some(self.entities[row.index()]) }, } } @@ -439,12 +474,12 @@ impl Table { /// row must be in-bounds pub(crate) unsafe fn move_to_and_drop_missing_unchecked( &mut self, - row: usize, + row: TableRow, new_table: &mut Table, ) -> TableMoveResult { - debug_assert!(row < self.entity_count()); - let is_last = row == self.entities.len() - 1; - let new_row = new_table.allocate(self.entities.swap_remove(row)); + debug_assert!(row.index() < self.entity_count()); + let is_last = row.index() == self.entities.len() - 1; + let new_row = new_table.allocate(self.entities.swap_remove(row.index())); for (component_id, column) in self.columns.iter_mut() { if let Some(new_column) = new_table.get_column_mut(*component_id) { new_column.initialize_from_unchecked(column, row, new_row); @@ -457,7 +492,7 @@ impl Table { swapped_entity: if is_last { None } else { - Some(self.entities[row]) + Some(self.entities[row.index()]) }, } } @@ -470,12 +505,12 @@ impl Table { /// `row` must be in-bounds. `new_table` must contain every component this table has pub(crate) unsafe fn move_to_superset_unchecked( &mut self, - row: usize, + row: TableRow, new_table: &mut Table, ) -> TableMoveResult { - debug_assert!(row < self.entity_count()); - let is_last = row == self.entities.len() - 1; - let new_row = new_table.allocate(self.entities.swap_remove(row)); + debug_assert!(row.index() < self.entity_count()); + let is_last = row.index() == self.entities.len() - 1; + let new_row = new_table.allocate(self.entities.swap_remove(row.index())); for (component_id, column) in self.columns.iter_mut() { new_table .get_column_mut(*component_id) @@ -487,7 +522,7 @@ impl Table { swapped_entity: if is_last { None } else { - Some(self.entities[row]) + Some(self.entities[row.index()]) }, } } @@ -524,7 +559,7 @@ impl Table { /// /// # Safety /// the allocated row must be written to immediately with valid values in each column - pub(crate) unsafe fn allocate(&mut self, entity: Entity) -> usize { + pub(crate) unsafe fn allocate(&mut self, entity: Entity) -> TableRow { self.reserve(1); let index = self.entities.len(); self.entities.push(entity); @@ -533,7 +568,7 @@ impl Table { column.added_ticks.push(UnsafeCell::new(Tick::new(0))); column.changed_ticks.push(UnsafeCell::new(Tick::new(0))); } - index + TableRow(index) } #[inline] @@ -594,7 +629,7 @@ impl Default for Tables { pub(crate) struct TableMoveResult { pub swapped_entity: Option, - pub new_row: usize, + pub new_row: TableRow, } impl Tables { @@ -690,7 +725,7 @@ mod tests { use crate::{ component::{Components, Tick}, entity::Entity, - storage::TableBuilder, + storage::{TableBuilder, TableRow}, }; #[derive(Component)] struct W(T); @@ -699,7 +734,7 @@ mod tests { fn table() { let mut components = Components::default(); let mut storages = Storages::default(); - let component_id = components.init_component::>(&mut storages); + let component_id = components.init_component::>(&mut storages); let columns = &[component_id]; let mut builder = TableBuilder::with_capacity(0, columns.len()); builder.add_column(components.get_info(component_id).unwrap()); @@ -709,7 +744,7 @@ mod tests { // SAFETY: we allocate and immediately set data afterwards unsafe { let row = table.allocate(*entity); - let value: W = W(row); + let value: W = W(row); OwningPtr::make(value, |value_ptr| { table.get_column_mut(component_id).unwrap().initialize( row, diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index e60bfbfd7d..458564beb4 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -7,7 +7,7 @@ use crate::{ TickCells, }, entity::{Entities, Entity, EntityLocation}, - storage::{Column, ComponentSparseSet, SparseSet, Storages}, + storage::{Column, ComponentSparseSet, SparseSet, Storages, TableRow}, world::{Mut, World}, }; use bevy_ptr::{OwningPtr, Ptr}; @@ -371,7 +371,8 @@ impl<'w> EntityMut<'w> { ); // SAFETY: location matches current entity. `T` matches `bundle_info` unsafe { - self.location = bundle_inserter.insert(self.entity, self.location.index, bundle); + self.location = + bundle_inserter.insert(self.entity, self.location.archetype_row, bundle); } self @@ -465,7 +466,7 @@ impl<'w> EntityMut<'w> { new_archetype_id: ArchetypeId, ) { let old_archetype = &mut archetypes[old_archetype_id]; - let remove_result = old_archetype.swap_remove(old_location.index); + let remove_result = old_archetype.swap_remove(old_location.archetype_row); if let Some(swapped_entity) = remove_result.swapped_entity { entities.set(swapped_entity.index(), old_location); } @@ -494,7 +495,7 @@ impl<'w> EntityMut<'w> { if let Some(swapped_entity) = move_result.swapped_entity { let swapped_location = entities.get(swapped_entity).unwrap(); archetypes[swapped_location.archetype_id] - .set_entity_table_row(swapped_location.index, old_table_row); + .set_entity_table_row(swapped_location.archetype_row, old_table_row); } new_location @@ -588,7 +589,7 @@ impl<'w> EntityMut<'w> { .get_or_insert_with(component_id, Vec::new); removed_components.push(self.entity); } - let remove_result = archetype.swap_remove(location.index); + let remove_result = archetype.swap_remove(location.archetype_row); if let Some(swapped_entity) = remove_result.swapped_entity { // SAFETY: swapped_entity is valid and the swapped entity's components are // moved to the new location immediately after. @@ -611,7 +612,7 @@ impl<'w> EntityMut<'w> { if let Some(moved_entity) = moved_entity { let moved_location = world.entities.get(moved_entity).unwrap(); world.archetypes[moved_location.archetype_id] - .set_entity_table_row(moved_location.index, table_row); + .set_entity_table_row(moved_location.archetype_row, table_row); } } @@ -701,11 +702,11 @@ fn fetch_table( world: &World, location: EntityLocation, component_id: ComponentId, -) -> Option<(&Column, usize)> { +) -> Option<(&Column, TableRow)> { let archetype = &world.archetypes[location.archetype_id]; let table = &world.storages.tables[archetype.table_id()]; let components = table.get_column(component_id)?; - let table_row = archetype.entity_table_row(location.index); + let table_row = archetype.entity_table_row(location.archetype_row); Some((components, table_row)) } @@ -823,7 +824,7 @@ unsafe fn take_component<'a>( let table = &mut storages.tables[archetype.table_id()]; // SAFETY: archetypes will always point to valid columns let components = table.get_column_mut(component_id).unwrap(); - let table_row = archetype.entity_table_row(location.index); + let table_row = archetype.entity_table_row(location.archetype_row); // SAFETY: archetypes only store valid table_rows and the stored component type is T components.get_data_unchecked_mut(table_row).promote() } diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 435694f23c..affc44085e 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -8,7 +8,7 @@ pub use spawn_batch::*; pub use world_cell::*; use crate::{ - archetype::{ArchetypeComponentId, ArchetypeId, Archetypes}, + archetype::{ArchetypeComponentId, ArchetypeId, ArchetypeRow, Archetypes}, bundle::{Bundle, BundleInserter, BundleSpawner, Bundles}, change_detection::{MutUntyped, Ticks}, component::{ @@ -329,10 +329,10 @@ impl World { .entities() .iter() .enumerate() - .map(|(index, archetype_entity)| { + .map(|(archetype_row, archetype_entity)| { let location = EntityLocation { archetype_id: archetype.id(), - index, + archetype_row: ArchetypeRow::new(archetype_row), }; EntityRef::new(self, archetype_entity.entity(), location) }) @@ -1096,7 +1096,7 @@ impl World { if location.archetype_id == archetype => { // SAFETY: `entity` is valid, `location` matches entity, bundle matches inserter - unsafe { inserter.insert(entity, location.index, bundle) }; + unsafe { inserter.insert(entity, location.archetype_row, bundle) }; } _ => { let mut inserter = bundle_info.get_bundle_inserter( @@ -1108,7 +1108,7 @@ impl World { change_tick, ); // SAFETY: `entity` is valid, `location` matches entity, bundle matches inserter - unsafe { inserter.insert(entity, location.index, bundle) }; + unsafe { inserter.insert(entity, location.archetype_row, bundle) }; spawn_or_insert = SpawnOrInsert::Insert(inserter, location.archetype_id); }