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:
<details>
<summary>Code</summary>
```
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);
}
}
```
</details>
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
```
This commit is contained in:
parent
20b2b5e6b1
commit
113d1b7dc1
@ -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;
|
||||
|
||||
@ -366,6 +366,13 @@ impl BlobVec {
|
||||
unsafe { core::slice::from_raw_parts(self.data.as_ptr() as *const UnsafeCell<T>, 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<unsafe fn(OwningPtr<'_>)> {
|
||||
self.drop
|
||||
}
|
||||
|
||||
/// Clears the vector, removing (and dropping) all values.
|
||||
///
|
||||
/// Note that this method has no effect on the allocated capacity of the vector.
|
||||
|
||||
@ -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<unsafe fn(OwningPtr<'_>)> {
|
||||
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."]
|
||||
|
||||
@ -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<unsafe fn(OwningPtr<'_>)> {
|
||||
self.data.get_drop()
|
||||
}
|
||||
}
|
||||
|
||||
Loading…
Reference in New Issue
Block a user