From 67146bdef73f02c6447901f6ba0ee1c7a9f5b485 Mon Sep 17 00:00:00 2001 From: JaySpruce Date: Thu, 27 Feb 2025 00:20:32 -0600 Subject: [PATCH] Add missing `unsafe` to `entity_command::insert_by_id` and make it more configurable (#18052) ## Objective `insert_by_id` is unsafe, but I forgot to add that to the manually-queueable version in `entity_command`. It also can only insert using `InsertMode::Replace`, when it could easily be configurable by threading an `InsertMode` parameter to the final `BundleInserter::insert` call. ## Solution - Add `unsafe` and safety comment. - Add `InsertMode` parameter to `entity_command::insert_by_id`, `EntityWorldMut::insert_by_id_with_caller`, and `EntityWorldMut::insert_dynamic_bundle`. - Add `InsertMode` parameter to `entity_command::insert` and remove `entity_command::insert_if_new`, for consistency with the other manually-queued insertion commands. --- .../src/system/commands/entity_command.rs | 30 +++++++++---------- crates/bevy_ecs/src/system/commands/mod.rs | 23 ++++++++++---- crates/bevy_ecs/src/world/entity_ref.rs | 17 +++++++++-- 3 files changed, 45 insertions(+), 25 deletions(-) diff --git a/crates/bevy_ecs/src/system/commands/entity_command.rs b/crates/bevy_ecs/src/system/commands/entity_command.rs index f24dbf8bba72b..ab0c47bf4e0e5 100644 --- a/crates/bevy_ecs/src/system/commands/entity_command.rs +++ b/crates/bevy_ecs/src/system/commands/entity_command.rs @@ -151,36 +151,34 @@ where } } -/// An [`EntityCommand`] that adds the components in a [`Bundle`] to an entity, -/// replacing any that were already present. +/// An [`EntityCommand`] that adds the components in a [`Bundle`] to an entity. #[track_caller] -pub fn insert(bundle: impl Bundle) -> impl EntityCommand { +pub fn insert(bundle: impl Bundle, mode: InsertMode) -> impl EntityCommand { let caller = MaybeLocation::caller(); move |mut entity: EntityWorldMut| { - entity.insert_with_caller(bundle, InsertMode::Replace, caller); - } -} - -/// An [`EntityCommand`] that adds the components in a [`Bundle`] to an entity, -/// except for any that were already present. -#[track_caller] -pub fn insert_if_new(bundle: impl Bundle) -> impl EntityCommand { - let caller = MaybeLocation::caller(); - move |mut entity: EntityWorldMut| { - entity.insert_with_caller(bundle, InsertMode::Keep, caller); + entity.insert_with_caller(bundle, mode, caller); } } /// An [`EntityCommand`] that adds a dynamic component to an entity. +/// +/// # Safety +/// +/// - [`ComponentId`] must be from the same world as the target entity. +/// - `T` must have the same layout as the one passed during `component_id` creation. #[track_caller] -pub fn insert_by_id(component_id: ComponentId, value: T) -> impl EntityCommand { +pub unsafe fn insert_by_id( + component_id: ComponentId, + value: T, + mode: InsertMode, +) -> impl EntityCommand { let caller = MaybeLocation::caller(); move |mut entity: EntityWorldMut| { // SAFETY: // - `component_id` safety is ensured by the caller // - `ptr` is valid within the `make` block OwningPtr::make(value, |ptr| unsafe { - entity.insert_by_id_with_caller(component_id, ptr, caller); + entity.insert_by_id_with_caller(component_id, ptr, mode, caller); }); } } diff --git a/crates/bevy_ecs/src/system/commands/mod.rs b/crates/bevy_ecs/src/system/commands/mod.rs index bee44ec8d63f6..8db032a996f10 100644 --- a/crates/bevy_ecs/src/system/commands/mod.rs +++ b/crates/bevy_ecs/src/system/commands/mod.rs @@ -1290,7 +1290,7 @@ impl<'a> EntityCommands<'a> { /// ``` #[track_caller] pub fn insert(&mut self, bundle: impl Bundle) -> &mut Self { - self.queue(entity_command::insert(bundle)) + self.queue(entity_command::insert(bundle, InsertMode::Replace)) } /// Similar to [`Self::insert`] but will only insert if the predicate returns true. @@ -1350,7 +1350,7 @@ impl<'a> EntityCommands<'a> { /// To avoid a panic in this case, use the command [`Self::try_insert_if_new`] instead. #[track_caller] pub fn insert_if_new(&mut self, bundle: impl Bundle) -> &mut Self { - self.queue(entity_command::insert_if_new(bundle)) + self.queue(entity_command::insert(bundle, InsertMode::Keep)) } /// Adds a [`Bundle`] of components to the entity without overwriting if the @@ -1399,7 +1399,12 @@ impl<'a> EntityCommands<'a> { component_id: ComponentId, value: T, ) -> &mut Self { - self.queue(entity_command::insert_by_id(component_id, value)) + self.queue( + // SAFETY: + // - `ComponentId` safety is ensured by the caller. + // - `T` safety is ensured by the caller. + unsafe { entity_command::insert_by_id(component_id, value, InsertMode::Replace) }, + ) } /// Attempts to add a dynamic component to an entity. @@ -1417,7 +1422,10 @@ impl<'a> EntityCommands<'a> { value: T, ) -> &mut Self { self.queue_handled( - entity_command::insert_by_id(component_id, value), + // SAFETY: + // - `ComponentId` safety is ensured by the caller. + // - `T` safety is ensured by the caller. + unsafe { entity_command::insert_by_id(component_id, value, InsertMode::Replace) }, error_handler::silent(), ) } @@ -1472,7 +1480,10 @@ impl<'a> EntityCommands<'a> { /// ``` #[track_caller] pub fn try_insert(&mut self, bundle: impl Bundle) -> &mut Self { - self.queue_handled(entity_command::insert(bundle), error_handler::silent()) + self.queue_handled( + entity_command::insert(bundle, InsertMode::Replace), + error_handler::silent(), + ) } /// Similar to [`Self::try_insert`] but will only try to insert if the predicate returns true. @@ -1572,7 +1583,7 @@ impl<'a> EntityCommands<'a> { #[track_caller] pub fn try_insert_if_new(&mut self, bundle: impl Bundle) -> &mut Self { self.queue_handled( - entity_command::insert_if_new(bundle), + entity_command::insert(bundle, InsertMode::Keep), error_handler::silent(), ) } diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index 297131e512bca..b4fe20b3a4d3d 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -1585,16 +1585,24 @@ impl<'w> EntityWorldMut<'w> { component_id: ComponentId, component: OwningPtr<'_>, ) -> &mut Self { - self.insert_by_id_with_caller(component_id, component, MaybeLocation::caller()) + self.insert_by_id_with_caller( + component_id, + component, + InsertMode::Replace, + MaybeLocation::caller(), + ) } /// # Safety - /// See [`EntityWorldMut::insert_by_id`] + /// + /// - [`ComponentId`] must be from the same world as [`EntityWorldMut`] + /// - [`OwningPtr`] must be a valid reference to the type represented by [`ComponentId`] #[inline] pub(crate) unsafe fn insert_by_id_with_caller( &mut self, component_id: ComponentId, component: OwningPtr<'_>, + mode: InsertMode, caller: MaybeLocation, ) -> &mut Self { self.assert_not_despawned(); @@ -1619,6 +1627,7 @@ impl<'w> EntityWorldMut<'w> { self.location, Some(component).into_iter(), Some(storage_type).iter().cloned(), + mode, caller, ); self.world.flush(); @@ -1670,6 +1679,7 @@ impl<'w> EntityWorldMut<'w> { self.location, iter_components, (*storage_types).iter().cloned(), + InsertMode::Replace, MaybeLocation::caller(), ); *self.world.bundles.get_storages_unchecked(bundle_id) = core::mem::take(&mut storage_types); @@ -4153,6 +4163,7 @@ unsafe fn insert_dynamic_bundle< location: EntityLocation, components: I, storage_types: S, + mode: InsertMode, caller: MaybeLocation, ) -> EntityLocation { struct DynamicInsertBundle<'a, I: Iterator)>> { @@ -4175,7 +4186,7 @@ unsafe fn insert_dynamic_bundle< // SAFETY: location matches current entity. unsafe { bundle_inserter - .insert(entity, location, bundle, InsertMode::Replace, caller) + .insert(entity, location, bundle, mode, caller) .0 } }