Skip to content

Commit

Permalink
Remove extra call to clear_trackers (#13762)
Browse files Browse the repository at this point in the history
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();
    }
}
```
  • Loading branch information
chrisjuchem authored and mockersf committed Jun 10, 2024
1 parent effbcdf commit 65dbfe2
Show file tree
Hide file tree
Showing 4 changed files with 87 additions and 17 deletions.
79 changes: 71 additions & 8 deletions crates/bevy_app/src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -919,14 +919,21 @@ impl Termination for AppExit {

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

use bevy_ecs::prelude::{Resource, World};
use bevy_ecs::world::FromWorld;
use bevy_ecs::{event::EventWriter, schedule::ScheduleLabel, system::Commands};

use crate::{App, AppExit, Plugin, Update};
use std::{iter, marker::PhantomData, mem, sync::Mutex};

use bevy_ecs::{
change_detection::{DetectChanges, ResMut},
component::Component,
entity::Entity,
event::EventWriter,
query::With,
removal_detection::RemovedComponents,
schedule::{IntoSystemConfigs, ScheduleLabel},
system::{Commands, Query, Resource},
world::{FromWorld, World},
};

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

struct PluginA;
impl Plugin for PluginA {
Expand Down Expand Up @@ -1129,6 +1136,62 @@ 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 test_extract_sees_changes() {
use super::AppLabel;
use crate::{self as bevy_app};

#[derive(AppLabel, Clone, Copy, Hash, PartialEq, Eq, Debug)]
struct MySubApp;

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

let mut app = App::new();
app.world_mut().insert_resource(Foo(0));
app.add_systems(Update, |mut foo: ResMut<Foo>| {
foo.0 += 1;
});

let mut sub_app = SubApp::new();
sub_app.set_extract(|main_world, _sub_world| {
assert!(main_world.get_resource_ref::<Foo>().unwrap().is_changed());
});

app.insert_sub_app(MySubApp, sub_app);

app.update();
}

#[test]
fn runner_returns_correct_exit_code() {
fn raise_exits(mut exits: EventWriter<AppExit>) {
Expand Down
11 changes: 9 additions & 2 deletions crates/bevy_app/src/sub_app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,14 +125,21 @@ impl SubApp {
}

/// Runs the default schedule.
pub fn update(&mut self) {
///
/// Does not clear internal trackers used for change detection.
pub fn run_default_schedule(&mut self) {
if self.is_building_plugins() {
panic!("SubApp::update() was called while a plugin was building.");
}

if let Some(label) = self.update_schedule {
self.world.run_schedule(label);
}
}

/// Runs the default schedule and updates internal component trackers.
pub fn update(&mut self) {
self.run_default_schedule();
self.world.clear_trackers();
}

Expand Down Expand Up @@ -421,7 +428,7 @@ impl SubApps {
{
#[cfg(feature = "trace")]
let _bevy_frame_update_span = info_span!("main app").entered();
self.main.update();
self.main.run_default_schedule();
}
for (_label, sub_app) in self.sub_apps.iter_mut() {
#[cfg(feature = "trace")]
Expand Down
8 changes: 4 additions & 4 deletions crates/bevy_ecs/src/removal_detection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,11 +116,11 @@ impl RemovedComponentEvents {
///
/// If you are using `bevy_ecs` as a standalone crate,
/// note that the `RemovedComponents` list will not be automatically cleared for you,
/// and will need to be manually flushed using [`World::clear_trackers`](World::clear_trackers)
/// and will need to be manually flushed using [`World::clear_trackers`](World::clear_trackers).
///
/// For users of `bevy` and `bevy_app`, this is automatically done in `bevy_app::App::update`.
/// For the main world, [`World::clear_trackers`](World::clear_trackers) is run after the main schedule is run and after
/// `SubApp`'s have run.
/// For users of `bevy` and `bevy_app`, [`World::clear_trackers`](World::clear_trackers) is
/// automatically called by `bevy_app::App::update` and `bevy_app::SubApp::update`.
/// For the main world, this is delayed until after all `SubApp`s have run.
///
/// # Examples
///
Expand Down
6 changes: 3 additions & 3 deletions crates/bevy_ecs/src/world/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1113,9 +1113,9 @@ impl World {
/// By clearing this internal state, the world "forgets" about those changes, allowing a new round
/// of detection to be recorded.
///
/// When using `bevy_ecs` as part of the full Bevy engine, this method is added as a system to the
/// main app, to run during `Last`, so you don't need to call it manually. When using `bevy_ecs`
/// as a separate standalone crate however, you need to call this manually.
/// When using `bevy_ecs` as part of the full Bevy engine, this method is called automatically
/// by `bevy_app::App::update` and `bevy_app::SubApp::update`, so you don't need to call it manually.
/// When using `bevy_ecs` as a separate standalone crate however, you do need to call this manually.
///
/// ```
/// # use bevy_ecs::prelude::*;
Expand Down

0 comments on commit 65dbfe2

Please sign in to comment.