Skip to content

Fix sparse set components ignoring insert_if_new/InsertMode #19059

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 5, 2025

Conversation

JaySpruce
Copy link
Member

@JaySpruce JaySpruce commented May 4, 2025

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

@JaySpruce JaySpruce added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 4, 2025
@alice-i-cecile alice-i-cecile added this to the 0.16.1 milestone May 4, 2025
Copy link
Contributor

@chescock chescock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a unit test that would have caught this?

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 5, 2025
@mockersf mockersf added this pull request to the merge queue May 5, 2025
Merged via the queue into bevyengine:main with commit 113d1b7 May 5, 2025
41 checks passed
@JaySpruce JaySpruce deleted the fix_sparse_insertion branch May 6, 2025 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants