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
92cda8b0bb
commit
c64f628bfb
@ -630,13 +630,14 @@ impl BundleInfo {
|
|||||||
let mut bundle_component = 0;
|
let mut bundle_component = 0;
|
||||||
let after_effect = bundle.get_components(&mut |storage_type, component_ptr| {
|
let after_effect = bundle.get_components(&mut |storage_type, component_ptr| {
|
||||||
let component_id = *self.component_ids.get_unchecked(bundle_component);
|
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 {
|
match storage_type {
|
||||||
StorageType::Table => {
|
StorageType::Table => {
|
||||||
// SAFETY: bundle_component is a valid index for this bundle
|
let column =
|
||||||
let status = unsafe { bundle_component_status.get_status(bundle_component) };
|
// SAFETY: If component_id is in self.component_ids, BundleInfo::new ensures that
|
||||||
// SAFETY: If component_id is in self.component_ids, BundleInfo::new ensures that
|
// the target table contains the component.
|
||||||
// the target table contains the component.
|
unsafe { table.get_column_mut(component_id).debug_checked_unwrap() };
|
||||||
let column = table.get_column_mut(component_id).debug_checked_unwrap();
|
|
||||||
match (status, insert_mode) {
|
match (status, insert_mode) {
|
||||||
(ComponentStatus::Added, _) => {
|
(ComponentStatus::Added, _) => {
|
||||||
column.initialize(table_row, component_ptr, change_tick, caller);
|
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
|
// SAFETY: If component_id is in self.component_ids, BundleInfo::new ensures that
|
||||||
// a sparse set exists for the component.
|
// a sparse set exists for the component.
|
||||||
unsafe { sparse_sets.get_mut(component_id).debug_checked_unwrap() };
|
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;
|
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) }
|
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.
|
/// Clears the vector, removing (and dropping) all values.
|
||||||
///
|
///
|
||||||
/// Note that this method has no effect on the allocated capacity of the vector.
|
/// 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
|
/// Removes the `entity` from this sparse set and returns a pointer to the associated value (if
|
||||||
/// it exists).
|
/// it exists).
|
||||||
#[must_use = "The returned pointer must be used to drop the removed component."]
|
#[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())
|
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