Reduced TableRow as Casting (#10811)

# Objective

- Fixes #10806

## Solution

Replaced `new` and `index` methods for both `TableRow` and `TableId`
with `from_*` and `as_*` methods. These remove the need to perform
casting at call sites, reducing the total number of casts in the Bevy
codebase. Within these methods, an appropriate `debug_assertion` ensures
the cast will behave in an expected manner (no wrapping, etc.). I am
using a `debug_assertion` instead of an `assert` to reduce any possible
runtime overhead, however minimal. This choice is something I am open to
changing (or leaving up to another PR) if anyone has any strong
arguments for it.

---

## Changelog

- `ComponentSparseSet::sparse` stores a `TableRow` instead of a `u32`
(private change)
- Replaced `TableRow::new` and `TableRow::index` methods with
`TableRow::from_*` and `TableRow::as_*`, with `debug_assertions`
protecting any internal casting.
- Replaced `TableId::new` and `TableId::index` methods with
`TableId::from_*` and `TableId::as_*`, with `debug_assertions`
protecting any internal casting.
- All `TableId` methods are now `const`

## Migration Guide

- `TableRow::new` -> `TableRow::from_usize`
- `TableRow::index` -> `TableRow::as_usize`
- `TableId::new` -> `TableId::from_usize`
- `TableId::index` -> `TableId::as_usize`

---

## Notes

I have chosen to remove the `index` and `new` methods for the following
chain of reasoning:

- Across the codebase, `new` was called with a mixture of `u32` and
`usize` values. Likewise for `index`.
- Choosing `new` to either be `usize` or `u32` would break half of these
call-sites, requiring `as` casting at the site.
- Adding a second method `new_u32` or `new_usize` avoids the above, bu
looks visually inconsistent.
- Therefore, they should be replaced with `from_*` and `as_*` methods
instead.

Worth noting is that by updating `ComponentSparseSet`, there are now
zero instances of interacting with the inner value of `TableRow` as a
`u32`, it is exclusively used as a `usize` value (due to interactions
with methods like `len` and slice indexing). I have left the `as_u32`
and `from_u32` methods as the "proper" constructors/getters.

Author: bushrat011899

crates/bevy_ecs/src/query/fetch.rs

@@ -605,7 +605,7 @@ unsafe impl<T: Component> WorldQuery for &T {
             StorageType::Table => fetch
                 .table_components
                 .debug_checked_unwrap()
-                .get(table_row.index())
+                .get(table_row.as_usize())
                 .deref(),
             StorageType::SparseSet => fetch
                 .sparse_set
@@ -760,10 +760,10 @@ unsafe impl<'__w, T: Component> WorldQuery for Ref<'__w, T> {
                 let (table_components, added_ticks, changed_ticks) =
                     fetch.table_data.debug_checked_unwrap();
                 Ref {
-                    value: table_components.get(table_row.index()).deref(),
+                    value: table_components.get(table_row.as_usize()).deref(),
                     ticks: Ticks {
-                        added: added_ticks.get(table_row.index()).deref(),
-                        changed: changed_ticks.get(table_row.index()).deref(),
+                        added: added_ticks.get(table_row.as_usize()).deref(),
+                        changed: changed_ticks.get(table_row.as_usize()).deref(),
                         this_run: fetch.this_run,
                         last_run: fetch.last_run,
                     },
@@ -927,10 +927,10 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T {
                 let (table_components, added_ticks, changed_ticks) =
                     fetch.table_data.debug_checked_unwrap();
                 Mut {
-                    value: table_components.get(table_row.index()).deref_mut(),
+                    value: table_components.get(table_row.as_usize()).deref_mut(),
                     ticks: TicksMut {
-                        added: added_ticks.get(table_row.index()).deref_mut(),
-                        changed: changed_ticks.get(table_row.index()).deref_mut(),
+                        added: added_ticks.get(table_row.as_usize()).deref_mut(),
+                        changed: changed_ticks.get(table_row.as_usize()).deref_mut(),
                         this_run: fetch.this_run,
                         last_run: fetch.last_run,
                     },

crates/bevy_ecs/src/query/filter.rs

@@ -626,7 +626,7 @@ unsafe impl<T: Component> WorldQuery for Added<T> {
             StorageType::Table => fetch
                 .table_ticks
                 .debug_checked_unwrap()
-                .get(table_row.index())
+                .get(table_row.as_usize())
                 .deref()
                 .is_newer_than(fetch.last_run, fetch.this_run),
             StorageType::SparseSet => {
@@ -802,7 +802,7 @@ unsafe impl<T: Component> WorldQuery for Changed<T> {
             StorageType::Table => fetch
                 .table_ticks
                 .debug_checked_unwrap()
-                .get(table_row.index())
+                .get(table_row.as_usize())
                 .deref()
                 .is_newer_than(fetch.last_run, fetch.this_run),
             StorageType::SparseSet => {

crates/bevy_ecs/src/query/iter.rs

@@ -106,6 +106,11 @@ impl<'w, 's, Q: WorldQueryData, F: WorldQueryFilter> QueryIter<'w, 's, Q, F> {
     where
         Func: FnMut(B, Q::Item<'w>) -> B,
     {
+        assert!(
+            rows.end <= u32::MAX as usize,
+            "TableRow is only valid up to u32::MAX"
+        );
+
         Q::set_table(&mut self.cursor.fetch, &self.query_state.fetch_state, table);
         F::set_table(
             &mut self.cursor.filter,
@@ -117,7 +122,7 @@ impl<'w, 's, Q: WorldQueryData, F: WorldQueryFilter> QueryIter<'w, 's, Q, F> {
         for row in rows {
             // SAFETY: Caller assures `row` in range of the current archetype.
             let entity = entities.get_unchecked(row);
-            let row = TableRow::new(row);
+            let row = TableRow::from_usize(row);
             // SAFETY: set_table was called prior.
             // Caller assures `row` in range of the current archetype.
             if !F::filter_fetch(&mut self.cursor.filter, *entity, row) {
@@ -707,7 +712,11 @@ impl<'w, 's, Q: WorldQueryData, F: WorldQueryFilter> QueryIterationCursor<'w, 's
             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, TableRow::new(index)))
+                Some(Q::fetch(
+                    &mut self.fetch,
+                    *entity,
+                    TableRow::from_usize(index),
+                ))
             } else {
                 let archetype_entity = self.archetype_entities.get_unchecked(index);
                 Some(Q::fetch(
@@ -768,7 +777,7 @@ impl<'w, 's, Q: WorldQueryData, F: WorldQueryFilter> QueryIterationCursor<'w, 's
                 // SAFETY: set_table was called prior.
                 // `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);
+                let row = TableRow::from_usize(self.current_row);
                 if !F::filter_fetch(&mut self.filter, *entity, row) {
                     self.current_row += 1;
                     continue;

crates/bevy_ecs/src/query/state.rs

@@ -267,7 +267,7 @@ impl<Q: WorldQueryData, F: WorldQueryFilter> QueryState<Q, F> {
                 self.matched_archetypes.set(archetype_index, true);
                 self.matched_archetype_ids.push(archetype.id());
             }
-            let table_index = archetype.table_id().index();
+            let table_index = archetype.table_id().as_usize();
             if !self.matched_tables.contains(table_index) {
                 self.matched_tables.grow(table_index + 1);
                 self.matched_tables.set(table_index, true);

crates/bevy_ecs/src/storage/resource.rs

@@ -40,7 +40,7 @@ impl<const SEND: bool> Drop for ResourceData<SEND> {
 
 impl<const SEND: bool> ResourceData<SEND> {
     /// The only row in the underlying column.
-    const ROW: TableRow = TableRow::new(0);
+    const ROW: TableRow = TableRow::from_u32(0);
 
     /// Validates the access to `!Send` resources is only done on the thread they were created from.
     ///

crates/bevy_ecs/src/storage/sparse_set.rs

@@ -124,7 +124,7 @@ pub struct ComponentSparseSet {
     entities: Vec<EntityIndex>,
     #[cfg(debug_assertions)]
     entities: Vec<Entity>,
-    sparse: SparseArray<EntityIndex, u32>,
+    sparse: SparseArray<EntityIndex, TableRow>,
 }
 
 impl ComponentSparseSet {
@@ -171,13 +171,13 @@ 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(TableRow::new(dense_index as usize), value, change_tick);
+            assert_eq!(entity, self.entities[dense_index.as_usize()]);
+            self.dense.replace(dense_index, value, change_tick);
         } else {
             let dense_index = self.dense.len();
             self.dense.push(value, ComponentTicks::new(change_tick));
-            self.sparse.insert(entity.index(), dense_index as u32);
+            self.sparse
+                .insert(entity.index(), TableRow::from_usize(dense_index));
             #[cfg(debug_assertions)]
             assert_eq!(self.entities.len(), dense_index);
             #[cfg(not(debug_assertions))]
@@ -194,7 +194,7 @@ impl ComponentSparseSet {
         {
             if let Some(&dense_index) = self.sparse.get(entity.index()) {
                 #[cfg(debug_assertions)]
-                assert_eq!(entity, self.entities[dense_index as usize]);
+                assert_eq!(entity, self.entities[dense_index.as_usize()]);
                 true
             } else {
                 false
@@ -209,12 +209,11 @@ impl ComponentSparseSet {
     /// Returns `None` if `entity` does not have a component in the sparse set.
     #[inline]
     pub fn get(&self, entity: Entity) -> Option<Ptr<'_>> {
-        self.sparse.get(entity.index()).map(|dense_index| {
-            let dense_index = (*dense_index) as usize;
+        self.sparse.get(entity.index()).map(|&dense_index| {
             #[cfg(debug_assertions)]
-            assert_eq!(entity, self.entities[dense_index]);
+            assert_eq!(entity, self.entities[dense_index.as_usize()]);
             // SAFETY: if the sparse index points to something in the dense vec, it exists
-            unsafe { self.dense.get_data_unchecked(TableRow::new(dense_index)) }
+            unsafe { self.dense.get_data_unchecked(dense_index) }
         })
     }
 
@@ -223,9 +222,9 @@ impl ComponentSparseSet {
     /// Returns `None` if `entity` does not have a component in the sparse set.
     #[inline]
     pub fn get_with_ticks(&self, entity: Entity) -> Option<(Ptr<'_>, TickCells<'_>)> {
-        let dense_index = TableRow::new(*self.sparse.get(entity.index())? as usize);
+        let dense_index = *self.sparse.get(entity.index())?;
         #[cfg(debug_assertions)]
-        assert_eq!(entity, self.entities[dense_index.index()]);
+        assert_eq!(entity, self.entities[dense_index.as_usize()]);
         // SAFETY: if the sparse index points to something in the dense vec, it exists
         unsafe {
             Some((
@@ -243,69 +242,55 @@ impl ComponentSparseSet {
     /// Returns `None` if `entity` does not have a component in the sparse set.
     #[inline]
     pub fn get_added_tick(&self, entity: Entity) -> Option<&UnsafeCell<Tick>> {
-        let dense_index = *self.sparse.get(entity.index())? as usize;
+        let dense_index = *self.sparse.get(entity.index())?;
         #[cfg(debug_assertions)]
-        assert_eq!(entity, self.entities[dense_index]);
+        assert_eq!(entity, self.entities[dense_index.as_usize()]);
         // SAFETY: if the sparse index points to something in the dense vec, it exists
-        unsafe {
-            Some(
-                self.dense
-                    .get_added_tick_unchecked(TableRow::new(dense_index)),
-            )
-        }
+        unsafe { Some(self.dense.get_added_tick_unchecked(dense_index)) }
     }
 
     /// Returns a reference to the "changed" tick of the entity's component value.
     ///
     /// Returns `None` if `entity` does not have a component in the sparse set.
     #[inline]
     pub fn get_changed_tick(&self, entity: Entity) -> Option<&UnsafeCell<Tick>> {
-        let dense_index = *self.sparse.get(entity.index())? as usize;
+        let dense_index = *self.sparse.get(entity.index())?;
         #[cfg(debug_assertions)]
-        assert_eq!(entity, self.entities[dense_index]);
+        assert_eq!(entity, self.entities[dense_index.as_usize()]);
         // SAFETY: if the sparse index points to something in the dense vec, it exists
-        unsafe {
-            Some(
-                self.dense
-                    .get_changed_tick_unchecked(TableRow::new(dense_index)),
-            )
-        }
+        unsafe { Some(self.dense.get_changed_tick_unchecked(dense_index)) }
     }
 
     /// Returns a reference to the "added" and "changed" ticks of the entity's component value.
     ///
     /// Returns `None` if `entity` does not have a component in the sparse set.
     #[inline]
     pub fn get_ticks(&self, entity: Entity) -> Option<ComponentTicks> {
-        let dense_index = *self.sparse.get(entity.index())? as usize;
+        let dense_index = *self.sparse.get(entity.index())?;
         #[cfg(debug_assertions)]
-        assert_eq!(entity, self.entities[dense_index]);
+        assert_eq!(entity, self.entities[dense_index.as_usize()]);
         // SAFETY: if the sparse index points to something in the dense vec, it exists
-        unsafe { Some(self.dense.get_ticks_unchecked(TableRow::new(dense_index))) }
+        unsafe { Some(self.dense.get_ticks_unchecked(dense_index)) }
     }
 
     /// 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(crate) fn remove_and_forget(&mut self, entity: Entity) -> Option<OwningPtr<'_>> {
         self.sparse.remove(entity.index()).map(|dense_index| {
-            let dense_index = dense_index as usize;
             #[cfg(debug_assertions)]
-            assert_eq!(entity, self.entities[dense_index]);
-            self.entities.swap_remove(dense_index);
-            let is_last = dense_index == self.dense.len() - 1;
+            assert_eq!(entity, self.entities[dense_index.as_usize()]);
+            self.entities.swap_remove(dense_index.as_usize());
+            let is_last = dense_index.as_usize() == 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(TableRow::new(dense_index))
-            };
+            let (value, _) = unsafe { self.dense.swap_remove_and_forget_unchecked(dense_index) };
             if !is_last {
-                let swapped_entity = self.entities[dense_index];
+                let swapped_entity = self.entities[dense_index.as_usize()];
                 #[cfg(not(debug_assertions))]
                 let index = swapped_entity;
                 #[cfg(debug_assertions)]
                 let index = swapped_entity.index();
-                *self.sparse.get_mut(index).unwrap() = dense_index as u32;
+                *self.sparse.get_mut(index).unwrap() = dense_index;
             }
             value
         })
@@ -316,20 +301,21 @@ impl ComponentSparseSet {
     /// Returns `true` if `entity` had a component value in the sparse set.
     pub(crate) fn remove(&mut self, entity: Entity) -> bool {
         if let Some(dense_index) = self.sparse.remove(entity.index()) {
-            let dense_index = dense_index as usize;
             #[cfg(debug_assertions)]
-            assert_eq!(entity, self.entities[dense_index]);
-            self.entities.swap_remove(dense_index);
-            let is_last = dense_index == self.dense.len() - 1;
+            assert_eq!(entity, self.entities[dense_index.as_usize()]);
+            self.entities.swap_remove(dense_index.as_usize());
+            let is_last = dense_index.as_usize() == self.dense.len() - 1;
             // SAFETY: if the sparse index points to something in the dense vec, it exists
-            unsafe { self.dense.swap_remove_unchecked(TableRow::new(dense_index)) }
+            unsafe {
+                self.dense.swap_remove_unchecked(dense_index);
+            }
             if !is_last {
-                let swapped_entity = self.entities[dense_index];
+                let swapped_entity = self.entities[dense_index.as_usize()];
                 #[cfg(not(debug_assertions))]
                 let index = swapped_entity;
                 #[cfg(debug_assertions)]
                 let index = swapped_entity.index();
-                *self.sparse.get_mut(index).unwrap() = dense_index as u32;
+                *self.sparse.get_mut(index).unwrap() = dense_index;
             }
             true
         } else {