From c64f628bfb0f6dba03c77733783431fb8a7b3530 Mon Sep 17 00:00:00 2001 From: JaySpruce Date: Mon, 5 May 2025 12:42:36 -0500 Subject: [PATCH] Fix sparse set components ignoring `insert_if_new`/`InsertMode` (#19059) # Objective I've been tinkering with ECS insertion/removal lately, and noticed that sparse sets just... don't interact with `InsertMode` at all. Sure enough, using `insert_if_new` with a sparse component does the same thing as `insert`. # Solution - Add a check in `BundleInfo::write_components` to drop the new value if the entity already has the component and `InsertMode` is `Keep`. - Add necessary methods to sparse set internals to fetch the drop function. # Testing Minimal reproduction:
Code ``` use bevy::prelude::*; fn main() { App::new() .add_plugins(DefaultPlugins) .add_systems(Startup, setup) .add_systems(PostStartup, component_print) .run(); } #[derive(Component)] #[component(storage = "SparseSet")] struct SparseComponent(u32); fn setup(mut commands: Commands) { let mut entity = commands.spawn_empty(); entity.insert(SparseComponent(1)); entity.insert(SparseComponent(2)); let mut entity = commands.spawn_empty(); entity.insert(SparseComponent(3)); entity.insert_if_new(SparseComponent(4)); } fn component_print(query: Query<&SparseComponent>) { for component in &query { info!("{}", component.0); } } ```
Here it is on Bevy Playground (0.15.3): https://learnbevy.com/playground?share=2a96a68a81e804d3fdd644a833c1d51f7fa8dd33fc6192fbfd077b082a6b1a41 Output on `main`: ``` 2025-05-04T17:50:50.401328Z INFO system{name="fork::component_print"}: fork: 2 2025-05-04T17:50:50.401583Z INFO system{name="fork::component_print"}: fork: 4 ``` Output with this PR : ``` 2025-05-04T17:51:33.461835Z INFO system{name="fork::component_print"}: fork: 2 2025-05-04T17:51:33.462091Z INFO system{name="fork::component_print"}: fork: 3 ``` --- crates/bevy_ecs/src/bundle.rs | 22 +++++++++++++++------ crates/bevy_ecs/src/storage/blob_vec.rs | 7 +++++++ crates/bevy_ecs/src/storage/sparse_set.rs | 7 +++++++ crates/bevy_ecs/src/storage/table/column.rs | 7 +++++++ 4 files changed, 37 insertions(+), 6 deletions(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index 5666d90c53..32a91279cb 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -630,13 +630,14 @@ impl BundleInfo { let mut bundle_component = 0; let after_effect = bundle.get_components(&mut |storage_type, component_ptr| { let component_id = *self.component_ids.get_unchecked(bundle_component); + // SAFETY: bundle_component is a valid index for this bundle + let status = unsafe { bundle_component_status.get_status(bundle_component) }; match storage_type { StorageType::Table => { - // SAFETY: bundle_component is a valid index for this bundle - let status = unsafe { bundle_component_status.get_status(bundle_component) }; - // SAFETY: If component_id is in self.component_ids, BundleInfo::new ensures that - // the target table contains the component. - let column = table.get_column_mut(component_id).debug_checked_unwrap(); + let column = + // SAFETY: If component_id is in self.component_ids, BundleInfo::new ensures that + // the target table contains the component. + unsafe { table.get_column_mut(component_id).debug_checked_unwrap() }; match (status, insert_mode) { (ComponentStatus::Added, _) => { column.initialize(table_row, component_ptr, change_tick, caller); @@ -656,7 +657,16 @@ impl BundleInfo { // SAFETY: If component_id is in self.component_ids, BundleInfo::new ensures that // a sparse set exists for the component. unsafe { sparse_sets.get_mut(component_id).debug_checked_unwrap() }; - sparse_set.insert(entity, component_ptr, change_tick, caller); + match (status, insert_mode) { + (ComponentStatus::Added, _) | (_, InsertMode::Replace) => { + sparse_set.insert(entity, component_ptr, change_tick, caller); + } + (ComponentStatus::Existing, InsertMode::Keep) => { + if let Some(drop_fn) = sparse_set.get_drop() { + drop_fn(component_ptr); + } + } + } } } bundle_component += 1; diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs index 2451fccb14..85852a2bea 100644 --- a/crates/bevy_ecs/src/storage/blob_vec.rs +++ b/crates/bevy_ecs/src/storage/blob_vec.rs @@ -366,6 +366,13 @@ impl BlobVec { unsafe { core::slice::from_raw_parts(self.data.as_ptr() as *const UnsafeCell, self.len) } } + /// Returns the drop function for values stored in the vector, + /// or `None` if they don't need to be dropped. + #[inline] + pub fn get_drop(&self) -> Option)> { + self.drop + } + /// Clears the vector, removing (and dropping) all values. /// /// Note that this method has no effect on the allocated capacity of the vector. diff --git a/crates/bevy_ecs/src/storage/sparse_set.rs b/crates/bevy_ecs/src/storage/sparse_set.rs index bb79382e06..6c809df849 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -300,6 +300,13 @@ impl ComponentSparseSet { }) } + /// Returns the drop function for the component type stored in the sparse set, + /// or `None` if it doesn't need to be dropped. + #[inline] + pub fn get_drop(&self) -> Option)> { + self.dense.get_drop() + } + /// Removes the `entity` from this sparse set and returns a pointer to the associated value (if /// it exists). #[must_use = "The returned pointer must be used to drop the removed component."] diff --git a/crates/bevy_ecs/src/storage/table/column.rs b/crates/bevy_ecs/src/storage/table/column.rs index d4690d264c..522df222c6 100644 --- a/crates/bevy_ecs/src/storage/table/column.rs +++ b/crates/bevy_ecs/src/storage/table/column.rs @@ -697,4 +697,11 @@ impl Column { changed_by.get_unchecked(row.as_usize()) }) } + + /// Returns the drop function for elements of the column, + /// or `None` if they don't need to be dropped. + #[inline] + pub fn get_drop(&self) -> Option)> { + self.data.get_drop() + } }