diff --git a/Cargo.toml b/Cargo.toml index 0fafd91be7970..4c7c1fbc7dc42 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -258,6 +258,10 @@ name = "custom_diagnostic" path = "examples/diagnostics/custom_diagnostic.rs" # ECS (Entity Component System) +[[example]] +name = "ambiguity_checker" +path = "examples/ecs/ambiguity_checker.rs" + [[example]] name = "ecs_guide" path = "examples/ecs/ecs_guide.rs" diff --git a/crates/bevy_audio/src/lib.rs b/crates/bevy_audio/src/lib.rs index 5f44d68f9f40b..2ddbecafaea59 100644 --- a/crates/bevy_audio/src/lib.rs +++ b/crates/bevy_audio/src/lib.rs @@ -13,7 +13,7 @@ pub use audio_source::*; use bevy_app::prelude::*; use bevy_asset::AddAsset; -use bevy_ecs::system::IntoExclusiveSystem; +use bevy_ecs::{schedule::ReportExecutionOrderAmbiguities, system::IntoExclusiveSystem}; /// Adds support for audio playback to an App #[derive(Default)] @@ -31,5 +31,10 @@ impl Plugin for AudioPlugin { #[cfg(any(feature = "mp3", feature = "flac", feature = "wav", feature = "vorbis"))] app.init_asset_loader::(); + + app.world + .get_resource_or_insert_with(ReportExecutionOrderAmbiguities::minimal) + .ignore_crates + .push("bevy_audio".to_string()); } } diff --git a/crates/bevy_ecs/src/schedule/stage.rs b/crates/bevy_ecs/src/schedule/stage.rs index 9d5dda4bdf385..ad9c46b4d86e5 100644 --- a/crates/bevy_ecs/src/schedule/stage.rs +++ b/crates/bevy_ecs/src/schedule/stage.rs @@ -11,12 +11,15 @@ use crate::{ }, world::{World, WorldId}, }; -use bevy_utils::{tracing::info, HashMap, HashSet}; +use bevy_utils::{ + tracing::{info, warn}, + HashMap, HashSet, +}; use downcast_rs::{impl_downcast, Downcast}; use fixedbitset::FixedBitSet; use std::fmt::Debug; -use super::IntoSystemDescriptor; +use super::{AmbiguityDetection, IntoSystemDescriptor}; pub trait Stage: Downcast + Send + Sync { /// Runs the stage; this happens once per update. @@ -26,10 +29,13 @@ pub trait Stage: Downcast + Send + Sync { impl_downcast!(Stage); -/// When this resource is present in the `App`'s `Resources`, -/// each `SystemStage` will log a report containing -/// pairs of systems with ambiguous execution order. -/// +#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] +pub enum AmbiguityReportLevel { + Off, + Minimal, + Verbose, +} + /// Systems that access the same Component or Resource within the same stage /// risk an ambiguous order that could result in logic bugs, unless they have an /// explicit execution ordering constraint between them. @@ -45,7 +51,78 @@ impl_downcast!(Stage); /// /// The checker may report a system more times than the amount of constraints it would actually need /// to have unambiguous order with regards to a group of already-constrained systems. -pub struct ReportExecutionOrderAmbiguities; +/// +/// By default only a warning with the number of unresolved ambiguities detected will be reported per [`SystemStage`]. +/// This behavior can be changed by explicitly adding this resource using the following constructors: +/// * [ReportExecutionOrderAmbiguities::off()] - Disables all messages reported by the ambiguity checker. +/// * [ReportExecutionOrderAmbiguities::minimal()] - Displays only the number of unresolved ambiguities detected by the ambiguity checker. +/// * [ReportExecutionOrderAmbiguities::verbose()] - Displays a full report of ambiguities detected by the ambiguity checker. +/// +/// The ambiguity checker will ignore ambiguities within official Bevy crates. +/// To ignore a custom crate, use [`ReportExecutionOrderAmbiguities::ignore`] +/// with an list of crate names as an argument. +/// This resource should be added before any bevy internal plugin. +/// +/// ## Example +/// ```ignore +/// # use bevy_app::App; +/// # use bevy_ecs::schedule::ReportExecutionOrderAmbiguities; +/// App::new() +/// .insert_resource(ReportExecutionOrderAmbiguities::verbose().ignore(&["my_external_crate"])); +/// ``` +pub struct ReportExecutionOrderAmbiguities { + pub level: AmbiguityReportLevel, + pub ignore_crates: Vec, +} + +impl ReportExecutionOrderAmbiguities { + /// Disables all messages reported by the ambiguity checker. + pub fn off() -> Self { + Self { + level: AmbiguityReportLevel::Off, + ..Default::default() + } + } + + /// Displays only the number of unresolved ambiguities detected by the ambiguity checker. This is the default behavior. + pub fn minimal() -> Self { + Self { + level: AmbiguityReportLevel::Minimal, + ..Default::default() + } + } + + /// Displays a full report of ambiguities detected by the ambiguity checker. + pub fn verbose() -> Self { + Self { + level: AmbiguityReportLevel::Verbose, + ..Default::default() + } + } + + /// Adds the given crate to be ignored by ambiguity checker. Check [`ReportExecutionOrderAmbiguities`] for more details. + pub fn ignore(mut self, crate_prefix: &str) -> Self { + self.ignore_crates.push(crate_prefix.to_string()); + self + } + + /// Adds all the given crates to be ignored by ambiguity checker. Check [`ReportExecutionOrderAmbiguities`] for more details. + pub fn ignore_all(mut self, crate_prefixes: &[&str]) -> Self { + for s in crate_prefixes { + self.ignore_crates.push(s.to_string()); + } + self + } +} + +impl Default for ReportExecutionOrderAmbiguities { + fn default() -> Self { + Self { + level: AmbiguityReportLevel::Minimal, + ignore_crates: vec![], + } + } +} /// Stores and executes systems. Execution order is not defined unless explicitly specified; /// see `SystemDescriptor` documentation. @@ -470,7 +547,14 @@ impl SystemStage { } /// Logs execution order ambiguities between systems. System orders must be fresh. - fn report_ambiguities(&self, world: &World) { + fn report_ambiguities(&self, world: &mut World) { + let ambiguity_report = + world.get_resource_or_insert_with(ReportExecutionOrderAmbiguities::default); + + if ambiguity_report.level == AmbiguityReportLevel::Off { + return; + } + debug_assert!(!self.systems_modified); use std::fmt::Write; fn write_display_names_of_pairs( @@ -478,61 +562,111 @@ impl SystemStage { systems: &[impl SystemContainer], mut ambiguities: Vec<(usize, usize, Vec)>, world: &World, + output_prefix: &str, ) { - for (index_a, index_b, conflicts) in ambiguities.drain(..) { + use std::collections::hash_map; + + let mut ambiguities_map: hash_map::HashMap, Vec> = + hash_map::HashMap::new(); + + for (index_a, index_b, components) in ambiguities.drain(..) { + let systems = ambiguities_map.entry(components).or_default(); + if !systems.contains(&index_a) { + systems.push(index_a); + } + if !systems.contains(&index_b) { + systems.push(index_b); + } + } + + for (idx, (conflicts, systems_indices)) in ambiguities_map.drain().enumerate() { + let system_names = systems_indices + .iter() + .map(|index| systems[*index].name()) + .collect::>(); + + let system_components = conflicts + .iter() + .map(|id| world.components().get_info(*id).unwrap().name()) + .collect::>(); + writeln!( string, - " -- {:?} and {:?}", - systems[index_a].name(), - systems[index_b].name() + "{}.{} - Systems:\n {:?}\n - Conflict on the following components/resources:\n {:?}", + output_prefix, idx, system_names, system_components ) .unwrap(); - if !conflicts.is_empty() { - let names = conflicts - .iter() - .map(|id| world.components().get_info(*id).unwrap().name()) - .collect::>(); - writeln!(string, " conflicts: {:?}", names).unwrap(); - } } } - let parallel = find_ambiguities(&self.parallel); - let at_start = find_ambiguities(&self.exclusive_at_start); - let before_commands = find_ambiguities(&self.exclusive_before_commands); - let at_end = find_ambiguities(&self.exclusive_at_end); + + let parallel = find_ambiguities(&self.parallel, &ambiguity_report.ignore_crates); + let at_start = find_ambiguities(&self.exclusive_at_start, &ambiguity_report.ignore_crates); + let before_commands = find_ambiguities( + &self.exclusive_before_commands, + &ambiguity_report.ignore_crates, + ); + let at_end = find_ambiguities(&self.exclusive_at_end, &ambiguity_report.ignore_crates); + + let mut unresolved_count = parallel.len(); + unresolved_count += at_start.len(); + unresolved_count += before_commands.len(); + unresolved_count += at_end.len(); + + if unresolved_count > 0 { + let mut details = String::from(""); + + if ambiguity_report.level != AmbiguityReportLevel::Verbose { + write!(details, " Set the level of the ReportExecutionOrderAmbiguities resource to AmbiguityReportLevel::Verbose for more details.").unwrap(); + } + + warn!( + "{} unresolved ambiguities detected.{}", + unresolved_count, &details + ); + } + if !(parallel.is_empty() && at_start.is_empty() && before_commands.is_empty() && at_end.is_empty()) + && ambiguity_report.level == AmbiguityReportLevel::Verbose { let mut string = "Execution order ambiguities detected, you might want to \ add an explicit dependency relation between some of these systems:\n" .to_owned(); if !parallel.is_empty() { - writeln!(string, " * Parallel systems:").unwrap(); - write_display_names_of_pairs(&mut string, &self.parallel, parallel, world); + writeln!(string, "1. Parallel systems:").unwrap(); + write_display_names_of_pairs(&mut string, &self.parallel, parallel, world, "1"); } if !at_start.is_empty() { - writeln!(string, " * Exclusive systems at start of stage:").unwrap(); + writeln!(string, "2. Exclusive systems at start of stage:").unwrap(); write_display_names_of_pairs( &mut string, &self.exclusive_at_start, at_start, world, + "2", ); } if !before_commands.is_empty() { - writeln!(string, " * Exclusive systems before commands of stage:").unwrap(); + writeln!(string, "3. Exclusive systems before commands of stage:").unwrap(); write_display_names_of_pairs( &mut string, &self.exclusive_before_commands, before_commands, world, + "3", ); } if !at_end.is_empty() { - writeln!(string, " * Exclusive systems at end of stage:").unwrap(); - write_display_names_of_pairs(&mut string, &self.exclusive_at_end, at_end, world); + writeln!(string, "4. Exclusive systems at end of stage:").unwrap(); + write_display_names_of_pairs( + &mut string, + &self.exclusive_at_end, + at_end, + world, + "4", + ); } info!("{}", string); } @@ -664,7 +798,35 @@ fn process_systems( /// Returns vector containing all pairs of indices of systems with ambiguous execution order, /// along with specific components that have triggered the warning. /// Systems must be topologically sorted beforehand. -fn find_ambiguities(systems: &[impl SystemContainer]) -> Vec<(usize, usize, Vec)> { +fn find_ambiguities( + systems: &[impl SystemContainer], + crates_filter: &[String], +) -> Vec<(usize, usize, Vec)> { + fn should_ignore_ambiguity( + systems: &[impl SystemContainer], + index_a: usize, + index_b: usize, + crates_filter: &[String], + ) -> bool { + let system_a = &systems[index_a]; + let system_b = &systems[index_b]; + + (match system_a.ambiguity_detection() { + AmbiguityDetection::Ignore => true, + AmbiguityDetection::Check => false, + AmbiguityDetection::IgnoreWithLabel(labels) => { + labels.iter().any(|l| system_b.labels().contains(l)) + } + }) || (match system_b.ambiguity_detection() { + AmbiguityDetection::Ignore => true, + AmbiguityDetection::Check => false, + AmbiguityDetection::IgnoreWithLabel(labels) => { + labels.iter().any(|l| system_a.labels().contains(l)) + } + }) || (crates_filter.iter().any(|s| system_a.name().starts_with(s)) + && crates_filter.iter().any(|s| system_b.name().starts_with(s))) + } + let mut ambiguity_set_labels = HashMap::default(); for set in systems.iter().flat_map(|c| c.ambiguity_sets()) { let len = ambiguity_set_labels.len(); @@ -719,6 +881,7 @@ fn find_ambiguities(systems: &[impl SystemContainer]) -> Vec<(usize, usize, Vec< { if !processed.contains(index_b) && all_ambiguity_sets[index_a].is_disjoint(&all_ambiguity_sets[index_b]) + && !should_ignore_ambiguity(systems, index_a, index_b, crates_filter) { let a_access = systems[index_a].component_access(); let b_access = systems[index_b].component_access(); @@ -754,9 +917,7 @@ impl Stage for SystemStage { self.systems_modified = false; self.executor.rebuild_cached_data(&self.parallel); self.executor_modified = false; - if world.contains_resource::() { - self.report_ambiguities(world); - } + self.report_ambiguities(world); } else if self.executor_modified { self.executor.rebuild_cached_data(&self.parallel); self.executor_modified = false; @@ -1559,7 +1720,24 @@ mod tests { fn find_ambiguities_first_labels( systems: &[impl SystemContainer], ) -> Vec<(BoxedSystemLabel, BoxedSystemLabel)> { - find_ambiguities(systems) + find_ambiguities(systems, &[]) + .drain(..) + .map(|(index_a, index_b, _conflicts)| { + ( + systems[index_a].labels()[0].clone(), + systems[index_b].labels()[0].clone(), + ) + }) + .collect() + } + + fn find_ambiguities_first_labels_with_filter( + systems: &[impl SystemContainer], + filter: &[String], + ) -> Vec<(BoxedSystemLabel, BoxedSystemLabel)> { + let mut find_ambiguities = find_ambiguities(systems, filter); + + find_ambiguities .drain(..) .map(|(index_a, index_b, _conflicts)| { ( @@ -1574,6 +1752,10 @@ mod tests { fn resource(_: ResMut) {} fn component(_: Query<&mut f32>) {} + mod inner { + pub(super) fn inner_fn(_: super::ResMut) {} + } + let mut world = World::new(); let mut stage = SystemStage::parallel() @@ -1584,7 +1766,7 @@ mod tests { .with_system(empty.label("4")); stage.initialize_systems(&mut world); stage.rebuild_orders_and_dependencies(); - assert_eq!(find_ambiguities(&stage.parallel).len(), 0); + assert_eq!(find_ambiguities(&stage.parallel, &[]).len(), 0); let mut stage = SystemStage::parallel() .with_system(empty.label("0")) @@ -1624,7 +1806,7 @@ mod tests { .with_system(component.label("4")); stage.initialize_systems(&mut world); stage.rebuild_orders_and_dependencies(); - assert_eq!(find_ambiguities(&stage.parallel).len(), 0); + assert_eq!(find_ambiguities(&stage.parallel, &[]).len(), 0); let mut stage = SystemStage::parallel() .with_system(component.label("0")) @@ -1850,7 +2032,7 @@ mod tests { .with_system(empty.exclusive_system().label("7").after("6")); stage.initialize_systems(&mut world); stage.rebuild_orders_and_dependencies(); - assert_eq!(find_ambiguities(&stage.exclusive_at_start).len(), 0); + assert_eq!(find_ambiguities(&stage.exclusive_at_start, &[]).len(), 0); let mut stage = SystemStage::parallel() .with_system(empty.exclusive_system().label("0").before("1").before("3")) @@ -1927,6 +2109,96 @@ mod tests { stage.rebuild_orders_and_dependencies(); let ambiguities = find_ambiguities_first_labels(&stage.exclusive_at_start); assert_eq!(ambiguities.len(), 0); + + let mut stage = SystemStage::parallel() + .with_system(component.label("0")) + .with_system(resource.label("1").after("0").silence_ambiguity_checks()) + .with_system(empty.label("2")) + .with_system(component.label("3").after("2").before("4")) + .with_system(resource.label("4")); + stage.initialize_systems(&mut world); + stage.rebuild_orders_and_dependencies(); + let ambiguities = find_ambiguities_first_labels(&stage.parallel); + assert!( + ambiguities.contains(&(Box::new("0"), Box::new("3"))) + || ambiguities.contains(&(Box::new("3"), Box::new("0"))) + ); + assert_eq!(ambiguities.len(), 1); + + let mut stage = SystemStage::parallel() + .with_system(component.label("0").silence_ambiguity_checks()) + .with_system(resource.label("1").after("0").silence_ambiguity_checks()) + .with_system(empty.label("2")) + .with_system(component.label("3").after("2").before("4")) + .with_system(resource.label("4")); + stage.initialize_systems(&mut world); + stage.rebuild_orders_and_dependencies(); + let ambiguities = find_ambiguities_first_labels(&stage.parallel); + assert_eq!(ambiguities.len(), 0); + + let mut stage = SystemStage::parallel() + .with_system(empty.exclusive_system().label("0").before("1").before("3")) + .with_system( + empty + .exclusive_system() + .label("1") + .silence_ambiguity_checks(), + ) + .with_system(empty.exclusive_system().label("2").after("1")) + .with_system( + empty + .exclusive_system() + .label("3") + .silence_ambiguity_checks(), + ) + .with_system(empty.exclusive_system().label("4").after("3").before("5")) + .with_system( + empty + .exclusive_system() + .label("5") + .silence_ambiguity_checks(), + ) + .with_system(empty.exclusive_system().label("6").after("2").after("5")); + stage.initialize_systems(&mut world); + stage.rebuild_orders_and_dependencies(); + let ambiguities = find_ambiguities_first_labels(&stage.exclusive_at_start); + assert!( + ambiguities.contains(&(Box::new("2"), Box::new("4"))) + || ambiguities.contains(&(Box::new("4"), Box::new("2"))) + ); + assert_eq!(ambiguities.len(), 1); + + let mut stage = SystemStage::parallel() + .with_system(component.label("0")) + .with_system(resource.label("1").after("0")) + .with_system(empty.label("2")) + .with_system( + component + .label("3") + .after("2") + .before("4") + .ambiguous_with("0"), + ) + .with_system(resource.label("4").ambiguous_with("4")); + stage.initialize_systems(&mut world); + stage.rebuild_orders_and_dependencies(); + let ambiguities = find_ambiguities_first_labels(&stage.parallel); + assert!( + ambiguities.contains(&(Box::new("1"), Box::new("4"))) + || ambiguities.contains(&(Box::new("4"), Box::new("1"))) + ); + assert_eq!(ambiguities.len(), 1); + + let mut stage = SystemStage::parallel() + .with_system(resource.label("1")) + .with_system(inner::inner_fn.label("2")); + stage.initialize_systems(&mut world); + stage.rebuild_orders_and_dependencies(); + let ambiguities = find_ambiguities_first_labels_with_filter( + &stage.parallel, + &["bevy_ecs::schedule::stage::tests::ambiguity_detection::inner::".into()], + ); + assert_eq!(ambiguities.len(), 1); } #[test] diff --git a/crates/bevy_ecs/src/schedule/system_container.rs b/crates/bevy_ecs/src/schedule/system_container.rs index 68e493da714b5..6ca8c7cf4296e 100644 --- a/crates/bevy_ecs/src/schedule/system_container.rs +++ b/crates/bevy_ecs/src/schedule/system_container.rs @@ -9,6 +9,8 @@ use crate::{ }; use std::{borrow::Cow, cell::UnsafeCell}; +use super::AmbiguityDetection; + /// System metadata like its name, labels, order requirements and component access. pub trait SystemContainer: GraphNode