diff --git a/_release-content/migration-guides/insert_if_new_required_components.md b/_release-content/migration-guides/insert_if_new_required_components.md new file mode 100644 index 0000000000000..d30fda2346cac --- /dev/null +++ b/_release-content/migration-guides/insert_if_new_required_components.md @@ -0,0 +1,24 @@ +--- +title: "`insert_if_new` no longer re-adds required components of already-present components" +pull_requests: [24593] +--- + +The `InsertMode::Keep` insert APIs (`EntityWorldMut::insert_if_new`, `EntityCommands::insert_if_new`, `World::insert_batch_if_new`, `World::try_insert_batch_if_new`) now only add a required component when the component requiring it is actually being inserted. + +Previously, inserting a component that was already present could still (re-)insert its required components. So if you removed a required component and then re-inserted its requirer with `insert_if_new`, the required component was silently brought back, even though the requirer itself was unchanged. Now inserting an already-present component is a true no-op, including for its required components. + +```rust +#[derive(Component)] +#[require(B)] +struct A; +#[derive(Component, Default)] +struct B; + +let id = world.spawn(A).id(); // inserts A and its required B +world.entity_mut(id).remove::(); +world.entity_mut(id).insert_if_new(A); +// Before: B is present again. +// Now: B stays absent, because A was already present so the insert did nothing. +``` + +If you relied on the old behavior to restore a required component, insert it explicitly (`insert(B)`), or use `insert` (`InsertMode::Replace`) instead of `insert_if_new`. diff --git a/crates/bevy_ecs/src/archetype.rs b/crates/bevy_ecs/src/archetype.rs index e4b75653aa57b..496a5463967b7 100644 --- a/crates/bevy_ecs/src/archetype.rs +++ b/crates/bevy_ecs/src/archetype.rs @@ -20,7 +20,7 @@ //! [`World::archetypes`]: crate::world::World::archetypes use crate::{ - bundle::BundleId, + bundle::{BundleId, InsertMode}, component::{ComponentId, Components, RequiredComponentConstructor, StorageType}, entity::{Entity, EntityLocation}, event::Event, @@ -205,6 +205,11 @@ impl BundleComponentStatus for SpawnBundleStatus { #[derive(Default)] pub struct Edges { insert_bundle: SparseArray, + // Separate edge for `InsertMode::Keep` (`insert_if_new`). It differs from `insert_bundle` only + // in that required components pulled in solely by components already present in the source + // archetype are excluded from the target, so `insert_if_new` on an already-present component + // does not resurrect a required component the caller removed. + insert_bundle_if_new: SparseArray, remove_bundle: SparseArray>, take_bundle: SparseArray>, } @@ -217,18 +222,25 @@ impl Edges { /// the source archetype via the provided bundle. #[inline] pub fn get_archetype_after_bundle_insert(&self, bundle_id: BundleId) -> Option { - self.get_archetype_after_bundle_insert_internal(bundle_id) + self.get_archetype_after_bundle_insert_internal(bundle_id, InsertMode::Replace) .map(|bundle| bundle.archetype_id) } /// Internal version of `get_archetype_after_bundle_insert` that /// fetches the full `ArchetypeAfterBundleInsert`. + /// + /// `InsertMode::Keep` (used by `insert_if_new`) selects an edge that excludes required + /// components pulled in only by already-present components. #[inline] pub(crate) fn get_archetype_after_bundle_insert_internal( &self, bundle_id: BundleId, + insert_mode: InsertMode, ) -> Option<&ArchetypeAfterBundleInsert> { - self.insert_bundle.get(bundle_id) + match insert_mode { + InsertMode::Keep => self.insert_bundle_if_new.get(bundle_id), + InsertMode::Replace => self.insert_bundle.get(bundle_id), + } } /// Caches the target archetype when inserting a bundle into the source archetype. @@ -236,6 +248,7 @@ impl Edges { pub(crate) fn cache_archetype_after_bundle_insert( &mut self, bundle_id: BundleId, + insert_mode: InsertMode, archetype_id: ArchetypeId, bundle_status: impl Into>, required_components: impl Into>, @@ -246,16 +259,17 @@ impl Edges { // Make sure `extend` doesn't over-reserve, since the conversion to `Box<[_]>` would reallocate to shrink. added.reserve_exact(existing.len()); added.extend(existing); - self.insert_bundle.insert( - bundle_id, - ArchetypeAfterBundleInsert { - archetype_id, - bundle_status: bundle_status.into(), - required_components: required_components.into(), - added_len, - inserted: added.into(), - }, - ); + let edge = ArchetypeAfterBundleInsert { + archetype_id, + bundle_status: bundle_status.into(), + required_components: required_components.into(), + added_len, + inserted: added.into(), + }; + match insert_mode { + InsertMode::Keep => self.insert_bundle_if_new.insert(bundle_id, edge), + InsertMode::Replace => self.insert_bundle.insert(bundle_id, edge), + }; } /// Checks the cache for the target archetype when removing a bundle from the diff --git a/crates/bevy_ecs/src/bundle/insert.rs b/crates/bevy_ecs/src/bundle/insert.rs index e745f7ddbeb5f..fbe1ddf96c261 100644 --- a/crates/bevy_ecs/src/bundle/insert.rs +++ b/crates/bevy_ecs/src/bundle/insert.rs @@ -1,4 +1,5 @@ use alloc::vec::Vec; +use bevy_platform::collections::HashMap; use bevy_ptr::{ConstNonNull, MovingPtr}; use core::ptr::NonNull; @@ -9,7 +10,7 @@ use crate::{ }, bundle::{ArchetypeMoveType, Bundle, BundleId, BundleInfo, DynamicBundle, InsertMode}, change_detection::{MaybeLocation, Tick}, - component::{Components, StorageType}, + component::{ComponentId, Components, RequiredComponentConstructor, StorageType}, entity::{Entities, Entity, EntityLocation}, event::EntityComponentsTrigger, lifecycle::{Add, Discard, Insert, ADD, DISCARD, INSERT}, @@ -39,11 +40,12 @@ impl<'w> BundleInserter<'w> { world: &'w mut World, archetype_id: ArchetypeId, change_tick: Tick, + insert_mode: InsertMode, ) -> Self { let bundle_id = world.register_bundle_info::(); // SAFETY: We just ensured this bundle exists - unsafe { Self::new_with_id(world, archetype_id, bundle_id, change_tick) } + unsafe { Self::new_with_id(world, archetype_id, bundle_id, change_tick, insert_mode) } } /// Creates a new [`BundleInserter`]. @@ -57,6 +59,7 @@ impl<'w> BundleInserter<'w> { archetype_id: ArchetypeId, bundle_id: BundleId, change_tick: Tick, + insert_mode: InsertMode, ) -> Self { // SAFETY: bundle exists per precondition let bundle_info = unsafe { world.bundles.get_unchecked(bundle_id) }; @@ -68,6 +71,7 @@ impl<'w> BundleInserter<'w> { &world.components, &world.observers, archetype_id, + insert_mode, ) }; @@ -84,7 +88,7 @@ impl<'w> BundleInserter<'w> { let archetype_after_insert = unsafe { archetype .edges() - .get_archetype_after_bundle_insert_internal(bundle_id) + .get_archetype_after_bundle_insert_internal(bundle_id, insert_mode) .debug_checked_unwrap() .into() }; @@ -538,17 +542,27 @@ impl BundleInfo { components: &Components, observers: &Observers, archetype_id: ArchetypeId, + insert_mode: InsertMode, ) -> (ArchetypeId, bool) { - if let Some(archetype_after_insert_id) = archetypes[archetype_id] + // `Keep` (`insert_if_new`) builds a distinct edge that excludes required components pulled + // in only by components already present in the source archetype. + let keep = insert_mode == InsertMode::Keep; + if let Some(edge) = archetypes[archetype_id] .edges() - .get_archetype_after_bundle_insert(self.id) + .get_archetype_after_bundle_insert_internal(self.id, insert_mode) { - return (archetype_after_insert_id, false); + return (edge.archetype_id, false); } let mut new_table_components = Vec::new(); let mut new_sparse_set_components = Vec::new(); let mut bundle_status = Vec::with_capacity(self.explicit_components_len()); let mut added_required_components = Vec::new(); + // Under `Keep`, required components reachable from the explicit components this insert + // actually adds, each mapped to the constructor from an added requiree. Required components + // reachable only from already-present components are absent (so they are skipped), and a + // shared required component takes an added requiree's constructor rather than the bundle's. + let mut required_by_added: HashMap = + HashMap::default(); let mut added = Vec::new(); let mut existing = Vec::new(); @@ -562,6 +576,15 @@ impl BundleInfo { added.push(component_id); // SAFETY: component_id exists let component_info = unsafe { components.get_info_unchecked(component_id) }; + if keep { + // Newly added, so its required components are genuinely new under `Keep`. First + // requiree wins, matching the bundle's own required-component precedence. + for (required_id, required) in &component_info.required_components().all { + required_by_added + .entry(*required_id) + .or_insert_with(|| required.constructor.clone()); + } + } match component_info.storage_type() { StorageType::Table => new_table_components.push(component_id), StorageType::SparseSet => new_sparse_set_components.push(component_id), @@ -571,7 +594,17 @@ impl BundleInfo { for (index, component_id) in self.iter_required_components().enumerate() { if !current_archetype.contains(component_id) { - added_required_components.push(self.required_component_constructors[index].clone()); + let constructor = if keep { + // Skip required components reachable only from already-present components; for the + // rest, use the constructor from a requiree that is actually being added. + match required_by_added.get(&component_id) { + Some(constructor) => constructor.clone(), + None => continue, + } + } else { + self.required_component_constructors[index].clone() + }; + added_required_components.push(constructor); added.push(component_id); // SAFETY: component_id exists let component_info = unsafe { components.get_info_unchecked(component_id) }; @@ -591,6 +624,7 @@ impl BundleInfo { // The archetype does not change when we insert this bundle. edges.cache_archetype_after_bundle_insert( self.id, + insert_mode, archetype_id, bundle_status, added_required_components, @@ -648,6 +682,7 @@ impl BundleInfo { .edges_mut() .cache_archetype_after_bundle_insert( self.id, + insert_mode, new_archetype_id, bundle_status, added_required_components, diff --git a/crates/bevy_ecs/src/bundle/spawner.rs b/crates/bevy_ecs/src/bundle/spawner.rs index 3bce44826cd29..00af3068e89b3 100644 --- a/crates/bevy_ecs/src/bundle/spawner.rs +++ b/crates/bevy_ecs/src/bundle/spawner.rs @@ -53,6 +53,7 @@ impl<'w> BundleSpawner<'w> { &world.components, &world.observers, ArchetypeId::EMPTY, + InsertMode::Replace, ) }; diff --git a/crates/bevy_ecs/src/component/mod.rs b/crates/bevy_ecs/src/component/mod.rs index 7182ec7f9dd13..6e462aca0356e 100644 --- a/crates/bevy_ecs/src/component/mod.rs +++ b/crates/bevy_ecs/src/component/mod.rs @@ -290,6 +290,34 @@ use core::{fmt::Debug, marker::PhantomData, ops::Deref}; /// 1. Specifying a required component constructor for Foo directly on a spawned component Bar will result in that constructor being used (and overriding existing constructors lower in the inheritance tree). This is the classic "inheritance override" behavior people expect. /// 2. For cases where "multiple inheritance" results in constructor clashes, Components should be listed in "importance order". List a component earlier in the requirement list to initialize its inheritance tree earlier. /// +/// ## Required components and `insert_if_new` +/// +/// [`insert_if_new`](crate::world::EntityWorldMut::insert_if_new) only adds components that are not +/// already present, and the same applies to required components: a required component is added only +/// for a component the call actually inserts. Inserting an already-present component with +/// `insert_if_new` is a no-op and does not re-add its required components, so it will not bring back +/// a required component you previously removed: +/// +/// ``` +/// # use bevy_ecs::prelude::*; +/// #[derive(Component)] +/// #[require(B)] +/// struct A; +/// #[derive(Component, Default)] +/// struct B; +/// +/// # let mut world = World::default(); +/// let id = world.spawn(A).id(); +/// world.entity_mut(id).remove::(); +/// // `A` is already present, so this is a no-op and does not re-add `B`. +/// world.entity_mut(id).insert_if_new(A); +/// assert!(!world.entity(id).contains::()); +/// ``` +/// +/// This is decided per explicitly-inserted component, not recursively: `insert_if_new` of a +/// newly-added component still adds that component's required components transitively, even where an +/// intermediate requirer already exists. +/// /// ## Registering required components at runtime /// /// In most cases, required components should be registered using the `require` attribute as shown above. diff --git a/crates/bevy_ecs/src/component/required.rs b/crates/bevy_ecs/src/component/required.rs index 632fad29fd90b..b3b72613e8ebc 100644 --- a/crates/bevy_ecs/src/component/required.rs +++ b/crates/bevy_ecs/src/component/required.rs @@ -662,6 +662,132 @@ mod tests { world::World, }; + // Regression tests for #24554: `insert_if_new` (`InsertMode::Keep`) must not resurrect a + // required component that the caller previously removed, while still adding required components + // for components it actually inserts. + + #[test] + fn insert_if_new_does_not_resurrect_removed_required_component() { + #[derive(Component)] + #[require(ReqMarker)] + struct Main; + + #[derive(Component, Default)] + struct ReqMarker; + + let mut world = World::new(); + let id = world.spawn(Main).id(); + assert!(world.entity(id).contains::()); + + world.entity_mut(id).remove::(); + assert!(!world.entity(id).contains::()); + + // `Main` already exists, so `insert_if_new` is a no-op and must not re-add `ReqMarker`. + world.entity_mut(id).insert_if_new(Main); + assert!( + !world.entity(id).contains::(), + "insert_if_new on an existing component must not resurrect its removed required component" + ); + } + + #[test] + fn insert_replace_re_adds_removed_required_component() { + #[derive(Component)] + #[require(ReqMarker)] + struct Main; + + #[derive(Component, Default)] + struct ReqMarker; + + let mut world = World::new(); + let id = world.spawn(Main).id(); + world.entity_mut(id).remove::(); + assert!(!world.entity(id).contains::()); + + // Plain `insert` (`Replace`) should still (re-)add required components. + world.entity_mut(id).insert(Main); + assert!( + world.entity(id).contains::(), + "insert (Replace) should re-add required components" + ); + } + + #[test] + fn insert_if_new_adds_required_for_newly_added_component() { + #[derive(Component)] + #[require(ReqMarker)] + struct Main; + + #[derive(Component, Default)] + struct ReqMarker; + + let mut world = World::new(); + let id = world.spawn_empty().id(); + + // `Main` is genuinely new here, so its required components must be pulled in even under Keep. + world.entity_mut(id).insert_if_new(Main); + assert!(world.entity(id).contains::
()); + assert!( + world.entity(id).contains::(), + "a newly inserted component should add its required components, even under Keep" + ); + } + + #[test] + fn insert_if_new_mixed_bundle_only_adds_required_for_new_component() { + #[derive(Component)] + #[require(ReqA)] + struct A; + #[derive(Component, Default)] + struct ReqA; + + #[derive(Component)] + #[require(ReqB)] + struct B; + #[derive(Component, Default)] + struct ReqB; + + let mut world = World::new(); + let id = world.spawn(A).id(); + world.entity_mut(id).remove::(); + assert!(!world.entity(id).contains::()); + + // `A` is already present (kept); `B` is new. Only `B`'s required component should be added. + world.entity_mut(id).insert_if_new((A, B)); + assert!(world.entity(id).contains::()); + assert!( + world.entity(id).contains::(), + "the newly inserted component B should add its required component" + ); + assert!( + !world.entity(id).contains::(), + "the already-present component A must not resurrect its required component" + ); + } + + #[test] + fn insert_if_new_shared_required_uses_added_requiree_constructor() { + #[derive(Component, PartialEq, Debug)] + struct Req(usize); + + #[derive(Component)] + #[require(Req = Req(1))] + struct A; + + #[derive(Component)] + #[require(Req = Req(2))] + struct B; + + let mut world = World::new(); + let id = world.spawn(A).id(); + world.entity_mut(id).remove::(); + + // `A` is already present, so this is effectively `insert(B)`: the shared `Req` should take + // `B`'s constructor, not the bundle's first-requiree `A`'s. + world.entity_mut(id).insert_if_new((A, B)); + assert_eq!(world.entity(id).get::(), Some(&Req(2))); + } + #[test] fn required_components() { #[derive(Component)] diff --git a/crates/bevy_ecs/src/world/entity_access/world_mut.rs b/crates/bevy_ecs/src/world/entity_access/world_mut.rs index 6ae9d32e255eb..22e5641d552c0 100644 --- a/crates/bevy_ecs/src/world/entity_access/world_mut.rs +++ b/crates/bevy_ecs/src/world/entity_access/world_mut.rs @@ -1074,8 +1074,9 @@ impl<'w> EntityWorldMut<'w> { let change_tick = self.world.change_tick(); // SAFETY: // - `location.archetype_id` is part of a valid `EntityLocation`. - let mut bundle_inserter = - unsafe { BundleInserter::new::(self.world, location.archetype_id, change_tick) }; + let mut bundle_inserter = unsafe { + BundleInserter::new::(self.world, location.archetype_id, change_tick, mode) + }; // SAFETY: // - `location` matches current entity and thus must currently exist in the source // archetype for this inserter and its location within the archetype. @@ -1162,7 +1163,13 @@ impl<'w> EntityWorldMut<'w> { // - bundle initialized above // - archetype id taken from existing entity let bundle_inserter = unsafe { - BundleInserter::new_with_id(self.world, location.archetype_id, bundle_id, change_tick) + BundleInserter::new_with_id( + self.world, + location.archetype_id, + bundle_id, + change_tick, + mode, + ) }; // SAFETY: @@ -1239,7 +1246,13 @@ impl<'w> EntityWorldMut<'w> { // - bundle initialized above // - archetype id taken from existing entity let bundle_inserter = unsafe { - BundleInserter::new_with_id(self.world, location.archetype_id, bundle_id, change_tick) + BundleInserter::new_with_id( + self.world, + location.archetype_id, + bundle_id, + change_tick, + InsertMode::Replace, + ) }; // SAFETY: diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 08f18eb42b22f..268d432d4088c 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -2582,6 +2582,7 @@ impl World { first_location.archetype_id, bundle_id, change_tick, + insert_mode, ) }, archetype_id: first_location.archetype_id, @@ -2611,6 +2612,7 @@ impl World { location.archetype_id, bundle_id, change_tick, + insert_mode, ) }, archetype_id: location.archetype_id, @@ -2730,6 +2732,7 @@ impl World { first_location.archetype_id, bundle_id, change_tick, + insert_mode, ) }, archetype_id: first_location.archetype_id, @@ -2771,6 +2774,7 @@ impl World { location.archetype_id, bundle_id, change_tick, + insert_mode, ) }, archetype_id: location.archetype_id, @@ -2945,8 +2949,14 @@ impl World { let tick = world.change_tick(); // SAFETY: // - `location.archetype_id` is part of a valid `EntityLocation`. - let mut bundle_inserter = - unsafe { BundleInserter::new::(world, location.archetype_id, tick) }; + let mut bundle_inserter = unsafe { + BundleInserter::new::( + world, + location.archetype_id, + tick, + InsertMode::Replace, + ) + }; // SAFETY: // - `location` matches current entity and thus must currently exist in the source // archetype for this inserter and its location within the archetype.