From ebb731885b1637295add707390fb890291f7d2fa Mon Sep 17 00:00:00 2001 From: Elliott Pierce Date: Mon, 24 Mar 2025 18:37:39 -0400 Subject: [PATCH 01/11] create the remover --- crates/bevy_ecs/src/bundle.rs | 257 +++++++++++++++++++++++++++++++++- 1 file changed, 256 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index d87ef517f15d2..944c43344ab06 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -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}; @@ -1361,6 +1364,258 @@ impl<'w> BundleInserter<'w> { } } +// SAFETY: We have exclusive world access so our pointers can't be invalidated externally +pub(crate) struct BundleRemover<'w> { + world: UnsafeWorldCell<'w>, + bundle_info: ConstNonNull, + old_and_new_table: Option<(NonNull, NonNull
)>, + old_archetype: NonNull, + new_archetype: NonNull, + change_tick: Tick, +} + +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. + #[inline] + pub(crate) fn new( + world: &'w mut World, + archetype_id: ArchetypeId, + require_all: bool, + change_tick: Tick, + ) -> Option { + // 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::(&mut registrator, &mut world.storages); + // SAFETY: we initialized this bundle_id in `init_info` + unsafe { Self::new_with_id(world, archetype_id, bundle_id, require_all, change_tick) } + } + + /// 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, + change_tick: Tick, + ) -> Option { + let bundle_info = world.bundles.get_unchecked(bundle_id); + // SAFETY: Ensured by caller and that intersections are never `None`. + 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, + change_tick, + world: world.as_unsafe_world_cell(), + }) + } + + /// Performs the removal. + /// + /// `pre_remove` should return a bool for if the components still need to be dropped. + /// + /// If `intersection` is true, the removal will go through with all overlapping components. + /// Otherwise, returns `None` if not all components are available for removal. + /// + /// # Safety + /// The `location` must have the same archetype as the remover. + #[inline] + pub(crate) unsafe fn remove( + &mut self, + entity: Entity, + location: EntityLocation, + caller: MaybeLocation, + pre_remove: impl FnOnce(&mut Storages, &[ComponentId]) -> bool, + ) -> Option { + // 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( + &mut world.storages, + self.bundle_info.as_ref().explicit_components(), + ); + + // Handle basic 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); + } + + Some(new_location) + } +} + // SAFETY: We have exclusive world access so our pointers can't be invalidated externally pub(crate) struct BundleSpawner<'w> { world: UnsafeWorldCell<'w>, From c08527ff14f201f9794d7fd9a555f130dd60e8f4 Mon Sep 17 00:00:00 2001 From: Elliott Pierce Date: Mon, 24 Mar 2025 19:10:24 -0400 Subject: [PATCH 02/11] use remover --- crates/bevy_ecs/src/bundle.rs | 11 +- crates/bevy_ecs/src/world/entity_ref.rs | 454 +++++++++--------------- 2 files changed, 175 insertions(+), 290 deletions(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index 944c43344ab06..634f09fe14580 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -1457,13 +1457,13 @@ impl<'w> BundleRemover<'w> { /// # Safety /// The `location` must have the same archetype as the remover. #[inline] - pub(crate) unsafe fn remove( + pub(crate) unsafe fn remove( &mut self, entity: Entity, location: EntityLocation, caller: MaybeLocation, - pre_remove: impl FnOnce(&mut Storages, &[ComponentId]) -> bool, - ) -> Option { + pre_remove: impl FnOnce(&mut Storages, &Components, &[ComponentId]) -> (bool, T), + ) -> Option<(EntityLocation, T)> { // Hooks // SAFETY: all bundle components exist in World unsafe { @@ -1509,8 +1509,9 @@ impl<'w> BundleRemover<'w> { // 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( + let (needs_drop, pre_remove_result) = pre_remove( &mut world.storages, + world.components(), self.bundle_info.as_ref().explicit_components(), ); @@ -1612,7 +1613,7 @@ impl<'w> BundleRemover<'w> { world.entities.set(entity.index(), new_location); } - Some(new_location) + Some((new_location, pre_remove_result)) } } diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index 3813d34745f32..e57b92f34d589 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -1,7 +1,7 @@ use crate::{ - archetype::{Archetype, ArchetypeId, Archetypes}, + archetype::{Archetype, ArchetypeId}, bundle::{ - Bundle, BundleEffect, BundleFromComponents, BundleId, BundleInfo, BundleInserter, + Bundle, BundleEffect, BundleFromComponents, BundleInfo, BundleInserter, BundleRemover, DynamicBundle, InsertMode, }, change_detection::{MaybeLocation, MutUntyped}, @@ -10,12 +10,12 @@ use crate::{ StorageType, }, entity::{ - Entities, Entity, EntityBorrow, EntityCloner, EntityClonerBuilder, EntityLocation, + Entity, EntityBorrow, EntityCloner, EntityClonerBuilder, EntityLocation, TrustedEntityBorrow, }, event::Event, observer::Observer, - query::{Access, ReadOnlyQueryData}, + query::{Access, DebugCheckedUnwrap, ReadOnlyQueryData}, relationship::RelationshipHookMode, removal_detection::RemovedComponentEvents, resource::Resource, @@ -1766,276 +1766,51 @@ impl<'w> EntityWorldMut<'w> { #[track_caller] pub fn take(&mut self) -> Option { self.assert_not_despawned(); - let world = &mut self.world; - let storages = &mut world.storages; - // SAFETY: These come from the same world. - let mut registrator = - unsafe { ComponentsRegistrator::new(&mut world.components, &mut world.component_ids) }; - let bundle_id = world.bundles.register_info::(&mut registrator, storages); - // SAFETY: We just ensured this bundle exists - let bundle_info = unsafe { world.bundles.get_unchecked(bundle_id) }; - let old_location = self.location; - // SAFETY: `archetype_id` exists because it is referenced in the old `EntityLocation` which is valid, - // components exist in `bundle_info` because `Bundles::init_info` initializes a `BundleInfo` containing all components of the bundle type `T` - let new_archetype_id = unsafe { - bundle_info.remove_bundle_from_archetype( - &mut world.archetypes, - storages, - ®istrator, - &world.observers, - old_location.archetype_id, - false, - )? - }; - - if new_archetype_id == old_location.archetype_id { - return None; - } - let entity = self.entity; - // SAFETY: Archetypes and Bundles cannot be mutably aliased through DeferredWorld - let (old_archetype, bundle_info, mut deferred_world) = unsafe { - let bundle_info: *const BundleInfo = bundle_info; - let world = world.as_unsafe_world_cell(); - ( - &world.archetypes()[old_location.archetype_id], - &*bundle_info, - world.into_deferred(), - ) - }; + let location = self.location; + let tick = self.world.change_tick(); - // SAFETY: all bundle components exist in World - unsafe { - trigger_on_replace_and_on_remove_hooks_and_observers( - &mut deferred_world, - old_archetype, + let mut remover = + BundleRemover::new::(self.world, self.location.archetype_id, true, tick)?; + // SAFETY: The passed location has the sane archetype as the remover, since they came from the same location. + let (new_location, result) = unsafe { + remover.remove( entity, - bundle_info, + location, MaybeLocation::caller(), - ); - } - - let archetypes = &mut world.archetypes; - let storages = &mut world.storages; - let components = &mut world.components; - let entities = &mut world.entities; - let removed_components = &mut world.removed_components; - - let entity = self.entity; - let mut bundle_components = bundle_info.iter_explicit_components(); - // SAFETY: bundle components are iterated in order, which guarantees that the component type - // matches - let result = unsafe { - T::from_components(storages, &mut |storages| { - let component_id = bundle_components.next().unwrap(); - // SAFETY: - // - entity location is valid - // - table row is removed below, without dropping the contents - // - `components` comes from the same world as `storages` - // - the component exists on the entity - take_component( - storages, - components, - removed_components, - component_id, - entity, - old_location, - ) - }) + |storages, components, bundle_components| { + let mut bundle_components = bundle_components.iter().copied(); + ( + false, + T::from_components(storages, &mut |storages| { + let component_id = bundle_components.next().unwrap(); + // SAFETY: the component existed to be removed, so its id must be valid. + let component_info = components.get_info_unchecked(component_id); + match component_info.storage_type() { + StorageType::Table => { + let table = &mut storages.tables[location.table_id]; + // SAFETY: The remover is cleaning this up. + table.take_component(component_id, location.table_row) + } + StorageType::SparseSet => storages + .sparse_sets + .get_mut(component_id) + .unwrap() + .remove_and_forget(entity) + .unwrap(), + } + }), + ) + }, + )? }; + self.location = new_location; - #[expect( - clippy::undocumented_unsafe_blocks, - reason = "Needs to be documented; see #17345." - )] - unsafe { - Self::move_entity_from_remove::( - entity, - &mut self.location, - old_location.archetype_id, - old_location, - entities, - archetypes, - storages, - new_archetype_id, - ); - } self.world.flush(); self.update_location(); Some(result) } - /// # Safety - /// - /// `new_archetype_id` must have the same or a subset of the components - /// in `old_archetype_id`. Probably more safety stuff too, audit a call to - /// this fn as if the code here was written inline - /// - /// when DROP is true removed components will be dropped otherwise they will be forgotten - // We use a const generic here so that we are less reliant on - // inlining for rustc to optimize out the `match DROP` - unsafe fn move_entity_from_remove( - entity: Entity, - self_location: &mut EntityLocation, - old_archetype_id: ArchetypeId, - old_location: EntityLocation, - entities: &mut Entities, - archetypes: &mut Archetypes, - storages: &mut Storages, - new_archetype_id: ArchetypeId, - ) { - let old_archetype = &mut archetypes[old_archetype_id]; - let remove_result = old_archetype.swap_remove(old_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 = entities.get(swapped_entity).unwrap(); - - entities.set( - swapped_entity.index(), - EntityLocation { - archetype_id: swapped_location.archetype_id, - archetype_row: old_location.archetype_row, - table_id: swapped_location.table_id, - table_row: swapped_location.table_row, - }, - ); - } - let old_table_row = remove_result.table_row; - let old_table_id = old_archetype.table_id(); - let new_archetype = &mut archetypes[new_archetype_id]; - - let new_location = if old_table_id == new_archetype.table_id() { - new_archetype.allocate(entity, old_table_row) - } else { - let (old_table, new_table) = storages - .tables - .get_2_mut(old_table_id, new_archetype.table_id()); - - let move_result = if DROP { - // SAFETY: old_table_row exists - unsafe { old_table.move_to_and_drop_missing_unchecked(old_table_row, new_table) } - } else { - // SAFETY: old_table_row exists - unsafe { old_table.move_to_and_forget_missing_unchecked(old_table_row, new_table) } - }; - - // SAFETY: move_result.new_row is a valid position in new_archetype's table - let new_location = unsafe { new_archetype.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 = entities.get(swapped_entity).unwrap(); - - 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: old_location.table_row, - }, - ); - archetypes[swapped_location.archetype_id] - .set_entity_table_row(swapped_location.archetype_row, old_table_row); - } - - new_location - }; - - *self_location = new_location; - // SAFETY: The entity is valid and has been moved to the new location already. - unsafe { - entities.set(entity.index(), new_location); - } - } - - /// Remove the components of `bundle` from `entity`. - /// - /// # Safety - /// - A `BundleInfo` with the corresponding `BundleId` must have been initialized. - unsafe fn remove_bundle(&mut self, bundle: BundleId, caller: MaybeLocation) -> EntityLocation { - let entity = self.entity; - let world = &mut self.world; - let location = self.location; - // SAFETY: the caller guarantees that the BundleInfo for this id has been initialized. - let bundle_info = world.bundles.get_unchecked(bundle); - - // SAFETY: `archetype_id` exists because it is referenced in `location` which is valid - // and components in `bundle_info` must exist due to this function's safety invariants. - let new_archetype_id = bundle_info - .remove_bundle_from_archetype( - &mut world.archetypes, - &mut world.storages, - &world.components, - &world.observers, - location.archetype_id, - // components from the bundle that are not present on the entity are ignored - true, - ) - .expect("intersections should always return a result"); - - if new_archetype_id == location.archetype_id { - return location; - } - - // SAFETY: Archetypes and Bundles cannot be mutably aliased through DeferredWorld - let (old_archetype, bundle_info, mut deferred_world) = unsafe { - let bundle_info: *const BundleInfo = bundle_info; - let world = world.as_unsafe_world_cell(); - ( - &world.archetypes()[location.archetype_id], - &*bundle_info, - world.into_deferred(), - ) - }; - - // SAFETY: all bundle components exist in World - unsafe { - trigger_on_replace_and_on_remove_hooks_and_observers( - &mut deferred_world, - old_archetype, - entity, - bundle_info, - caller, - ); - } - - let old_archetype = &world.archetypes[location.archetype_id]; - for component_id in bundle_info.iter_explicit_components() { - if old_archetype.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) = old_archetype.get_storage_type(component_id) { - world - .storages - .sparse_sets - .get_mut(component_id) - // Set exists because the component existed on the entity - .unwrap() - .remove(entity); - } - } - } - - // SAFETY: `new_archetype_id` is a subset of the components in `old_location.archetype_id` - // because it is created by removing a bundle from these components. - let mut new_location = location; - Self::move_entity_from_remove::( - entity, - &mut new_location, - location.archetype_id, - location, - &mut world.entities, - &mut world.archetypes, - &mut world.storages, - new_archetype_id, - ); - - new_location - } - /// Removes any components in the [`Bundle`] from the entity. /// /// See [`EntityCommands::remove`](crate::system::EntityCommands::remove) for more details. @@ -2052,18 +1827,22 @@ impl<'w> EntityWorldMut<'w> { #[inline] pub(crate) fn remove_with_caller(&mut self, caller: MaybeLocation) -> &mut Self { self.assert_not_despawned(); - let storages = &mut self.world.storages; - // SAFETY: These come from the same world. - let mut registrator = unsafe { - ComponentsRegistrator::new(&mut self.world.components, &mut self.world.component_ids) + let tick = self.world.change_tick(); + + let Some(mut remover) = + BundleRemover::new::(self.world, self.location.archetype_id, false, tick) + else { + return self; }; - let bundle_info = self - .world - .bundles - .register_info::(&mut registrator, storages); + // SAFETY: The remover archetype came from the passed location and the removal can not fail. + let new_location = unsafe { + remover + .remove(self.entity, self.location, caller, |_, _, _| (true, ())) + .debug_checked_unwrap() + } + .0; - // SAFETY: the `BundleInfo` is initialized above - self.location = unsafe { self.remove_bundle(bundle_info, caller) }; + self.location = new_location; self.world.flush(); self.update_location(); self @@ -2085,16 +1864,36 @@ impl<'w> EntityWorldMut<'w> { ) -> &mut Self { self.assert_not_despawned(); let storages = &mut self.world.storages; + let bundles = &mut self.world.bundles; // SAFETY: These come from the same world. let mut registrator = unsafe { ComponentsRegistrator::new(&mut self.world.components, &mut self.world.component_ids) }; - let bundles = &mut self.world.bundles; - let bundle_id = bundles.register_contributed_bundle_info::(&mut registrator, storages); - // SAFETY: the dynamic `BundleInfo` is initialized above - self.location = unsafe { self.remove_bundle(bundle_id, caller) }; + let tick = self.world.change_tick(); + + // SAFETY: We just created the bundle, and the archetype is valid, since we are in it. + let Some(mut remover) = (unsafe { + BundleRemover::new_with_id( + self.world, + self.location.archetype_id, + bundle_id, + false, + tick, + ) + }) else { + return self; + }; + // SAFETY: The remover archetype came from the passed location and the removal can not fail. + let new_location = unsafe { + remover + .remove(self.entity, self.location, caller, |_, _, _| (true, ())) + .debug_checked_unwrap() + } + .0; + + self.location = new_location; self.world.flush(); self.update_location(); self @@ -2141,8 +1940,29 @@ impl<'w> EntityWorldMut<'w> { .bundles .init_dynamic_info(&mut self.world.storages, ®istrator, to_remove); - // SAFETY: the `BundleInfo` for the components to remove is initialized above - self.location = unsafe { self.remove_bundle(remove_bundle, caller) }; + let tick = self.world.change_tick(); + + // SAFETY: We just created the bundle, and the archetype is valid, since we are in it. + let Some(mut remover) = (unsafe { + BundleRemover::new_with_id( + self.world, + self.location.archetype_id, + remove_bundle, + false, + tick, + ) + }) else { + return self; + }; + // SAFETY: The remover archetype came from the passed location and the removal can not fail. + let new_location = unsafe { + remover + .remove(self.entity, self.location, caller, |_, _, _| (true, ())) + .debug_checked_unwrap() + } + .0; + + self.location = new_location; self.world.flush(); self.update_location(); self @@ -2176,8 +1996,28 @@ impl<'w> EntityWorldMut<'w> { component_id, ); - // SAFETY: the `BundleInfo` for this `component_id` is initialized above - self.location = unsafe { self.remove_bundle(bundle_id, caller) }; + let tick = self.world.change_tick(); + // SAFETY: We just created the bundle, and the archetype is valid, since we are in it. + let Some(mut remover) = (unsafe { + BundleRemover::new_with_id( + self.world, + self.location.archetype_id, + bundle_id, + false, + tick, + ) + }) else { + return self; + }; + // SAFETY: The remover archetype came from the passed location and the removal can not fail. + let new_location = unsafe { + remover + .remove(self.entity, self.location, caller, |_, _, _| (true, ())) + .debug_checked_unwrap() + } + .0; + + self.location = new_location; self.world.flush(); self.update_location(); self @@ -2202,9 +2042,33 @@ impl<'w> EntityWorldMut<'w> { component_ids, ); - // SAFETY: the `BundleInfo` for this `bundle_id` is initialized above - unsafe { self.remove_bundle(bundle_id, MaybeLocation::caller()) }; + let tick = self.world.change_tick(); + // SAFETY: We just created the bundle, and the archetype is valid, since we are in it. + let Some(mut remover) = (unsafe { + BundleRemover::new_with_id( + self.world, + self.location.archetype_id, + bundle_id, + false, + tick, + ) + }) else { + return self; + }; + // SAFETY: The remover archetype came from the passed location and the removal can not fail. + let new_location = unsafe { + remover + .remove( + self.entity, + self.location, + MaybeLocation::caller(), + |_, _, _| (true, ()), + ) + .debug_checked_unwrap() + } + .0; + self.location = new_location; self.world.flush(); self.update_location(); self @@ -2232,8 +2096,28 @@ impl<'w> EntityWorldMut<'w> { component_ids.as_slice(), ); - // SAFETY: the `BundleInfo` for this `component_id` is initialized above - self.location = unsafe { self.remove_bundle(bundle_id, caller) }; + let tick = self.world.change_tick(); + // SAFETY: We just created the bundle, and the archetype is valid, since we are in it. + let Some(mut remover) = (unsafe { + BundleRemover::new_with_id( + self.world, + self.location.archetype_id, + bundle_id, + false, + tick, + ) + }) else { + return self; + }; + // SAFETY: The remover archetype came from the passed location and the removal can not fail. + let new_location = unsafe { + remover + .remove(self.entity, self.location, caller, |_, _, _| (true, ())) + .debug_checked_unwrap() + } + .0; + + self.location = new_location; self.world.flush(); self.update_location(); self From 8e306cf627497ddf44614520717474c1f644fcec Mon Sep 17 00:00:00 2001 From: Elliott Pierce Date: Mon, 24 Mar 2025 19:16:38 -0400 Subject: [PATCH 03/11] Fixed warnings --- crates/bevy_ecs/src/bundle.rs | 6 +- crates/bevy_ecs/src/world/entity_ref.rs | 94 ++----------------------- 2 files changed, 9 insertions(+), 91 deletions(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index 634f09fe14580..df504dfc89bb1 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -1371,6 +1371,10 @@ pub(crate) struct BundleRemover<'w> { old_and_new_table: Option<(NonNull
, NonNull
)>, old_archetype: NonNull, new_archetype: NonNull, + #[expect( + unused, + reason = "It makes sense to know the tick the bundle is removed on. This could be useful." + )] change_tick: Tick, } @@ -1511,7 +1515,7 @@ impl<'w> BundleRemover<'w> { let (needs_drop, pre_remove_result) = pre_remove( &mut world.storages, - world.components(), + &world.components, self.bundle_info.as_ref().explicit_components(), ); diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index e57b92f34d589..3d3e1c03d7a71 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -1,8 +1,8 @@ use crate::{ archetype::{Archetype, ArchetypeId}, bundle::{ - Bundle, BundleEffect, BundleFromComponents, BundleInfo, BundleInserter, BundleRemover, - DynamicBundle, InsertMode, + Bundle, BundleEffect, BundleFromComponents, BundleInserter, BundleRemover, DynamicBundle, + InsertMode, }, change_detection::{MaybeLocation, MutUntyped}, component::{ @@ -17,13 +17,11 @@ use crate::{ observer::Observer, query::{Access, DebugCheckedUnwrap, ReadOnlyQueryData}, relationship::RelationshipHookMode, - removal_detection::RemovedComponentEvents, resource::Resource, - storage::Storages, system::IntoObserverSystem, world::{ - error::EntityComponentError, unsafe_world_cell::UnsafeEntityCell, DeferredWorld, Mut, Ref, - World, ON_DESPAWN, ON_REMOVE, ON_REPLACE, + error::EntityComponentError, unsafe_world_cell::UnsafeEntityCell, Mut, Ref, World, + ON_DESPAWN, ON_REMOVE, ON_REPLACE, }, }; use alloc::vec::Vec; @@ -2621,46 +2619,6 @@ impl<'w> EntityWorldMut<'w> { } } -/// # Safety -/// All components in the archetype must exist in world -unsafe fn trigger_on_replace_and_on_remove_hooks_and_observers( - deferred_world: &mut DeferredWorld, - archetype: &Archetype, - entity: Entity, - bundle_info: &BundleInfo, - caller: MaybeLocation, -) { - let bundle_components_in_archetype = || { - bundle_info - .iter_explicit_components() - .filter(|component_id| archetype.contains(*component_id)) - }; - if archetype.has_replace_observer() { - deferred_world.trigger_observers( - ON_REPLACE, - entity, - bundle_components_in_archetype(), - caller, - ); - } - deferred_world.trigger_on_replace( - archetype, - entity, - bundle_components_in_archetype(), - caller, - RelationshipHookMode::Run, - ); - if archetype.has_remove_observer() { - deferred_world.trigger_observers( - ON_REMOVE, - entity, - bundle_components_in_archetype(), - caller, - ); - } - deferred_world.trigger_on_remove(archetype, entity, bundle_components_in_archetype(), caller); -} - /// A view into a single entity and component in a world, which may either be vacant or occupied. /// /// This `enum` can only be constructed from the [`entry`] method on [`EntityWorldMut`]. @@ -4172,50 +4130,6 @@ unsafe fn insert_dynamic_bundle< } } -/// Moves component data out of storage. -/// -/// This function leaves the underlying memory unchanged, but the component behind -/// returned pointer is semantically owned by the caller and will not be dropped in its original location. -/// Caller is responsible to drop component data behind returned pointer. -/// -/// # Safety -/// - `location.table_row` must be in bounds of column of component id `component_id` -/// - `component_id` must be valid -/// - `components` must come from the same world as `self` -/// - The relevant table row **must be removed** by the caller once all components are taken, without dropping the value -/// -/// # Panics -/// Panics if the entity did not have the component. -#[inline] -pub(crate) unsafe fn take_component<'a>( - storages: &'a mut Storages, - components: &Components, - removed_components: &mut RemovedComponentEvents, - component_id: ComponentId, - entity: Entity, - location: EntityLocation, -) -> OwningPtr<'a> { - // SAFETY: caller promises component_id to be valid - let component_info = unsafe { components.get_info_unchecked(component_id) }; - removed_components.send(component_id, entity); - match component_info.storage_type() { - StorageType::Table => { - let table = &mut storages.tables[location.table_id]; - // SAFETY: - // - archetypes only store valid table_rows - // - index is in bounds as promised by caller - // - promote is safe because the caller promises to remove the table row without dropping it immediately afterwards - unsafe { table.take_component(component_id, location.table_row) } - } - StorageType::SparseSet => storages - .sparse_sets - .get_mut(component_id) - .unwrap() - .remove_and_forget(entity) - .unwrap(), - } -} - /// Types that can be used to fetch components from an entity dynamically by /// [`ComponentId`]s. /// From 487cc5d19d94b117ca2c9a22b7b2cf839d8f8126 Mon Sep 17 00:00:00 2001 From: Elliott Pierce Date: Mon, 24 Mar 2025 19:17:57 -0400 Subject: [PATCH 04/11] removed change tick --- crates/bevy_ecs/src/bundle.rs | 10 +---- crates/bevy_ecs/src/world/entity_ref.rs | 54 ++++--------------------- 2 files changed, 8 insertions(+), 56 deletions(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index df504dfc89bb1..826db094115b1 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -1371,11 +1371,6 @@ pub(crate) struct BundleRemover<'w> { old_and_new_table: Option<(NonNull
, NonNull
)>, old_archetype: NonNull, new_archetype: NonNull, - #[expect( - unused, - reason = "It makes sense to know the tick the bundle is removed on. This could be useful." - )] - change_tick: Tick, } impl<'w> BundleRemover<'w> { @@ -1387,7 +1382,6 @@ impl<'w> BundleRemover<'w> { world: &'w mut World, archetype_id: ArchetypeId, require_all: bool, - change_tick: Tick, ) -> Option { // SAFETY: These come from the same world. `world.components_registrator` can't be used since we borrow other fields too. let mut registrator = @@ -1396,7 +1390,7 @@ impl<'w> BundleRemover<'w> { .bundles .register_info::(&mut registrator, &mut world.storages); // SAFETY: we initialized this bundle_id in `init_info` - unsafe { Self::new_with_id(world, archetype_id, bundle_id, require_all, change_tick) } + unsafe { Self::new_with_id(world, archetype_id, bundle_id, require_all) } } /// Creates a new [`BundleRemover`], if such a remover would do anything. @@ -1411,7 +1405,6 @@ impl<'w> BundleRemover<'w> { archetype_id: ArchetypeId, bundle_id: BundleId, require_all: bool, - change_tick: Tick, ) -> Option { let bundle_info = world.bundles.get_unchecked(bundle_id); // SAFETY: Ensured by caller and that intersections are never `None`. @@ -1446,7 +1439,6 @@ impl<'w> BundleRemover<'w> { new_archetype: new_archetype.into(), old_archetype: old_archetype.into(), old_and_new_table: tables, - change_tick, world: world.as_unsafe_world_cell(), }) } diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index 3d3e1c03d7a71..4df8df2ab6d23 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -1766,10 +1766,8 @@ impl<'w> EntityWorldMut<'w> { self.assert_not_despawned(); let entity = self.entity; let location = self.location; - let tick = self.world.change_tick(); - let mut remover = - BundleRemover::new::(self.world, self.location.archetype_id, true, tick)?; + let mut remover = BundleRemover::new::(self.world, self.location.archetype_id, true)?; // SAFETY: The passed location has the sane archetype as the remover, since they came from the same location. let (new_location, result) = unsafe { remover.remove( @@ -1825,10 +1823,9 @@ impl<'w> EntityWorldMut<'w> { #[inline] pub(crate) fn remove_with_caller(&mut self, caller: MaybeLocation) -> &mut Self { self.assert_not_despawned(); - let tick = self.world.change_tick(); let Some(mut remover) = - BundleRemover::new::(self.world, self.location.archetype_id, false, tick) + BundleRemover::new::(self.world, self.location.archetype_id, false) else { return self; }; @@ -1869,17 +1866,9 @@ impl<'w> EntityWorldMut<'w> { }; let bundle_id = bundles.register_contributed_bundle_info::(&mut registrator, storages); - let tick = self.world.change_tick(); - // SAFETY: We just created the bundle, and the archetype is valid, since we are in it. let Some(mut remover) = (unsafe { - BundleRemover::new_with_id( - self.world, - self.location.archetype_id, - bundle_id, - false, - tick, - ) + BundleRemover::new_with_id(self.world, self.location.archetype_id, bundle_id, false) }) else { return self; }; @@ -1938,17 +1927,9 @@ impl<'w> EntityWorldMut<'w> { .bundles .init_dynamic_info(&mut self.world.storages, ®istrator, to_remove); - let tick = self.world.change_tick(); - // SAFETY: We just created the bundle, and the archetype is valid, since we are in it. let Some(mut remover) = (unsafe { - BundleRemover::new_with_id( - self.world, - self.location.archetype_id, - remove_bundle, - false, - tick, - ) + BundleRemover::new_with_id(self.world, self.location.archetype_id, remove_bundle, false) }) else { return self; }; @@ -1994,16 +1975,9 @@ impl<'w> EntityWorldMut<'w> { component_id, ); - let tick = self.world.change_tick(); // SAFETY: We just created the bundle, and the archetype is valid, since we are in it. let Some(mut remover) = (unsafe { - BundleRemover::new_with_id( - self.world, - self.location.archetype_id, - bundle_id, - false, - tick, - ) + BundleRemover::new_with_id(self.world, self.location.archetype_id, bundle_id, false) }) else { return self; }; @@ -2040,16 +2014,9 @@ impl<'w> EntityWorldMut<'w> { component_ids, ); - let tick = self.world.change_tick(); // SAFETY: We just created the bundle, and the archetype is valid, since we are in it. let Some(mut remover) = (unsafe { - BundleRemover::new_with_id( - self.world, - self.location.archetype_id, - bundle_id, - false, - tick, - ) + BundleRemover::new_with_id(self.world, self.location.archetype_id, bundle_id, false) }) else { return self; }; @@ -2094,16 +2061,9 @@ impl<'w> EntityWorldMut<'w> { component_ids.as_slice(), ); - let tick = self.world.change_tick(); // SAFETY: We just created the bundle, and the archetype is valid, since we are in it. let Some(mut remover) = (unsafe { - BundleRemover::new_with_id( - self.world, - self.location.archetype_id, - bundle_id, - false, - tick, - ) + BundleRemover::new_with_id(self.world, self.location.archetype_id, bundle_id, false) }) else { return self; }; From fcb20336740c50be31a018350ab00e6b7c764466 Mon Sep 17 00:00:00 2001 From: Elliott Pierce Date: Mon, 24 Mar 2025 19:22:57 -0400 Subject: [PATCH 05/11] remove the todos --- crates/bevy_ecs/src/world/entity_ref.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index 4df8df2ab6d23..540c797ad10a9 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -1759,7 +1759,6 @@ impl<'w> EntityWorldMut<'w> { /// # Panics /// /// If the entity has been despawned while this `EntityWorldMut` is still alive. - // TODO: BundleRemover? #[must_use] #[track_caller] pub fn take(&mut self) -> Option { @@ -1814,7 +1813,6 @@ impl<'w> EntityWorldMut<'w> { /// # Panics /// /// If the entity has been despawned while this `EntityWorldMut` is still alive. - // TODO: BundleRemover? #[track_caller] pub fn remove(&mut self) -> &mut Self { self.remove_with_caller::(MaybeLocation::caller()) From 0d4f1c7ffab3d0c9258f5b30cd1b566ff714251b Mon Sep 17 00:00:00 2001 From: Elliott Pierce Date: Mon, 24 Mar 2025 21:24:37 -0400 Subject: [PATCH 06/11] fix doc --- crates/bevy_ecs/src/world/entity_ref.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index 540c797ad10a9..9d3a3c8e22e7f 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -4040,7 +4040,7 @@ where /// # Safety /// /// - [`OwningPtr`] and [`StorageType`] iterators must correspond to the -/// [`BundleInfo`] used to construct [`BundleInserter`] +/// [`BundleInfo`](crate::bundle::BundleInfo) used to construct [`BundleInserter`] /// - [`Entity`] must correspond to [`EntityLocation`] unsafe fn insert_dynamic_bundle< 'a, From ad78601595e656d7d6001acff492c3f9e41d1e0d Mon Sep 17 00:00:00 2001 From: Elliott Pierce Date: Mon, 24 Mar 2025 22:02:47 -0400 Subject: [PATCH 07/11] don't re-index the table --- crates/bevy_ecs/src/bundle.rs | 23 ++++++++++- crates/bevy_ecs/src/world/entity_ref.rs | 53 +++++++++++++++++++------ 2 files changed, 61 insertions(+), 15 deletions(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index 826db094115b1..0de02ed05c401 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -1443,6 +1443,16 @@ impl<'w> BundleRemover<'w> { }) } + /// 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. @@ -1458,7 +1468,12 @@ impl<'w> BundleRemover<'w> { entity: Entity, location: EntityLocation, caller: MaybeLocation, - pre_remove: impl FnOnce(&mut Storages, &Components, &[ComponentId]) -> (bool, T), + pre_remove: impl FnOnce( + &mut SparseSets, + Option<&mut Table>, + &Components, + &[ComponentId], + ) -> (bool, T), ) -> Option<(EntityLocation, T)> { // Hooks // SAFETY: all bundle components exist in World @@ -1506,7 +1521,11 @@ impl<'w> BundleRemover<'w> { let world = unsafe { self.world.world_mut() }; let (needs_drop, pre_remove_result) = pre_remove( - &mut world.storages, + &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(), ); diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index 9d3a3c8e22e7f..a4a30d9d70770 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -1773,22 +1773,24 @@ impl<'w> EntityWorldMut<'w> { entity, location, MaybeLocation::caller(), - |storages, components, bundle_components| { + |sets, table, components, bundle_components| { let mut bundle_components = bundle_components.iter().copied(); ( false, - T::from_components(storages, &mut |storages| { + T::from_components(&mut (sets, table), &mut |(sets, table)| { let component_id = bundle_components.next().unwrap(); // SAFETY: the component existed to be removed, so its id must be valid. let component_info = components.get_info_unchecked(component_id); match component_info.storage_type() { StorageType::Table => { - let table = &mut storages.tables[location.table_id]; - // SAFETY: The remover is cleaning this up. - table.take_component(component_id, location.table_row) + table + .as_mut() + // SAFETY: The table must be valid if the component is in it. + .debug_checked_unwrap() + // SAFETY: The remover is cleaning this up. + .take_component(component_id, location.table_row) } - StorageType::SparseSet => storages - .sparse_sets + StorageType::SparseSet => sets .get_mut(component_id) .unwrap() .remove_and_forget(entity) @@ -1830,7 +1832,12 @@ impl<'w> EntityWorldMut<'w> { // SAFETY: The remover archetype came from the passed location and the removal can not fail. let new_location = unsafe { remover - .remove(self.entity, self.location, caller, |_, _, _| (true, ())) + .remove( + self.entity, + self.location, + caller, + BundleRemover::empty_pre_remove, + ) .debug_checked_unwrap() } .0; @@ -1873,7 +1880,12 @@ impl<'w> EntityWorldMut<'w> { // SAFETY: The remover archetype came from the passed location and the removal can not fail. let new_location = unsafe { remover - .remove(self.entity, self.location, caller, |_, _, _| (true, ())) + .remove( + self.entity, + self.location, + caller, + BundleRemover::empty_pre_remove, + ) .debug_checked_unwrap() } .0; @@ -1934,7 +1946,12 @@ impl<'w> EntityWorldMut<'w> { // SAFETY: The remover archetype came from the passed location and the removal can not fail. let new_location = unsafe { remover - .remove(self.entity, self.location, caller, |_, _, _| (true, ())) + .remove( + self.entity, + self.location, + caller, + BundleRemover::empty_pre_remove, + ) .debug_checked_unwrap() } .0; @@ -1982,7 +1999,12 @@ impl<'w> EntityWorldMut<'w> { // SAFETY: The remover archetype came from the passed location and the removal can not fail. let new_location = unsafe { remover - .remove(self.entity, self.location, caller, |_, _, _| (true, ())) + .remove( + self.entity, + self.location, + caller, + BundleRemover::empty_pre_remove, + ) .debug_checked_unwrap() } .0; @@ -2025,7 +2047,7 @@ impl<'w> EntityWorldMut<'w> { self.entity, self.location, MaybeLocation::caller(), - |_, _, _| (true, ()), + BundleRemover::empty_pre_remove, ) .debug_checked_unwrap() } @@ -2068,7 +2090,12 @@ impl<'w> EntityWorldMut<'w> { // SAFETY: The remover archetype came from the passed location and the removal can not fail. let new_location = unsafe { remover - .remove(self.entity, self.location, caller, |_, _, _| (true, ())) + .remove( + self.entity, + self.location, + caller, + BundleRemover::empty_pre_remove, + ) .debug_checked_unwrap() } .0; From c0f6024e726700643c9fe04bae67e639d9d51895 Mon Sep 17 00:00:00 2001 From: Elliott Pierce Date: Sun, 6 Apr 2025 17:11:29 -0400 Subject: [PATCH 08/11] fixed bad main merge --- crates/bevy_ecs/src/world/entity_ref.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index 3814dfe2ee3dc..932d62b4420aa 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -10,8 +10,7 @@ use crate::{ StorageType, }, entity::{ - Entity, EntityBorrow, EntityCloner, EntityClonerBuilder, EntityLocation, - TrustedEntityBorrow, EntityEquivalent + ContainsEntity, Entity, EntityCloner, EntityClonerBuilder, EntityEquivalent, EntityLocation, }, event::Event, observer::Observer, From 0fe2b5c84eb0ff0b83dd3e171bee9d603fd460e0 Mon Sep 17 00:00:00 2001 From: Elliott Pierce Date: Fri, 2 May 2025 13:21:26 -0400 Subject: [PATCH 09/11] make remove infalible --- crates/bevy_ecs/src/bundle.rs | 9 +-- crates/bevy_ecs/src/world/entity_ref.rs | 86 +++++++++++-------------- 2 files changed, 40 insertions(+), 55 deletions(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index 0de02ed05c401..baae52af9e541 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -1457,9 +1457,6 @@ impl<'w> BundleRemover<'w> { /// /// `pre_remove` should return a bool for if the components still need to be dropped. /// - /// If `intersection` is true, the removal will go through with all overlapping components. - /// Otherwise, returns `None` if not all components are available for removal. - /// /// # Safety /// The `location` must have the same archetype as the remover. #[inline] @@ -1474,7 +1471,7 @@ impl<'w> BundleRemover<'w> { &Components, &[ComponentId], ) -> (bool, T), - ) -> Option<(EntityLocation, T)> { + ) -> (EntityLocation, T) { // Hooks // SAFETY: all bundle components exist in World unsafe { @@ -1530,7 +1527,7 @@ impl<'w> BundleRemover<'w> { self.bundle_info.as_ref().explicit_components(), ); - // Handle basic removes + // 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); @@ -1628,7 +1625,7 @@ impl<'w> BundleRemover<'w> { world.entities.set(entity.index(), new_location); } - Some((new_location, pre_remove_result)) + (new_location, pre_remove_result) } } diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index 932d62b4420aa..a6ed4015440e4 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -2015,7 +2015,7 @@ impl<'w> EntityWorldMut<'w> { }), ) }, - )? + ) }; self.location = new_location; @@ -2047,14 +2047,12 @@ impl<'w> EntityWorldMut<'w> { }; // SAFETY: The remover archetype came from the passed location and the removal can not fail. let new_location = unsafe { - remover - .remove( - self.entity, - self.location, - caller, - BundleRemover::empty_pre_remove, - ) - .debug_checked_unwrap() + remover.remove( + self.entity, + self.location, + caller, + BundleRemover::empty_pre_remove, + ) } .0; @@ -2095,14 +2093,12 @@ impl<'w> EntityWorldMut<'w> { }; // SAFETY: The remover archetype came from the passed location and the removal can not fail. let new_location = unsafe { - remover - .remove( - self.entity, - self.location, - caller, - BundleRemover::empty_pre_remove, - ) - .debug_checked_unwrap() + remover.remove( + self.entity, + self.location, + caller, + BundleRemover::empty_pre_remove, + ) } .0; @@ -2161,14 +2157,12 @@ impl<'w> EntityWorldMut<'w> { }; // SAFETY: The remover archetype came from the passed location and the removal can not fail. let new_location = unsafe { - remover - .remove( - self.entity, - self.location, - caller, - BundleRemover::empty_pre_remove, - ) - .debug_checked_unwrap() + remover.remove( + self.entity, + self.location, + caller, + BundleRemover::empty_pre_remove, + ) } .0; @@ -2214,14 +2208,12 @@ impl<'w> EntityWorldMut<'w> { }; // SAFETY: The remover archetype came from the passed location and the removal can not fail. let new_location = unsafe { - remover - .remove( - self.entity, - self.location, - caller, - BundleRemover::empty_pre_remove, - ) - .debug_checked_unwrap() + remover.remove( + self.entity, + self.location, + caller, + BundleRemover::empty_pre_remove, + ) } .0; @@ -2258,14 +2250,12 @@ impl<'w> EntityWorldMut<'w> { }; // SAFETY: The remover archetype came from the passed location and the removal can not fail. let new_location = unsafe { - remover - .remove( - self.entity, - self.location, - MaybeLocation::caller(), - BundleRemover::empty_pre_remove, - ) - .debug_checked_unwrap() + remover.remove( + self.entity, + self.location, + MaybeLocation::caller(), + BundleRemover::empty_pre_remove, + ) } .0; @@ -2305,14 +2295,12 @@ impl<'w> EntityWorldMut<'w> { }; // SAFETY: The remover archetype came from the passed location and the removal can not fail. let new_location = unsafe { - remover - .remove( - self.entity, - self.location, - caller, - BundleRemover::empty_pre_remove, - ) - .debug_checked_unwrap() + remover.remove( + self.entity, + self.location, + caller, + BundleRemover::empty_pre_remove, + ) } .0; From b207eaf63494b2f78c5594a399c39d34d187b085 Mon Sep 17 00:00:00 2001 From: Elliott Pierce Date: Fri, 2 May 2025 13:24:34 -0400 Subject: [PATCH 10/11] make new unsafe --- crates/bevy_ecs/src/bundle.rs | 7 +++++-- crates/bevy_ecs/src/world/entity_ref.rs | 7 +++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index baae52af9e541..b0d12f10e42f7 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -1377,8 +1377,11 @@ 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) fn new( + pub(crate) unsafe fn new( world: &'w mut World, archetype_id: ArchetypeId, require_all: bool, @@ -1389,7 +1392,7 @@ impl<'w> BundleRemover<'w> { let bundle_id = world .bundles .register_info::(&mut registrator, &mut world.storages); - // SAFETY: we initialized this bundle_id in `init_info` + // 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) } } diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index a6ed4015440e4..343b455e79677 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -1982,7 +1982,9 @@ impl<'w> EntityWorldMut<'w> { let entity = self.entity; let location = self.location; - let mut remover = BundleRemover::new::(self.world, self.location.archetype_id, true)?; + let mut remover = + // SAFETY: The archetype id must be valid since this entity is in it. + unsafe { BundleRemover::new::(self.world, self.location.archetype_id, true) }?; // SAFETY: The passed location has the sane archetype as the remover, since they came from the same location. let (new_location, result) = unsafe { remover.remove( @@ -2041,7 +2043,8 @@ impl<'w> EntityWorldMut<'w> { self.assert_not_despawned(); let Some(mut remover) = - BundleRemover::new::(self.world, self.location.archetype_id, false) + // SAFETY: The archetype id must be valid since this entity is in it. + (unsafe { BundleRemover::new::(self.world, self.location.archetype_id, false) }) else { return self; }; From 0893fdf31e76779ec786deffba00f1df61c9150b Mon Sep 17 00:00:00 2001 From: Elliott Pierce Date: Fri, 2 May 2025 13:27:09 -0400 Subject: [PATCH 11/11] fix safety comment --- crates/bevy_ecs/src/bundle.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index b0d12f10e42f7..4c9d0d46ac89c 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -1410,7 +1410,7 @@ impl<'w> BundleRemover<'w> { require_all: bool, ) -> Option { let bundle_info = world.bundles.get_unchecked(bundle_id); - // SAFETY: Ensured by caller and that intersections are never `None`. + // SAFETY: Caller ensures archetype and bundle ids are correct. let new_archetype_id = unsafe { bundle_info.remove_bundle_from_archetype( &mut world.archetypes,