From 26fc4c71988cac5bed90ab1005bdea2517808ba7 Mon Sep 17 00:00:00 2001 From: Thierry Berger Date: Wed, 17 Jul 2024 23:05:48 +0200 Subject: [PATCH] Test for ambiguous system ordering in CI (#13950) Progress towards https://github.com/bevyengine/bevy/issues/7386. Following discussion https://discord.com/channels/691052431525675048/1253260494538539048/1253387942311886960 This Pull Request adds an example to detect system order ambiguities, and also asserts none exist. A lot of schedules are ignored in ordered to have the test passing, we should thrive to make them pass, but in other pull requests.
example output summary, without ignored schedules

```txt $ cargo run --example ambiguity_detection 2>&1 | grep -C 1 "pairs of syst" 2024-06-21T13:17:55.776585Z WARN bevy_ecs::schedule::schedule: Schedule First has ambiguities. 1 pairs of systems with conflicting data access have indeterminate execution order. Consider adding `before`, `after`, or `ambiguous_with` relationships between these: -- bevy_time::time_system (in set TimeSystem) and bevy_ecs::event::event_update_system (in set EventUpdates) -- 2024-06-21T13:17:55.782265Z WARN bevy_ecs::schedule::schedule: Schedule PreUpdate has ambiguities. 11 pairs of systems with conflicting data access have indeterminate execution order. Consider adding `before`, `after`, or `ambiguous_with` relationships between these: -- bevy_pbr::prepass::update_mesh_previous_global_transforms and bevy_asset::server::handle_internal_asset_events -- 2024-06-21T13:17:55.809516Z WARN bevy_ecs::schedule::schedule: Schedule PostUpdate has ambiguities. 63 pairs of systems with conflicting data access have indeterminate execution order. Consider adding `before`, `after`, or `ambiguous_with` relationships between these: -- bevy_ui::accessibility::image_changed and bevy_ecs::schedule::executor::apply_deferred -- 2024-06-21T13:17:55.816287Z WARN bevy_ecs::schedule::schedule: Schedule Last has ambiguities. 3 pairs of systems with conflicting data access have indeterminate execution order. Consider adding `before`, `after`, or `ambiguous_with` relationships between these: -- bevy_gizmos::update_gizmo_meshes (in set UpdateGizmoMeshes) and bevy_gizmos::update_gizmo_meshes (in set UpdateGizmoMeshes) -- 2024-06-21T13:17:55.831074Z WARN bevy_ecs::schedule::schedule: Schedule ExtractSchedule has ambiguities. 296 pairs of systems with conflicting data access have indeterminate execution order. Consider adding `before`, `after`, or `ambiguous_with` relationships between these: -- bevy_render::extract_component::extract_components and bevy_render::render_asset::extract_render_asset> ```

To try locally: ```sh CI_TESTING_CONFIG="./.github/example-run/ambiguity_detection.ron" cargo run --example ambiguity_detection --features "bevy_ci_testing,trace,trace_chrome" ``` --------- Co-authored-by: Jan Hohenheim --- .github/example-run/ambiguity_detection.ron | 2 + Cargo.toml | 9 ++ crates/bevy_dev_tools/src/ci_testing/mod.rs | 7 +- tests/ecs/ambiguity_detection.rs | 128 ++++++++++++++++++++ 4 files changed, 144 insertions(+), 2 deletions(-) create mode 100644 .github/example-run/ambiguity_detection.ron create mode 100644 tests/ecs/ambiguity_detection.rs diff --git a/.github/example-run/ambiguity_detection.ron b/.github/example-run/ambiguity_detection.ron new file mode 100644 index 0000000000..72873dd667 --- /dev/null +++ b/.github/example-run/ambiguity_detection.ron @@ -0,0 +1,2 @@ +( +) diff --git a/Cargo.toml b/Cargo.toml index 8c30d93cf3..456689e02d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -365,6 +365,7 @@ ron = "0.8.0" flate2 = "1.0" serde = { version = "1", features = ["derive"] } bytemuck = "1.7" +bevy_render = { path = "crates/bevy_render", version = "0.15.0-dev", default-features = false } # Needed to poll Task examples futures-lite = "2.0.1" async-std = "1.12" @@ -2980,6 +2981,14 @@ description = "Demonstrates customizing default window settings" category = "Window" wasm = true +[[example]] +name = "ambiguity_detection" +path = "tests/ecs/ambiguity_detection.rs" +doc-scrape-examples = true + +[package.metadata.example.ambiguity_detection] +hidden = true + [[example]] name = "resizing" path = "tests/window/resizing.rs" diff --git a/crates/bevy_dev_tools/src/ci_testing/mod.rs b/crates/bevy_dev_tools/src/ci_testing/mod.rs index 0e10eb1a5b..f6510f68a1 100644 --- a/crates/bevy_dev_tools/src/ci_testing/mod.rs +++ b/crates/bevy_dev_tools/src/ci_testing/mod.rs @@ -6,6 +6,7 @@ mod systems; pub use self::config::*; use bevy_app::prelude::*; +use bevy_ecs::schedule::IntoSystemConfigs; use bevy_time::TimeUpdateStrategy; use std::time::Duration; @@ -46,9 +47,11 @@ impl Plugin for CiTestingPlugin { fixed_frame_time, ))); } - app.add_event::() .insert_resource(config) - .add_systems(Update, systems::send_events); + .add_systems( + Update, + systems::send_events.before(bevy_window::close_when_requested), + ); } } diff --git a/tests/ecs/ambiguity_detection.rs b/tests/ecs/ambiguity_detection.rs new file mode 100644 index 0000000000..98c5c2fb77 --- /dev/null +++ b/tests/ecs/ambiguity_detection.rs @@ -0,0 +1,128 @@ +//! A test to confirm that `bevy` doesn't regress its system ambiguities count when using [`DefaultPlugins`]. +//! This is run in CI. + +use bevy::{ + ecs::schedule::{InternedScheduleLabel, LogLevel, ScheduleBuildSettings, ScheduleLabel}, + prelude::*, + utils::HashMap, +}; +use bevy_render::{pipelined_rendering::RenderExtractApp, Render, RenderApp}; + +/// FIXME: bevy should not have any ambiguities, but it takes time to clean these up, +/// so we're just ignoring those for now. +/// +/// See [#7386](https://github.com/bevyengine/bevy/issues/7386) for relevant issue. +pub fn get_ignored_ambiguous_system_schedules() -> Vec> { + vec![ + Box::new(First), + Box::new(PreUpdate), + Box::new(Update), + Box::new(PostUpdate), + Box::new(Last), + Box::new(ExtractSchedule), + Box::new(Render), + ] +} + +/// A test to confirm that `bevy` doesn't regress its system ambiguities count when using [`DefaultPlugins`]. +/// This is run in CI. +pub fn main() { + let mut app = App::new(); + app.add_plugins(DefaultPlugins); + + let sub_app = app.main_mut(); + configure_ambiguity_detection(sub_app); + let sub_app = app.sub_app_mut(RenderApp); + configure_ambiguity_detection(sub_app); + let sub_app = app.sub_app_mut(RenderExtractApp); + configure_ambiguity_detection(sub_app); + + app.finish(); + app.cleanup(); + app.update(); + + let sub_app = app.main(); + + let ignored_schedules = get_ignored_ambiguous_system_schedules(); + + let ambiguities = count_ambiguities(sub_app); + let mut unexpected_ambiguities = vec![]; + for (&label, &count) in ambiguities.0.iter() { + if ignored_schedules + .iter() + .any(|label_to_ignore| **label_to_ignore == *label) + { + continue; + } + if count != 0 { + unexpected_ambiguities.push(label); + } + } + assert_eq!( + unexpected_ambiguities.len(), + 0, + "Main app has unexpected ambiguities among these schedules: {:?}.\n\ + More Details:\n{:#?}", + unexpected_ambiguities, + ambiguities + ); + + let total_ambiguities = ambiguities.total(); + assert_eq!( + total_ambiguities, 82, + "Main app does not have an expected conflicting systems count, \ + you might consider verifying if it's normal, or change the expected number.\n\ + Details:\n{:#?}", + ambiguities + ); + + // RenderApp is not checked here, because it is not within the App at this point. + let sub_app = app.sub_app(RenderExtractApp); + + let ambiguities = count_ambiguities(sub_app); + let total_ambiguities = ambiguities.total(); + assert_eq!( + total_ambiguities, 0, + "RenderExtractApp contains conflicting systems.", + ); +} + +/// Contains the number of conflicting systems per schedule. +#[derive(Debug, Deref, DerefMut)] +struct AmbiguitiesCount(pub HashMap); + +impl AmbiguitiesCount { + fn total(&self) -> usize { + self.values().sum() + } +} + +fn configure_ambiguity_detection(sub_app: &mut SubApp) { + let ignored_ambiguous_systems = get_ignored_ambiguous_system_schedules(); + let mut schedules = sub_app.world_mut().resource_mut::(); + for (_, schedule) in schedules.iter_mut() { + if ignored_ambiguous_systems + .iter() + .any(|label| **label == *schedule.label()) + { + // Note: you can remove this bypass to get full details about ambiguities. + continue; + } + schedule.set_build_settings(ScheduleBuildSettings { + ambiguity_detection: LogLevel::Warn, + use_shortnames: false, + ..default() + }); + } +} + +/// Returns the number of conflicting systems per schedule. +fn count_ambiguities(sub_app: &SubApp) -> AmbiguitiesCount { + let schedules = sub_app.world().resource::(); + let mut ambiguities = HashMap::new(); + for (_, schedule) in schedules.iter() { + let ambiguities_in_schedule = schedule.graph().conflicting_systems().len(); + ambiguities.insert(schedule.label(), ambiguities_in_schedule); + } + AmbiguitiesCount(ambiguities) +}