Skip to content

Add BundleRemover #18521

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.

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
merged 12 commits into from
May 6, 2025
273 changes: 272 additions & 1 deletion crates/bevy_ecs/src/bundle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@ use crate::{
query::DebugCheckedUnwrap,
relationship::RelationshipHookMode,
storage::{SparseSetIndex, SparseSets, Storages, Table, TableRow},
world::{unsafe_world_cell::UnsafeWorldCell, EntityWorldMut, ON_ADD, ON_INSERT, ON_REPLACE},
world::{
unsafe_world_cell::UnsafeWorldCell, EntityWorldMut, ON_ADD, ON_INSERT, ON_REMOVE,
ON_REPLACE,
},
};
use alloc::{boxed::Box, vec, vec::Vec};
use bevy_platform_support::collections::{HashMap, HashSet};
Expand Down Expand Up @@ -1361,6 +1364,274 @@ impl<'w> BundleInserter<'w> {
}
}

// SAFETY: We have exclusive world access so our pointers can't be invalidated externally
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This safety comment is on a struct rather than an unsafe block, so I don't think it will get seen. Did you mean to put it somewhere else, or to put it as a doc comment for the struct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just copied from BundleInserter. I don't mind it, but I can understand your point. I wouldn't be opposed to moving it for all Bundle*er maybe in a different pr.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, okay, I didn't realize this was an existing pattern. That makes sense, then!

pub(crate) struct BundleRemover<'w> {
world: UnsafeWorldCell<'w>,
bundle_info: ConstNonNull<BundleInfo>,
old_and_new_table: Option<(NonNull<Table>, NonNull<Table>)>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth trying to make this look more like BundleInserter? That records the current table and archetype pointers along with an enum ArchetypeMoveType that has the new ones.

Although I suppose one major difference is that you don't need to do anything on a remove that doesn't change archetypes, while an insert might be changing the values.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been a while since I wrote this, so I don't remember the details. But IIRC, I originally tried to do this with effectively the same structure as BundleInserter and ended up running into issues. I think it was easier to justify safety where accessing the new table mutably could be the same as the old table's mutable pointer. I could take another stab at it, but I don't feel it would be worth it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree it's not worth revisiting. I was just surprised that it looked so different! But if you already tried it that way then there must be a good reason for the differences!

old_archetype: NonNull<Archetype>,
new_archetype: NonNull<Archetype>,
}

impl<'w> BundleRemover<'w> {
/// Creates a new [`BundleRemover`], if such a remover would do anything.
///
/// If `require_all` is true, the [`BundleRemover`] is only created if the entire bundle is present on the archetype.
///
/// # Safety
/// Caller must ensure that `archetype_id` is valid
#[inline]
pub(crate) unsafe fn new<T: Bundle>(
world: &'w mut World,
archetype_id: ArchetypeId,
require_all: bool,
) -> Option<Self> {
// SAFETY: These come from the same world. `world.components_registrator` can't be used since we borrow other fields too.
let mut registrator =
unsafe { ComponentsRegistrator::new(&mut world.components, &mut world.component_ids) };
let bundle_id = world
.bundles
.register_info::<T>(&mut registrator, &mut world.storages);
// SAFETY: we initialized this bundle_id in `init_info`, and caller ensures archetype is valid.
unsafe { Self::new_with_id(world, archetype_id, bundle_id, require_all) }
}

/// Creates a new [`BundleRemover`], if such a remover would do anything.
///
/// If `require_all` is true, the [`BundleRemover`] is only created if the entire bundle is present on the archetype.
///
/// # Safety
/// Caller must ensure that `bundle_id` exists in `world.bundles` and `archetype_id` is valid.
#[inline]
pub(crate) unsafe fn new_with_id(
world: &'w mut World,
archetype_id: ArchetypeId,
bundle_id: BundleId,
require_all: bool,
) -> Option<Self> {
let bundle_info = world.bundles.get_unchecked(bundle_id);
// SAFETY: Caller ensures archetype and bundle ids are correct.
let new_archetype_id = unsafe {
bundle_info.remove_bundle_from_archetype(
&mut world.archetypes,
&mut world.storages,
&world.components,
&world.observers,
archetype_id,
!require_all,
)?
};
if new_archetype_id == archetype_id {
return None;
}
let (old_archetype, new_archetype) =
world.archetypes.get_2_mut(archetype_id, new_archetype_id);

let tables = if old_archetype.table_id() == new_archetype.table_id() {
None
} else {
let (old, new) = world
.storages
.tables
.get_2_mut(old_archetype.table_id(), new_archetype.table_id());
Some((old.into(), new.into()))
};

Some(Self {
bundle_info: bundle_info.into(),
new_archetype: new_archetype.into(),
old_archetype: old_archetype.into(),
old_and_new_table: tables,
world: world.as_unsafe_world_cell(),
})
}

/// This can be passed to [`remove`](Self::remove) as the `pre_remove` function if you don't want to do anything before removing.
pub fn empty_pre_remove(
_: &mut SparseSets,
_: Option<&mut Table>,
_: &Components,
_: &[ComponentId],
) -> (bool, ()) {
(true, ())
}

/// Performs the removal.
///
/// `pre_remove` should return a bool for if the components still need to be dropped.
///
/// # Safety
/// The `location` must have the same archetype as the remover.
#[inline]
pub(crate) unsafe fn remove<T: 'static>(
&mut self,
entity: Entity,
location: EntityLocation,
caller: MaybeLocation,
pre_remove: impl FnOnce(
&mut SparseSets,
Option<&mut Table>,
&Components,
&[ComponentId],
) -> (bool, T),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old code intentionally used a const generic instead of an ordinary bool to make sure that the branches optimized away. Do we need to preserve that?

Even if we don't, it looks like there are only two functions passed as pre_remove, and one has hard-coded true and the other hard-coded false. It might be simpler to make a separate drop: bool parameter to avoid the tuple and make it clear that it doesn't depend on the values passed to pre_remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old code intentionally used a const generic instead of an ordinary bool to make sure that the branches optimized away. Do we need to preserve that?

It's an impl Fn, and with only two closures types, monomorphization will effectively the same thing. They are both generic, and both bools are constants.

It might be simpler to make a separate drop: bool parameter to avoid the tuple and make it clear that it doesn't depend on the values passed to pre_remove.

I mean, we could add that restriction, but I don't see the point. Letting it depend on it seems more flexible for the future.

) -> (EntityLocation, T) {
// Hooks
// SAFETY: all bundle components exist in World
unsafe {
// SAFETY: We only keep access to archetype/bundle data.
let mut deferred_world = self.world.into_deferred();
let bundle_components_in_archetype = || {
self.bundle_info
.as_ref()
.iter_explicit_components()
.filter(|component_id| self.old_archetype.as_ref().contains(*component_id))
};
if self.old_archetype.as_ref().has_replace_observer() {
deferred_world.trigger_observers(
ON_REPLACE,
entity,
bundle_components_in_archetype(),
caller,
);
}
deferred_world.trigger_on_replace(
self.old_archetype.as_ref(),
entity,
bundle_components_in_archetype(),
caller,
RelationshipHookMode::Run,
);
if self.old_archetype.as_ref().has_remove_observer() {
deferred_world.trigger_observers(
ON_REMOVE,
entity,
bundle_components_in_archetype(),
caller,
);
}
deferred_world.trigger_on_remove(
self.old_archetype.as_ref(),
entity,
bundle_components_in_archetype(),
caller,
);
}

// SAFETY: We still have the cell, so this is unique, it doesn't conflict with other references, and we drop it shortly.
let world = unsafe { self.world.world_mut() };

let (needs_drop, pre_remove_result) = pre_remove(
&mut world.storages.sparse_sets,
self.old_and_new_table
.as_ref()
// SAFETY: There is no conflicting access for this scope.
.map(|(old, _)| unsafe { &mut *old.as_ptr() }),
&world.components,
self.bundle_info.as_ref().explicit_components(),
);

// Handle sparse set removes
for component_id in self.bundle_info.as_ref().iter_explicit_components() {
if self.old_archetype.as_ref().contains(component_id) {
world.removed_components.send(component_id, entity);

// Make sure to drop components stored in sparse sets.
// Dense components are dropped later in `move_to_and_drop_missing_unchecked`.
if let Some(StorageType::SparseSet) =
self.old_archetype.as_ref().get_storage_type(component_id)
{
world
.storages
.sparse_sets
.get_mut(component_id)
// Set exists because the component existed on the entity
.unwrap()
// If it was already forgotten, it would not be in the set.
.remove(entity);
}
}
}

// Handle archetype change
let remove_result = self
.old_archetype
.as_mut()
.swap_remove(location.archetype_row);
// if an entity was moved into this entity's archetype row, update its archetype row
if let Some(swapped_entity) = remove_result.swapped_entity {
let swapped_location = world.entities.get(swapped_entity).unwrap();

world.entities.set(
swapped_entity.index(),
EntityLocation {
archetype_id: swapped_location.archetype_id,
archetype_row: location.archetype_row,
table_id: swapped_location.table_id,
table_row: swapped_location.table_row,
},
);
}

// Handle table change
let new_location = if let Some((mut old_table, mut new_table)) = self.old_and_new_table {
let move_result = if needs_drop {
// SAFETY: old_table_row exists
unsafe {
old_table
.as_mut()
.move_to_and_drop_missing_unchecked(location.table_row, new_table.as_mut())
}
} else {
// SAFETY: old_table_row exists
unsafe {
old_table.as_mut().move_to_and_forget_missing_unchecked(
location.table_row,
new_table.as_mut(),
)
}
};

// SAFETY: move_result.new_row is a valid position in new_archetype's table
let new_location = unsafe {
self.new_archetype
.as_mut()
.allocate(entity, move_result.new_row)
};

// if an entity was moved into this entity's table row, update its table row
if let Some(swapped_entity) = move_result.swapped_entity {
let swapped_location = world.entities.get(swapped_entity).unwrap();

world.entities.set(
swapped_entity.index(),
EntityLocation {
archetype_id: swapped_location.archetype_id,
archetype_row: swapped_location.archetype_row,
table_id: swapped_location.table_id,
table_row: location.table_row,
},
);
world.archetypes[swapped_location.archetype_id]
.set_entity_table_row(swapped_location.archetype_row, location.table_row);
}

new_location
} else {
// The tables are the same
self.new_archetype
.as_mut()
.allocate(entity, location.table_row)
};

// SAFETY: The entity is valid and has been moved to the new location already.
unsafe {
world.entities.set(entity.index(), new_location);
}

(new_location, pre_remove_result)
}
}

// SAFETY: We have exclusive world access so our pointers can't be invalidated externally
pub(crate) struct BundleSpawner<'w> {
world: UnsafeWorldCell<'w>,
Expand Down
Loading