Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Merged by Bors] - Use u32 over usize for ComponentSparseSet indicies #4723

Closed
wants to merge 3 commits into from
Closed

[Merged by Bors] - Use u32 over usize for ComponentSparseSet indicies #4723

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4a51aef
Use u32 over usize for ComponentSparseSet indicies
james7132 May 11, 2022
ec00802
Get the ones that were missed
james7132 May 11, 2022
d53d503
Add #[cfg(debug_assertions)] conditional checks
james7132 May 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 52 additions & 14 deletions crates/bevy_ecs/src/storage/sparse_set.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ use crate::{
use bevy_ptr::{OwningPtr, Ptr};
use std::{cell::UnsafeCell, hash::Hash, marker::PhantomData};

type EntityId = u32;

#[derive(Debug)]
pub struct SparseArray<I, V = I> {
values: Vec<Option<V>>,
Expand Down Expand Up @@ -96,8 +98,14 @@ impl<I: SparseSetIndex, V> SparseArray<I, V> {
pub struct ComponentSparseSet {
james7132 marked this conversation as resolved.
Show resolved Hide resolved
dense: BlobVec,
ticks: Vec<UnsafeCell<ComponentTicks>>,
// Internally this only relies on the Entity ID to keep track of where the component data is
// stored for entities that are alive. The generation is not required, but is stored
// in debug builds to validate that access is correct.
#[cfg(not(debug_assertions))]
entities: Vec<EntityId>,
james7132 marked this conversation as resolved.
Show resolved Hide resolved
#[cfg(debug_assertions)]
entities: Vec<Entity>,
sparse: SparseArray<Entity, usize>,
sparse: SparseArray<EntityId, u32>,
}

impl ComponentSparseSet {
Expand Down Expand Up @@ -137,38 +145,55 @@ impl ComponentSparseSet {
/// The `value` pointer must point to a valid address that matches the [`Layout`](std::alloc::Layout)
/// inside the [`ComponentInfo`] given when constructing this sparse set.
pub unsafe fn insert(&mut self, entity: Entity, value: OwningPtr<'_>, change_tick: u32) {
if let Some(&dense_index) = self.sparse.get(entity) {
self.dense.replace_unchecked(dense_index, value);
*self.ticks.get_unchecked_mut(dense_index) =
if let Some(&dense_index) = self.sparse.get(entity.id()) {
debug_assert_eq!(entity, self.entities[dense_index as usize]);
let _entity = self.dense.replace_unchecked(dense_index as usize, value);
*self.ticks.get_unchecked_mut(dense_index as usize) =
UnsafeCell::new(ComponentTicks::new(change_tick));
} else {
let dense_index = self.dense.len();
self.dense.push(value);
self.sparse.insert(entity, dense_index);
self.sparse.insert(entity.id(), dense_index as u32);
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)));
#[cfg(not(debug_assertions))]
self.entities.push(entity.id());
#[cfg(debug_assertions)]
self.entities.push(entity);
}
}

#[inline]
pub fn contains(&self, entity: Entity) -> bool {
self.sparse.contains(entity)
#[cfg(debug_assertions)]
{
if let Some(&dense_index) = self.sparse.get(entity.id()) {
debug_assert_eq!(entity, self.entities[dense_index as usize]);
true
} else {
false
}
}
#[cfg(not(debug_assertions))]
self.sparse.contains(entity.id())
james7132 marked this conversation as resolved.
Show resolved Hide resolved
}

#[inline]
pub fn get(&self, entity: Entity) -> Option<Ptr<'_>> {
self.sparse.get(entity).map(|dense_index| {
self.sparse.get(entity.id()).map(|dense_index| {
let dense_index = *dense_index as usize;
debug_assert_eq!(entity, self.entities[dense_index]);
// SAFE: if the sparse index points to something in the dense vec, it exists
unsafe { self.dense.get_unchecked(*dense_index) }
unsafe { self.dense.get_unchecked(dense_index) }
})
}

#[inline]
pub fn get_with_ticks(&self, entity: Entity) -> Option<(Ptr<'_>, &UnsafeCell<ComponentTicks>)> {
let dense_index = *self.sparse.get(entity)?;
let dense_index = *self.sparse.get(entity.id())? as usize;
debug_assert_eq!(entity, self.entities[dense_index]);
// SAFE: if the sparse index points to something in the dense vec, it exists
unsafe {
Some((
Expand All @@ -180,7 +205,8 @@ impl ComponentSparseSet {

#[inline]
pub fn get_ticks(&self, entity: Entity) -> Option<&UnsafeCell<ComponentTicks>> {
let dense_index = *self.sparse.get(entity)?;
let dense_index = *self.sparse.get(entity.id())? as usize;
debug_assert_eq!(entity, self.entities[dense_index]);
// SAFE: if the sparse index points to something in the dense vec, it exists
unsafe { Some(self.ticks.get_unchecked(dense_index)) }
}
Expand All @@ -189,30 +215,42 @@ impl ComponentSparseSet {
/// it exists).
#[must_use = "The returned pointer must be used to drop the removed component."]
pub fn remove_and_forget(&mut self, entity: Entity) -> Option<OwningPtr<'_>> {
self.sparse.remove(entity).map(|dense_index| {
self.sparse.remove(entity.id()).map(|dense_index| {
let dense_index = dense_index as usize;
debug_assert_eq!(entity, self.entities[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
let value = unsafe { self.dense.swap_remove_and_forget_unchecked(dense_index) };
if !is_last {
let swapped_entity = self.entities[dense_index];
*self.sparse.get_mut(swapped_entity).unwrap() = dense_index;
#[cfg(not(debug_assertions))]
let idx = swapped_entity;
#[cfg(debug_assertions)]
let idx = swapped_entity.id();
*self.sparse.get_mut(idx).unwrap() = dense_index as u32;
}
value
})
}

pub fn remove(&mut self, entity: Entity) -> bool {
if let Some(dense_index) = self.sparse.remove(entity) {
if let Some(dense_index) = self.sparse.remove(entity.id()) {
let dense_index = dense_index as usize;
debug_assert_eq!(entity, self.entities[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
unsafe { self.dense.swap_remove_and_drop_unchecked(dense_index) }
if !is_last {
let swapped_entity = self.entities[dense_index];
*self.sparse.get_mut(swapped_entity).unwrap() = dense_index;
#[cfg(not(debug_assertions))]
let idx = swapped_entity;
#[cfg(debug_assertions)]
let idx = swapped_entity.id();
*self.sparse.get_mut(idx).unwrap() = dense_index as u32;
}
true
} else {
Expand Down