Skip to content
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

Remove extra call to clear_trackers #13762

Merged
merged 10 commits into from
Jun 10, 2024
43 changes: 40 additions & 3 deletions crates/bevy_app/src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -916,9 +916,17 @@ impl Termination for AppExit {

#[cfg(test)]
mod tests {
use std::{marker::PhantomData, mem};

use bevy_ecs::{event::EventWriter, schedule::ScheduleLabel, system::Commands};
use std::{iter, marker::PhantomData, mem};

use bevy_ecs::{
component::Component,
entity::Entity,
event::EventWriter,
query::With,
removal_detection::RemovedComponents,
schedule::{IntoSystemConfigs, ScheduleLabel},
system::{Commands, Query},
};

use crate::{App, AppExit, Plugin, Update};

Expand Down Expand Up @@ -1123,6 +1131,35 @@ mod tests {
);
}

#[test]
fn test_update_clears_trackers_once() {
#[derive(Component, Copy, Clone)]
struct Foo;

let mut app = App::new();
app.world_mut().spawn_batch(iter::repeat(Foo).take(5));

fn despawn_one_foo(mut commands: Commands, foos: Query<Entity, With<Foo>>) {
if let Some(e) = foos.iter().next() {
commands.entity(e).despawn();
};
}
fn check_despawns(mut removed_foos: RemovedComponents<Foo>) {
let mut despawn_count = 0;
for _ in removed_foos.read() {
despawn_count += 1;
}

assert_eq!(despawn_count, 2);
}

app.add_systems(Update, despawn_one_foo);
app.update(); // Frame 0
app.update(); // Frame 1
app.add_systems(Update, check_despawns.after(despawn_one_foo));
app.update(); // Should see despawns from frames 1 & 2, but not frame 0
}

#[test]
fn runner_returns_correct_exit_code() {
fn raise_exits(mut exits: EventWriter<AppExit>) {
Expand Down
2 changes: 0 additions & 2 deletions crates/bevy_app/src/sub_app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -429,8 +429,6 @@ impl SubApps {
sub_app.extract(&mut self.main.world);
sub_app.update();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Does removing this clear trackers rather than the other one mean that you can't use RemovedComponents in sub apps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RemovedComponents should still work the same for SubApps. clear_trackers is called once here per SubApp (the main app is also a SubApp internally): https://github.com/bevyengine/bevy/pull/13762/files/5888bf276daa1959eeb62671d1a8d6dd230eb6db#diff-1c2be7dcf5607cc6b3a4a9e664e9de0712a5ea313079b5451464513be643e747R136

Copy link
Contributor

Choose a reason for hiding this comment

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

There was some confusion in the past that you couldn't use Extract<RemovedComponents<T>> in the extraction subapp. I believe this extra call is here, because the intention was to not clear the trackers at the end of the main app schedule to enable this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm. If that's the case, then I think we would need another version of update For the main app that doesn't call clear_trackers.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds reasonable. But if it ends up being too involved, it might be better to merge this for 0.14 and fix it in a followup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wasn't too bad at all once I came up with a test that caught the issue. I couldn't find any problems using RemovedComponents since it has a two frame buffer, but this this was an issue with regular change detection.

self.main.world.clear_trackers();
}

/// Returns an iterator over the sub-apps (starting with the main one).
Expand Down