Skip to content

RemovedComponents can miss removals in 0.14.0-rc2 #13758

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

Closed
chrisjuchem opened this issue Jun 8, 2024 · 3 comments · Fixed by #13762
Closed

RemovedComponents can miss removals in 0.14.0-rc2 #13758

chrisjuchem opened this issue Jun 8, 2024 · 3 comments · Fixed by #13762
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior
Milestone

Comments

@chrisjuchem
Copy link
Contributor

Bevy version

0.14.0-rc.2

Reproduction Example

use bevy::core::{FrameCount, FrameCountPlugin};
use bevy::prelude::*;

#[derive(Component)]
struct Foo;

fn main() {
    let mut app = App::new();
    app.add_plugins(FrameCountPlugin);
    app.add_systems(Startup, |mut commands: Commands| {
        for _ in 0..100 {
            commands.spawn(Foo);
        }
    });

    app.add_systems(First, |counter: Res<FrameCount>| {
        println!("Frame {}:", counter.0)
    });

    fn detector_system(mut removals: RemovedComponents<Foo>, foos: Query<Entity, With<Foo>>) {
        for e in removals.read() {
            println!("  Detected removed Foo component for {e:?}")
        }
        println!("  Total Foos: {}", foos.iter().count())
    }
    fn deleter_system(foos: Query<Entity, With<Foo>>, mut commands: Commands) {
        foos.iter().next().map(|e| {
            commands.entity(e).remove::<Foo>();
        });
    }
    app.add_systems(Update, (detector_system, deleter_system).chain());

    app.update();
    app.update();
    app.update();
    app.update();
}

Output when using bevy 0.13.2

Frame 0:
  Total Foos: 100
Frame 1:
  Detected removed Foo component for 0v1
  Total Foos: 99
Frame 2:
  Detected removed Foo component for 99v1
  Total Foos: 98
Frame 3:
  Detected removed Foo component for 98v1
  Total Foos: 97

Output when using bevy `0.14.0-rc.2

Frame 0:
  Total Foos: 100
Frame 1:
  Total Foos: 99
Frame 2:
  Total Foos: 98
Frame 3:
  Total Foos: 97

Additional information

Swapping the order of detector_system and deleter_system results in the events showing up as expected.

Replacing remove::<Foo> with .despawn() does not change the outcome.

@chrisjuchem chrisjuchem added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Jun 8, 2024
@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Jun 8, 2024
@alice-i-cecile
Copy link
Member

I believe this is a genuine bug: we changed the backing of RemovedComponents to use double buffered events at some point. It's the weekend right now but bother me if you can't find the PR.

@chrisjuchem
Copy link
Contributor Author

Looks like that was back in 0.10 #5680

@alice-i-cecile
Copy link
Member

Great! Can you turn your example into a test and try to bisect the problem?

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events S-Needs-Investigation This issue requires detective work to figure out what's going wrong and removed S-Needs-Triage This issue needs to be labelled labels Jun 8, 2024
@NthTensor NthTensor removed the S-Needs-Investigation This issue requires detective work to figure out what's going wrong label Jun 9, 2024
github-merge-queue bot pushed a commit that referenced this issue Jun 10, 2024
Fixes #13758.

# Objective

Calling `update` on the main app already calls `clear_trackers`. Calling
it again in `SubApps::update` caused RemovedCompenet Events to be
cleared earlier than they should be.

## Solution

- Don't call clear_trackers an extra time.

## Testing

I manually tested the fix with this unit test: 
```
#[cfg(test)]
mod test {
    use crate::core::{FrameCount, FrameCountPlugin};
    use crate::prelude::*;

    #[test]
    fn test_next_frame_removal() {
        #[derive(Component)]
        struct Foo;

        #[derive(Resource)]
        struct RemovedCount(usize);

        let mut app = App::new();
        app.add_plugins(FrameCountPlugin);
        app.add_systems(Startup, |mut commands: Commands| {
            for _ in 0..100 {
                commands.spawn(Foo);
            }
            commands.insert_resource(RemovedCount(0));
        });

        app.add_systems(First, |counter: Res<FrameCount>| {
            println!("Frame {}:", counter.0)
        });

        fn detector_system(
            mut removals: RemovedComponents<Foo>,
            foos: Query<Entity, With<Foo>>,
            mut removed_c: ResMut<RemovedCount>,
        ) {
            for e in removals.read() {
                println!("  Detected removed Foo component for {e:?}");
                removed_c.0 += 1;
            }
            let c = foos.iter().count();
            println!("  Total Foos: {}", c);
            assert_eq!(c + removed_c.0, 100);
        }
        fn deleter_system(foos: Query<Entity, With<Foo>>, mut commands: Commands) {
            foos.iter().next().map(|e| {
                commands.entity(e).remove::<Foo>();
            });
        }
        app.add_systems(Update, (detector_system, deleter_system).chain());

        app.update();
        app.update();
        app.update();
        app.update();
    }
}
```
mockersf pushed a commit that referenced this issue Jun 10, 2024
Fixes #13758.

# Objective

Calling `update` on the main app already calls `clear_trackers`. Calling
it again in `SubApps::update` caused RemovedCompenet Events to be
cleared earlier than they should be.

## Solution

- Don't call clear_trackers an extra time.

## Testing

I manually tested the fix with this unit test: 
```
#[cfg(test)]
mod test {
    use crate::core::{FrameCount, FrameCountPlugin};
    use crate::prelude::*;

    #[test]
    fn test_next_frame_removal() {
        #[derive(Component)]
        struct Foo;

        #[derive(Resource)]
        struct RemovedCount(usize);

        let mut app = App::new();
        app.add_plugins(FrameCountPlugin);
        app.add_systems(Startup, |mut commands: Commands| {
            for _ in 0..100 {
                commands.spawn(Foo);
            }
            commands.insert_resource(RemovedCount(0));
        });

        app.add_systems(First, |counter: Res<FrameCount>| {
            println!("Frame {}:", counter.0)
        });

        fn detector_system(
            mut removals: RemovedComponents<Foo>,
            foos: Query<Entity, With<Foo>>,
            mut removed_c: ResMut<RemovedCount>,
        ) {
            for e in removals.read() {
                println!("  Detected removed Foo component for {e:?}");
                removed_c.0 += 1;
            }
            let c = foos.iter().count();
            println!("  Total Foos: {}", c);
            assert_eq!(c + removed_c.0, 100);
        }
        fn deleter_system(foos: Query<Entity, With<Foo>>, mut commands: Commands) {
            foos.iter().next().map(|e| {
                commands.entity(e).remove::<Foo>();
            });
        }
        app.add_systems(Update, (detector_system, deleter_system).chain());

        app.update();
        app.update();
        app.update();
        app.update();
    }
}
```
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants