From a98945394fd51beb1282fed67a0b5699462b864b Mon Sep 17 00:00:00 2001 From: satyakwok <119509589+satyakwok@users.noreply.github.com> Date: Thu, 11 Jun 2026 21:25:44 +0200 Subject: [PATCH 1/2] fix(ecs): don't resurrect removed required components on insert_if_new `insert_if_new` (`InsertMode::Keep`) re-added a required component even when the component that required it was already present and the caller had removed the required component. The required-components transition was computed mode-agnostically and cached on a single archetype edge shared by `Replace` and `Keep`, so gating only value initialization left the component in the target archetype (and uninitialized for non-ZSTs). Add a separate `Keep` archetype edge that excludes required components pulled in solely by already-present components, while still adding required components for components the insert actually adds (mixed bundles stay correct). `Replace` is unchanged. Closes #24554 --- crates/bevy_ecs/src/archetype.rs | 40 +++++-- crates/bevy_ecs/src/bundle/insert.rs | 36 +++++- crates/bevy_ecs/src/bundle/spawner.rs | 2 + crates/bevy_ecs/src/component/required.rs | 103 ++++++++++++++++++ .../src/world/entity_access/world_mut.rs | 21 +++- crates/bevy_ecs/src/world/mod.rs | 14 ++- 6 files changed, 193 insertions(+), 23 deletions(-) diff --git a/crates/bevy_ecs/src/archetype.rs b/crates/bevy_ecs/src/archetype.rs index e4b75653aa57b..08851331bb982 100644 --- a/crates/bevy_ecs/src/archetype.rs +++ b/crates/bevy_ecs/src/archetype.rs @@ -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 (see issue #24554). + insert_bundle_if_new: SparseArray, remove_bundle: SparseArray>, take_bundle: SparseArray>, } @@ -217,18 +222,26 @@ 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, false) .map(|bundle| bundle.archetype_id) } /// Internal version of `get_archetype_after_bundle_insert` that /// fetches the full `ArchetypeAfterBundleInsert`. + /// + /// `keep` selects the `InsertMode::Keep` edge (used by `insert_if_new`), which excludes + /// required components pulled in only by already-present components. #[inline] pub(crate) fn get_archetype_after_bundle_insert_internal( &self, bundle_id: BundleId, + keep: bool, ) -> Option<&ArchetypeAfterBundleInsert> { - self.insert_bundle.get(bundle_id) + if keep { + self.insert_bundle_if_new.get(bundle_id) + } else { + self.insert_bundle.get(bundle_id) + } } /// Caches the target archetype when inserting a bundle into the source archetype. @@ -236,6 +249,7 @@ impl Edges { pub(crate) fn cache_archetype_after_bundle_insert( &mut self, bundle_id: BundleId, + keep: bool, archetype_id: ArchetypeId, bundle_status: impl Into>, required_components: impl Into>, @@ -246,16 +260,18 @@ 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(), + }; + if keep { + self.insert_bundle_if_new.insert(bundle_id, edge); + } else { + 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..5697f0e99642d 100644 --- a/crates/bevy_ecs/src/bundle/insert.rs +++ b/crates/bevy_ecs/src/bundle/insert.rs @@ -39,11 +39,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,7 +58,11 @@ impl<'w> BundleInserter<'w> { archetype_id: ArchetypeId, bundle_id: BundleId, change_tick: Tick, + insert_mode: InsertMode, ) -> Self { + // `Keep` (`insert_if_new`) uses a distinct archetype edge that excludes required components + // pulled in only by already-present components (issue #24554). + let keep = insert_mode == InsertMode::Keep; // SAFETY: bundle exists per precondition let bundle_info = unsafe { world.bundles.get_unchecked(bundle_id) }; // SAFETY: same world @@ -68,6 +73,7 @@ impl<'w> BundleInserter<'w> { &world.components, &world.observers, archetype_id, + keep, ) }; @@ -84,7 +90,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, keep) .debug_checked_unwrap() .into() }; @@ -538,17 +544,24 @@ impl BundleInfo { components: &Components, observers: &Observers, archetype_id: ArchetypeId, + // When `true`, build the `InsertMode::Keep` edge: required components pulled in only by + // components already present in the source archetype are excluded from the target. + keep: bool, ) -> (ArchetypeId, bool) { - if let Some(archetype_after_insert_id) = archetypes[archetype_id] + if let Some(edge) = archetypes[archetype_id] .edges() - .get_archetype_after_bundle_insert(self.id) + .get_archetype_after_bundle_insert_internal(self.id, keep) { - 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(); + // Required-component ids transitively pulled in by the explicit components this insert + // actually adds. Required components only reachable from already-present explicit components + // are not in here, so they can be skipped under `InsertMode::Keep` (see issue #24554). + let mut required_by_added = Vec::new(); let mut added = Vec::new(); let mut existing = Vec::new(); @@ -562,6 +575,12 @@ impl BundleInfo { added.push(component_id); // SAFETY: component_id exists let component_info = unsafe { components.get_info_unchecked(component_id) }; + if keep { + // This explicit component is newly added, so its (transitive) required + // components are legitimately new and must remain in the `Keep` target. + required_by_added + .extend(component_info.required_components().all.keys().copied()); + } match component_info.storage_type() { StorageType::Table => new_table_components.push(component_id), StorageType::SparseSet => new_sparse_set_components.push(component_id), @@ -571,6 +590,11 @@ impl BundleInfo { for (index, component_id) in self.iter_required_components().enumerate() { if !current_archetype.contains(component_id) { + // Under `Keep`, skip required components pulled in only by components already + // present in the source archetype — they must not be (re-)added (issue #24554). + if keep && !required_by_added.contains(&component_id) { + continue; + } added_required_components.push(self.required_component_constructors[index].clone()); added.push(component_id); // SAFETY: component_id exists @@ -591,6 +615,7 @@ impl BundleInfo { // The archetype does not change when we insert this bundle. edges.cache_archetype_after_bundle_insert( self.id, + keep, archetype_id, bundle_status, added_required_components, @@ -648,6 +673,7 @@ impl BundleInfo { .edges_mut() .cache_archetype_after_bundle_insert( self.id, + keep, 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..99868c10edd4b 100644 --- a/crates/bevy_ecs/src/bundle/spawner.rs +++ b/crates/bevy_ecs/src/bundle/spawner.rs @@ -53,6 +53,8 @@ impl<'w> BundleSpawner<'w> { &world.components, &world.observers, ArchetypeId::EMPTY, + // Spawning always uses `InsertMode::Replace`. + false, ) }; diff --git a/crates/bevy_ecs/src/component/required.rs b/crates/bevy_ecs/src/component/required.rs index 632fad29fd90b..987e29b33cb0c 100644 --- a/crates/bevy_ecs/src/component/required.rs +++ b/crates/bevy_ecs/src/component/required.rs @@ -662,6 +662,109 @@ 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 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. From 1461154e8f30d2092ec1b73b613f1571818635ec Mon Sep 17 00:00:00 2001 From: satyakwok <119509589+satyakwok@users.noreply.github.com> Date: Sat, 13 Jun 2026 19:12:15 +0200 Subject: [PATCH 2/2] fix(ecs): refine insert_if_new required-component handling per review - thread InsertMode through the archetype-edge build instead of a bool - pick a shared required component's constructor from a requiree that is actually being added, not whichever came first in the bundle - look up the keep set with a map (O(1)) instead of a linear scan - drop resolved-issue links from comments - document the insert_if_new / required-components interaction and add a migration guide --- .../insert_if_new_required_components.md | 24 ++++++++ crates/bevy_ecs/src/archetype.rs | 30 +++++---- crates/bevy_ecs/src/bundle/insert.rs | 61 +++++++++++-------- crates/bevy_ecs/src/bundle/spawner.rs | 3 +- crates/bevy_ecs/src/component/mod.rs | 28 +++++++++ crates/bevy_ecs/src/component/required.rs | 23 +++++++ 6 files changed, 125 insertions(+), 44 deletions(-) create mode 100644 _release-content/migration-guides/insert_if_new_required_components.md 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 08851331bb982..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, @@ -208,7 +208,7 @@ pub struct Edges { // 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 (see issue #24554). + // does not resurrect a required component the caller removed. insert_bundle_if_new: SparseArray, remove_bundle: SparseArray>, take_bundle: SparseArray>, @@ -222,25 +222,24 @@ 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, false) + 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`. /// - /// `keep` selects the `InsertMode::Keep` edge (used by `insert_if_new`), which excludes - /// required components pulled in only by already-present components. + /// `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, - keep: bool, + insert_mode: InsertMode, ) -> Option<&ArchetypeAfterBundleInsert> { - if keep { - self.insert_bundle_if_new.get(bundle_id) - } else { - 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), } } @@ -249,7 +248,7 @@ impl Edges { pub(crate) fn cache_archetype_after_bundle_insert( &mut self, bundle_id: BundleId, - keep: bool, + insert_mode: InsertMode, archetype_id: ArchetypeId, bundle_status: impl Into>, required_components: impl Into>, @@ -267,11 +266,10 @@ impl Edges { added_len, inserted: added.into(), }; - if keep { - self.insert_bundle_if_new.insert(bundle_id, edge); - } else { - self.insert_bundle.insert(bundle_id, edge); - } + 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 5697f0e99642d..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}, @@ -60,9 +61,6 @@ impl<'w> BundleInserter<'w> { change_tick: Tick, insert_mode: InsertMode, ) -> Self { - // `Keep` (`insert_if_new`) uses a distinct archetype edge that excludes required components - // pulled in only by already-present components (issue #24554). - let keep = insert_mode == InsertMode::Keep; // SAFETY: bundle exists per precondition let bundle_info = unsafe { world.bundles.get_unchecked(bundle_id) }; // SAFETY: same world @@ -73,7 +71,7 @@ impl<'w> BundleInserter<'w> { &world.components, &world.observers, archetype_id, - keep, + insert_mode, ) }; @@ -90,7 +88,7 @@ impl<'w> BundleInserter<'w> { let archetype_after_insert = unsafe { archetype .edges() - .get_archetype_after_bundle_insert_internal(bundle_id, keep) + .get_archetype_after_bundle_insert_internal(bundle_id, insert_mode) .debug_checked_unwrap() .into() }; @@ -544,13 +542,14 @@ impl BundleInfo { components: &Components, observers: &Observers, archetype_id: ArchetypeId, - // When `true`, build the `InsertMode::Keep` edge: required components pulled in only by - // components already present in the source archetype are excluded from the target. - keep: bool, + insert_mode: InsertMode, ) -> (ArchetypeId, bool) { + // `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_internal(self.id, keep) + .get_archetype_after_bundle_insert_internal(self.id, insert_mode) { return (edge.archetype_id, false); } @@ -558,10 +557,12 @@ impl BundleInfo { 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(); - // Required-component ids transitively pulled in by the explicit components this insert - // actually adds. Required components only reachable from already-present explicit components - // are not in here, so they can be skipped under `InsertMode::Keep` (see issue #24554). - let mut required_by_added = 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(); @@ -576,10 +577,13 @@ impl BundleInfo { // SAFETY: component_id exists let component_info = unsafe { components.get_info_unchecked(component_id) }; if keep { - // This explicit component is newly added, so its (transitive) required - // components are legitimately new and must remain in the `Keep` target. - required_by_added - .extend(component_info.required_components().all.keys().copied()); + // 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), @@ -590,12 +594,17 @@ impl BundleInfo { for (index, component_id) in self.iter_required_components().enumerate() { if !current_archetype.contains(component_id) { - // Under `Keep`, skip required components pulled in only by components already - // present in the source archetype — they must not be (re-)added (issue #24554). - if keep && !required_by_added.contains(&component_id) { - continue; - } - 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) }; @@ -615,7 +624,7 @@ impl BundleInfo { // The archetype does not change when we insert this bundle. edges.cache_archetype_after_bundle_insert( self.id, - keep, + insert_mode, archetype_id, bundle_status, added_required_components, @@ -673,7 +682,7 @@ impl BundleInfo { .edges_mut() .cache_archetype_after_bundle_insert( self.id, - keep, + 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 99868c10edd4b..00af3068e89b3 100644 --- a/crates/bevy_ecs/src/bundle/spawner.rs +++ b/crates/bevy_ecs/src/bundle/spawner.rs @@ -53,8 +53,7 @@ impl<'w> BundleSpawner<'w> { &world.components, &world.observers, ArchetypeId::EMPTY, - // Spawning always uses `InsertMode::Replace`. - false, + 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 987e29b33cb0c..b3b72613e8ebc 100644 --- a/crates/bevy_ecs/src/component/required.rs +++ b/crates/bevy_ecs/src/component/required.rs @@ -765,6 +765,29 @@ mod tests { ); } + #[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)]