diff --git a/Cargo.toml b/Cargo.toml index 3f5c13b5109b7..c5193c920d8ac 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -339,6 +339,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" @@ -544,6 +548,10 @@ path = "examples/stress_tests/transform_hierarchy.rs" # Tools +[[example]] +name = "internal_ambiguities" +path = "examples/tools/internal_ambiguities.rs" + [[example]] name = "scene_viewer" path = "examples/tools/scene_viewer.rs" diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index e2f62cca07a7c..d5f43a9252814 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -28,9 +28,10 @@ pub mod prelude { event::{EventReader, EventWriter}, query::{Added, AnyOf, ChangeTrackers, Changed, Or, QueryState, With, Without}, schedule::{ - AmbiguitySetLabel, ExclusiveSystemDescriptorCoercion, ParallelSystemDescriptorCoercion, - RunCriteria, RunCriteriaDescriptorCoercion, RunCriteriaLabel, Schedule, Stage, - StageLabel, State, SystemLabel, SystemSet, SystemStage, + AmbiguitySetLabel, ExclusiveSystemDescriptorCoercion, ExecutionOrderAmbiguities, + ParallelSystemDescriptorCoercion, RunCriteria, RunCriteriaDescriptorCoercion, + RunCriteriaLabel, Schedule, Stage, StageLabel, State, SystemLabel, SystemSet, + SystemStage, }, system::{ Commands, In, IntoChainSystem, IntoExclusiveSystem, IntoSystem, Local, NonSend, diff --git a/crates/bevy_ecs/src/schedule/ambiguity_detection.rs b/crates/bevy_ecs/src/schedule/ambiguity_detection.rs new file mode 100644 index 0000000000000..cd9f14bac01c7 --- /dev/null +++ b/crates/bevy_ecs/src/schedule/ambiguity_detection.rs @@ -0,0 +1,809 @@ +use crate::component::ComponentId; +use crate::schedule::{AmbiguityDetection, SystemContainer, SystemStage}; +use crate::world::World; + +use bevy_utils::get_short_name; +use bevy_utils::tracing::{error, warn}; +use fixedbitset::FixedBitSet; +use std::hash::Hash; + +/// Within a stage, writes to a component or resource (and reads relative to writes) should typically have a well-defined order. +/// +/// Ambiguous order between systems that have conflicting access can result in subtle logic bugs since, +/// in the absence of constraints, systems are executed in an arbitrary order and that can change between runs +/// and even frames. +/// +/// Some ambiguities reported by the ambiguity checker may be warranted (to allow two systems to run +/// without blocking each other) or spurious, as the exact combination of archetypes used may +/// prevent them from ever conflicting during actual gameplay. +/// You can resolve the warnings produced by the ambiguity checker by adding `.before` or `.after` +/// to one of the conflicting systems referencing the other system to force a specific ordering. +/// Alternatively, if you're confident the error is a false positive (and you don't need true determinism), +/// you can explicitly ignore it using the `.ambiguous_with` method. +/// +/// Note that the checker reports each pairwise ambiguity: +/// typically, these can be resolved with fewer constraints than the number of ambiguities listed +/// as transitive orderings will resolve ambiguities (e.g. A before B before C will resolve an ambiguity between A and C). +/// It's generally more productive to pause and reflect on the underlying structure of your logic, +/// rather than blindly resolving one warning at a time. +/// +/// By default, the value of this resource is set to `Warn`. +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub enum ExecutionOrderAmbiguities { + /// Disables all checks for execution order ambiguities + Allow, + /// Displays only the number of unresolved ambiguities detected by the ambiguity checker + Warn, + /// Displays a full report of ambiguities detected by the ambiguity checker + WarnVerbose, + /// Verbosely reports all non-ignored ambiguities, including those between Bevy's systems + /// + /// These will not be actionable: you should only turn on this functionality when + /// investigating to see if there's a Bevy bug or working on the engine itself. + WarnInternal, + /// Like `WarnVerbose`, but panics if any non-ignored ambiguities exist + Deny, + /// Verbosely reports ALL ambiguities, even ignored ones + /// + /// Panics if any ambiguities exist. + /// + /// This will be very noisy, but can be useful when attempting to track down subtle determinism issues, + /// as you might need when attempting to implement deterministic networking. + Forbid, +} + +/// A pair of systems that have conflicting access and an ambiguous execution order. +/// +/// Created by applying [`find_ambiguities`] to a [`SystemContainer`]. +/// These can be reported by configuring the [`ExecutionOrderAmbiguities`] resource. +#[derive(Debug, Clone, Eq)] +pub struct SystemOrderAmbiguity { + // The names of the conflicting systems + pub system_names: [String; 2], + /// The components (and resources) that these systems have incompatible access to + pub conflicts: Vec, + /// The segment of the [`SystemStage`] that the conflicting systems were stored in + pub segment: SystemStageSegment, +} + +impl PartialEq for SystemOrderAmbiguity { + fn eq(&self, other: &Self) -> bool { + let mut self_names = self.system_names.clone(); + self_names.sort(); + + let mut other_names = self.system_names.clone(); + other_names.sort(); + + let mut self_conflicts = self.conflicts.clone(); + self_conflicts.sort(); + + let mut other_conflicts = self.conflicts.clone(); + other_conflicts.sort(); + + (self_names == other_names) + && (self_conflicts == other_conflicts) + && (self.segment == other.segment) + } +} + +// This impl is needed to allow us to test whether a returned set of ambiguities +// matches the expected value +impl Hash for SystemOrderAmbiguity { + fn hash(&self, state: &mut H) { + // The order of the systems doesn't matter + let mut system_names = self.system_names.clone(); + system_names.sort(); + system_names.hash(state); + // The order of the reported conflicts doesn't matter + let mut conflicts = self.conflicts.clone(); + conflicts.sort(); + conflicts.hash(state); + self.segment.hash(state); + } +} + +/// Which part of a [`SystemStage`] was a [`SystemOrderAmbiguity`] detected in? +#[derive(Debug, PartialEq, Eq, Clone, Copy, Hash)] +pub enum SystemStageSegment { + Parallel, + ExclusiveAtStart, + ExclusiveBeforeCommands, + ExclusiveAtEnd, +} + +impl SystemOrderAmbiguity { + fn from_raw( + system_a_index: usize, + system_b_index: usize, + component_ids: Vec, + segment: SystemStageSegment, + stage: &SystemStage, + world: &World, + ) -> Self { + use crate::schedule::graph_utils::GraphNode; + use SystemStageSegment::*; + + // TODO: blocked on https://github.com/bevyengine/bevy/pull/4166 + // We can't grab the system container generically, because .parallel_systems() + // and the exclusive equivalent return a different type, + // and SystemContainer is not object-safe + let (system_a_name, system_b_name) = match segment { + Parallel => { + let system_container = stage.parallel_systems(); + ( + system_container[system_a_index].name(), + system_container[system_b_index].name(), + ) + } + ExclusiveAtStart => { + let system_container = stage.exclusive_at_start_systems(); + ( + system_container[system_a_index].name(), + system_container[system_b_index].name(), + ) + } + ExclusiveBeforeCommands => { + let system_container = stage.exclusive_before_commands_systems(); + ( + system_container[system_a_index].name(), + system_container[system_b_index].name(), + ) + } + ExclusiveAtEnd => { + let system_container = stage.exclusive_at_end_systems(); + ( + system_container[system_a_index].name(), + system_container[system_b_index].name(), + ) + } + }; + + let conflicts: Vec = component_ids + .iter() + .map(|id| world.components().get_info(*id).unwrap().name().into()) + .collect(); + + Self { + // Don't bother with Cows here + system_names: [system_a_name.into(), system_b_name.into()], + conflicts, + segment, + } + } +} + +/// 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. +pub fn find_ambiguities( + systems: &[impl SystemContainer], + crates_filter: &[String], + // Should explicit attempts to ignore ambiguities be obeyed? + report_level: ExecutionOrderAmbiguities, +) -> Vec<(usize, usize, Vec)> { + fn should_ignore_ambiguity( + systems: &[impl SystemContainer], + index_a: usize, + index_b: usize, + crates_filter: &[String], + report_level: ExecutionOrderAmbiguities, + ) -> bool { + if report_level == ExecutionOrderAmbiguities::Forbid { + return false; + } + + 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 all_dependencies = Vec::::with_capacity(systems.len()); + let mut all_dependants = Vec::::with_capacity(systems.len()); + for (index, container) in systems.iter().enumerate() { + let mut dependencies = FixedBitSet::with_capacity(systems.len()); + for &dependency in container.dependencies() { + dependencies.union_with(&all_dependencies[dependency]); + dependencies.insert(dependency); + all_dependants[dependency].insert(index); + } + + all_dependants.push(FixedBitSet::with_capacity(systems.len())); + all_dependencies.push(dependencies); + } + for index in (0..systems.len()).rev() { + let mut dependants = FixedBitSet::with_capacity(systems.len()); + for dependant in all_dependants[index].ones() { + dependants.union_with(&all_dependants[dependant]); + dependants.insert(dependant); + } + all_dependants[index] = dependants; + } + let mut all_relations = all_dependencies + .drain(..) + .zip(all_dependants.drain(..)) + .enumerate() + .map(|(index, (dependencies, dependants))| { + let mut relations = FixedBitSet::with_capacity(systems.len()); + relations.union_with(&dependencies); + relations.union_with(&dependants); + relations.insert(index); + relations + }) + .collect::>(); + let mut ambiguities = Vec::new(); + let full_bitset: FixedBitSet = (0..systems.len()).collect(); + let mut processed = FixedBitSet::with_capacity(systems.len()); + for (index_a, relations) in all_relations.drain(..).enumerate() { + for index_b in full_bitset.difference(&relations) { + if !processed.contains(index_b) + && !should_ignore_ambiguity(systems, index_a, index_b, crates_filter, report_level) + { + let a_access = systems[index_a].component_access(); + let b_access = systems[index_b].component_access(); + if let (Some(a), Some(b)) = (a_access, b_access) { + let component_ids = a.get_conflicts(b); + if !component_ids.is_empty() { + ambiguities.push((index_a, index_b, component_ids)); + } + } else { + // The ambiguity is for an exclusive system, + // which conflict on all data + ambiguities.push((index_a, index_b, Vec::default())); + } + } + } + processed.insert(index_a); + } + ambiguities +} + +impl Default for ExecutionOrderAmbiguities { + fn default() -> Self { + ExecutionOrderAmbiguities::Warn + } +} + +impl SystemStage { + /// Returns all execution order ambiguities between systems + /// + /// Returns 4 vectors of ambiguities for each stage, in the following order: + /// - parallel + /// - exclusive at start, + /// - exclusive before commands + /// - exclusive at end + pub fn ambiguities( + &mut self, + // FIXME: these methods should not have tor rely on &mut World, or any specific World + // see https://github.com/bevyengine/bevy/issues/4364 + world: &mut World, + report_level: ExecutionOrderAmbiguities, + ) -> Vec { + self.initialize(world); + + if report_level == ExecutionOrderAmbiguities::Allow { + return Vec::default(); + } + + // System order must be fresh + debug_assert!(!self.systems_modified); + + // TODO: remove all internal ambiguities and remove this logic + let ignored_crates = if report_level != ExecutionOrderAmbiguities::WarnInternal { + vec![ + // Rendering + "bevy_render".to_string(), + "bevy_sprite".to_string(), + "bevy_render".to_string(), + "bevy_pbr".to_string(), + "bevy_text".to_string(), + "bevy_core_pipeline".to_string(), + "bevy_ui".to_string(), + "bevy_hierarchy".to_string(), + "bevy_animation".to_string(), + // Misc + "bevy_winit".to_string(), + "bevy_audio".to_string(), + ] + } else { + Vec::default() + }; + + let parallel: Vec = + find_ambiguities(&self.parallel, &ignored_crates, report_level) + .iter() + .map(|(system_a_index, system_b_index, component_ids)| { + SystemOrderAmbiguity::from_raw( + *system_a_index, + *system_b_index, + component_ids.to_vec(), + SystemStageSegment::Parallel, + self, + world, + ) + }) + .collect(); + + let at_start: Vec = + find_ambiguities(&self.exclusive_at_start, &ignored_crates, report_level) + .iter() + .map(|(system_a_index, system_b_index, component_ids)| { + SystemOrderAmbiguity::from_raw( + *system_a_index, + *system_b_index, + component_ids.to_vec(), + SystemStageSegment::ExclusiveAtStart, + self, + world, + ) + }) + .collect(); + + let before_commands: Vec = find_ambiguities( + &self.exclusive_before_commands, + &ignored_crates, + report_level, + ) + .iter() + .map(|(system_a_index, system_b_index, component_ids)| { + SystemOrderAmbiguity::from_raw( + *system_a_index, + *system_b_index, + component_ids.to_vec(), + SystemStageSegment::ExclusiveBeforeCommands, + self, + world, + ) + }) + .collect(); + + let at_end: Vec = + find_ambiguities(&self.exclusive_at_end, &ignored_crates, report_level) + .iter() + .map(|(system_a_index, system_b_index, component_ids)| { + SystemOrderAmbiguity::from_raw( + *system_a_index, + *system_b_index, + component_ids.to_vec(), + SystemStageSegment::ExclusiveAtEnd, + self, + world, + ) + }) + .collect(); + + let mut ambiguities = Vec::default(); + ambiguities.extend(at_start); + ambiguities.extend(parallel); + ambiguities.extend(before_commands); + ambiguities.extend(at_end); + + ambiguities + } + + /// Returns the number of system order ambiguities between systems in this stage + pub fn n_ambiguities( + &mut self, + world: &mut World, + report_level: ExecutionOrderAmbiguities, + ) -> usize { + self.ambiguities(world, report_level).len() + } + + /// Reports all execution order ambiguities between systems + pub fn report_ambiguities( + &mut self, + world: &mut World, + report_level: ExecutionOrderAmbiguities, + ) { + let ambiguities = self.ambiguities(world, report_level); + let unresolved_count = ambiguities.len(); + + let mut warning_string = String::new(); + + if unresolved_count > 0 { + let plural = if unresolved_count == 1 { "" } else { "s" }; + warning_string += &format!("One of your stages contains {unresolved_count} pair{plural} of systems with unknown order and conflicting data access. \n\ + You may want to add `.before()` or `.after()` ordering constraints between some of these systems to prevent bugs.\n"); + + if report_level == ExecutionOrderAmbiguities::Warn { + warning_string += "Set the level of the `ExecutionOrderAmbiguities` resource to `ExecutionOrderAmbiguities::WarnVerbose` for more details."; + } else { + for (i, ambiguity) in ambiguities.iter().enumerate() { + let ambiguity_number = i + 1; + // The path name is often just noise, and this gets us consistency with `conflicts`'s formatting + let system_a_name = get_short_name(ambiguity.system_names[0].as_str()); + let system_b_name = get_short_name(ambiguity.system_names[1].as_str()); + let mut conflicts: Vec = ambiguity + .conflicts + .iter() + .map(|name| get_short_name(name.as_str())) + .collect(); + + // Exclusive system conflicts are reported as conflicting on the empty set + if conflicts.is_empty() { + conflicts = vec!["World".to_string()]; + } + + warning_string += &format!("\n{ambiguity_number:?}. `{system_a_name}` conflicts with `{system_b_name}` on {conflicts:?}"); + } + // Print an empty line to space out multiple stages nicely + warning_string.push('\n'); + } + + if report_level == ExecutionOrderAmbiguities::Deny + || report_level == ExecutionOrderAmbiguities::Forbid + { + error!("{warning_string}"); + panic!("`ExecutionOrderAmbiguities` is set to `{report_level:?}`, which forbids the `App` from running if any two systems with incompatible data access have ambiguous execution order (unless permitted).") + } else { + warn!("{warning_string}"); + } + } + } +} + +// Systems and TestResource are used in tests +#[allow(dead_code)] +#[cfg(test)] +mod tests { + // Required to make the derive macro behave + use crate as bevy_ecs; + use crate::event::Events; + use crate::prelude::*; + + struct R; + + #[derive(Component)] + struct A; + + #[derive(Component)] + struct B; + + // An event type + struct E; + + fn empty_system() {} + fn res_system(_res: Res) {} + fn resmut_system(_res: ResMut) {} + fn nonsend_system(_ns: NonSend) {} + fn nonsendmut_system(_ns: NonSendMut) {} + fn read_component_system(_query: Query<&A>) {} + fn write_component_system(_query: Query<&mut A>) {} + fn with_filtered_component_system(_query: Query<&mut A, With>) {} + fn without_filtered_component_system(_query: Query<&mut A, Without>) {} + fn event_reader_system(_reader: EventReader) {} + fn event_writer_system(_writer: EventWriter) {} + fn event_resource_system(_events: ResMut>) {} + fn read_world_system(_world: &World) {} + fn write_world_system(_world: &mut World) {} + + // Tests for conflict detection + #[test] + fn one_of_everything() { + let mut world = World::new(); + let mut test_stage = SystemStage::parallel(); + + test_stage + .add_system(empty_system) + // nonsendmut system deliberately conflicts with resmut system + .add_system(resmut_system) + .add_system(write_component_system) + .add_system(event_writer_system); + + assert_eq!( + test_stage.n_ambiguities(&mut world, ExecutionOrderAmbiguities::Forbid), + 0 + ); + } + + #[test] + fn read_only() { + let mut world = World::new(); + let mut test_stage = SystemStage::parallel(); + + test_stage + .add_system(empty_system) + .add_system(empty_system) + .add_system(res_system) + .add_system(res_system) + .add_system(nonsend_system) + .add_system(nonsend_system) + .add_system(read_component_system) + .add_system(read_component_system) + .add_system(event_reader_system) + .add_system(event_reader_system) + .add_system(read_world_system) + .add_system(read_world_system); + + assert_eq!( + test_stage.n_ambiguities(&mut world, ExecutionOrderAmbiguities::Forbid), + 0 + ); + } + + #[test] + fn read_world() { + let mut world = World::new(); + let mut test_stage = SystemStage::parallel(); + + test_stage + .add_system(empty_system) + .add_system(resmut_system) + .add_system(write_component_system) + .add_system(event_writer_system) + .add_system(read_world_system); + + assert_eq!( + test_stage.n_ambiguities(&mut world, ExecutionOrderAmbiguities::Forbid), + 3 + ); + } + + #[test] + fn resources() { + let mut world = World::new(); + let mut test_stage = SystemStage::parallel(); + + test_stage.add_system(resmut_system).add_system(res_system); + + assert_eq!( + test_stage.n_ambiguities(&mut world, ExecutionOrderAmbiguities::Forbid), + 1 + ); + } + + #[test] + fn nonsend() { + let mut world = World::new(); + let mut test_stage = SystemStage::parallel(); + + test_stage + .add_system(nonsendmut_system) + .add_system(nonsend_system); + + assert_eq!( + test_stage.n_ambiguities(&mut world, ExecutionOrderAmbiguities::Forbid), + 1 + ); + } + + #[test] + fn components() { + let mut world = World::new(); + let mut test_stage = SystemStage::parallel(); + + test_stage + .add_system(read_component_system) + .add_system(write_component_system); + + assert_eq!( + test_stage.n_ambiguities(&mut world, ExecutionOrderAmbiguities::Forbid), + 1 + ); + } + + #[test] + #[ignore = "Known failing but fix is non-trivial: https://github.com/bevyengine/bevy/issues/4381"] + fn filtered_components() { + let mut world = World::new(); + let mut test_stage = SystemStage::parallel(); + + test_stage + .add_system(with_filtered_component_system) + .add_system(without_filtered_component_system); + + assert_eq!( + test_stage.n_ambiguities(&mut world, ExecutionOrderAmbiguities::Forbid), + 0 + ); + } + + #[test] + fn events() { + let mut world = World::new(); + let mut test_stage = SystemStage::parallel(); + + test_stage + // All of these systems clash + .add_system(event_reader_system) + .add_system(event_writer_system) + .add_system(event_resource_system); + + assert_eq!( + test_stage.n_ambiguities(&mut world, ExecutionOrderAmbiguities::Forbid), + 3 + ); + } + + #[test] + fn exclusive() { + let mut world = World::new(); + let mut test_stage = SystemStage::parallel(); + + test_stage + // All 3 of these conflict with each other + .add_system(write_world_system.exclusive_system()) + .add_system(write_world_system.exclusive_system().at_end()) + .add_system(res_system.exclusive_system()) + // These do not, as they're in different segments of the stage + .add_system(write_world_system.exclusive_system().at_start()) + .add_system(write_world_system.exclusive_system().before_commands()); + + assert_eq!( + test_stage.n_ambiguities(&mut world, ExecutionOrderAmbiguities::Forbid), + 3 + ); + } + + // Tests for silencing and resolving ambiguities + + #[test] + fn before_and_after() { + let mut world = World::new(); + let mut test_stage = SystemStage::parallel(); + + test_stage + .add_system(event_reader_system.before(event_writer_system)) + .add_system(event_writer_system) + .add_system(event_resource_system.after(event_writer_system)); + + assert_eq!( + test_stage.n_ambiguities(&mut world, ExecutionOrderAmbiguities::Forbid), + // All of these systems clash + 0 + ); + } + + #[test] + fn ignore_all_ambiguities() { + let mut world = World::new(); + let mut test_stage = SystemStage::parallel(); + test_stage + .add_system(resmut_system.ignore_all_ambiguities()) + .add_system(res_system); + + assert_eq!( + test_stage.n_ambiguities(&mut world, ExecutionOrderAmbiguities::Warn), + 0 + ); + } + + #[test] + fn ambiguous_with_label() { + let mut world = World::new(); + let mut test_stage = SystemStage::parallel(); + test_stage + .add_system(resmut_system.ambiguous_with("IGNORE_ME")) + .add_system(res_system.label("IGNORE_ME")); + + assert_eq!( + test_stage.n_ambiguities(&mut world, ExecutionOrderAmbiguities::Warn), + 0 + ); + } + + #[test] + fn ambiguous_with_system() { + let mut world = World::new(); + let mut test_stage = SystemStage::parallel(); + test_stage + .add_system(system_a.ambiguous_with(system_b)) + .add_system(system_b); + + assert_eq!( + test_stage.n_ambiguities(&mut world, ExecutionOrderAmbiguities::Warn), + 0 + ); + } + + // Tests for reporting levels + + fn system_a(_res: ResMut) {} + fn system_b(_res: ResMut) {} + fn system_c(_res: ResMut) {} + fn system_d(_res: ResMut) {} + + fn make_test_stage(world: &mut World) -> SystemStage { + let mut test_stage = SystemStage::parallel(); + world.insert_resource(R); + + test_stage + // Ambiguous with B and D + .add_system(system_a) + // Ambiguous with A + .add_system(system_b.label("b")) + .add_system(system_c.ignore_all_ambiguities()) + // Ambiguous with A + .add_system(system_d.ambiguous_with("b")); + + test_stage + } + + #[test] + fn allow() { + let mut world = World::new(); + let mut test_stage = make_test_stage(&mut world); + assert_eq!( + test_stage.n_ambiguities(&mut world, ExecutionOrderAmbiguities::Allow), + 0 + ); + } + + #[test] + fn warn() { + let mut world = World::new(); + let mut test_stage = make_test_stage(&mut world); + assert_eq!( + test_stage.n_ambiguities(&mut world, ExecutionOrderAmbiguities::Warn), + 2 + ); + } + + #[test] + fn warn_verbose() { + let mut world = World::new(); + let mut test_stage = make_test_stage(&mut world); + assert_eq!( + test_stage.n_ambiguities(&mut world, ExecutionOrderAmbiguities::WarnVerbose), + 2 + ); + } + + #[test] + fn forbid() { + let mut world = World::new(); + let mut test_stage = make_test_stage(&mut world); + assert_eq!( + test_stage.n_ambiguities(&mut world, ExecutionOrderAmbiguities::Forbid), + 6 + ); + } + + // Tests that the correct ambiguities were reported + #[test] + fn correct_ambiguities() { + use crate::schedule::SystemOrderAmbiguity; + use bevy_utils::HashSet; + + let mut world = World::new(); + let mut test_stage = make_test_stage(&mut world); + let ambiguities = + test_stage.ambiguities(&mut world, ExecutionOrderAmbiguities::WarnVerbose); + assert_eq!( + // We don't care if the reported order varies + HashSet::from_iter(ambiguities), + HashSet::from_iter(vec![ + SystemOrderAmbiguity { + system_names: [ + "bevy_ecs::schedule::ambiguity_detection::tests::system_a".to_string(), + "bevy_ecs::schedule::ambiguity_detection::tests::system_b".to_string() + ], + conflicts: vec!["bevy_ecs::schedule::ambiguity_detection::tests::R".to_string()], + segment: bevy_ecs::schedule::SystemStageSegment::Parallel, + }, + SystemOrderAmbiguity { + system_names: [ + "bevy_ecs::schedule::ambiguity_detection::tests::system_a".to_string(), + "bevy_ecs::schedule::ambiguity_detection::tests::system_d".to_string() + ], + conflicts: vec!["bevy_ecs::schedule::ambiguity_detection::tests::R".to_string()], + segment: bevy_ecs::schedule::SystemStageSegment::Parallel, + }, + ],) + ); + } +} diff --git a/crates/bevy_ecs/src/schedule/label.rs b/crates/bevy_ecs/src/schedule/label.rs index 5235dc5837692..f539f89c44a85 100644 --- a/crates/bevy_ecs/src/schedule/label.rs +++ b/crates/bevy_ecs/src/schedule/label.rs @@ -3,10 +3,8 @@ use bevy_utils::define_label; define_label!(StageLabel); define_label!(SystemLabel); -define_label!(AmbiguitySetLabel); define_label!(RunCriteriaLabel); pub(crate) type BoxedStageLabel = Box; pub(crate) type BoxedSystemLabel = Box; -pub(crate) type BoxedAmbiguitySetLabel = Box; pub(crate) type BoxedRunCriteriaLabel = Box; diff --git a/crates/bevy_ecs/src/schedule/mod.rs b/crates/bevy_ecs/src/schedule/mod.rs index dc228f4265de3..dc15f631ebd45 100644 --- a/crates/bevy_ecs/src/schedule/mod.rs +++ b/crates/bevy_ecs/src/schedule/mod.rs @@ -3,6 +3,7 @@ //! When using Bevy ECS, systems are usually not run directly, but are inserted into a //! [`Stage`], which then lives within a [`Schedule`]. +mod ambiguity_detection; mod executor; mod executor_parallel; pub mod graph_utils; @@ -14,6 +15,7 @@ mod system_container; mod system_descriptor; mod system_set; +pub use ambiguity_detection::*; pub use executor::*; pub use executor_parallel::*; pub use graph_utils::GraphNode; diff --git a/crates/bevy_ecs/src/schedule/stage.rs b/crates/bevy_ecs/src/schedule/stage.rs index c2a040d648517..ea98511e95fbf 100644 --- a/crates/bevy_ecs/src/schedule/stage.rs +++ b/crates/bevy_ecs/src/schedule/stage.rs @@ -1,23 +1,20 @@ use crate::{ - component::ComponentId, prelude::IntoSystem, schedule::{ graph_utils::{self, DependencyGraphError}, BoxedRunCriteria, BoxedRunCriteriaLabel, BoxedSystemLabel, DuplicateLabelStrategy, - ExclusiveSystemContainer, GraphNode, InsertionPoint, ParallelExecutor, - ParallelSystemContainer, ParallelSystemExecutor, RunCriteriaContainer, - RunCriteriaDescriptor, RunCriteriaDescriptorOrLabel, RunCriteriaInner, ShouldRun, - SingleThreadedExecutor, SystemContainer, SystemDescriptor, SystemSet, + ExclusiveSystemContainer, ExecutionOrderAmbiguities, GraphNode, InsertionPoint, + IntoSystemDescriptor, ParallelExecutor, ParallelSystemContainer, ParallelSystemExecutor, + RunCriteriaContainer, RunCriteriaDescriptor, RunCriteriaDescriptorOrLabel, + RunCriteriaInner, ShouldRun, SingleThreadedExecutor, SystemContainer, SystemDescriptor, + SystemSet, }, world::{World, WorldId}, }; -use bevy_utils::{tracing::info, HashMap, HashSet}; +use bevy_utils::{HashMap, HashSet}; use downcast_rs::{impl_downcast, Downcast}; -use fixedbitset::FixedBitSet; use std::fmt::Debug; -use super::IntoSystemDescriptor; - /// A type that can run as a step of a [`Schedule`](super::Schedule). pub trait Stage: Downcast + Send + Sync { /// Runs the stage; this happens once per update. @@ -27,27 +24,6 @@ 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. -/// -/// 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. -/// -/// This occurs because, in the absence of explicit constraints, systems are executed in -/// an unstable, arbitrary order within each stage that may vary between runs and frames. -/// -/// Some ambiguities reported by the ambiguity checker may be warranted (to allow two systems to run -/// without blocking each other) or spurious, as the exact combination of archetypes used may -/// prevent them from ever conflicting during actual gameplay. You can resolve the warnings produced -/// by the ambiguity checker by adding `.before` or `.after` to one of the conflicting systems -/// referencing the other system to force a specific ordering. -/// -/// 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; - /// Stores and executes systems. Execution order is not defined unless explicitly specified; /// see `SystemDescriptor` documentation. pub struct SystemStage { @@ -60,16 +36,16 @@ pub struct SystemStage { /// Topologically sorted run criteria of systems. run_criteria: Vec, /// Topologically sorted exclusive systems that want to be run at the start of the stage. - exclusive_at_start: Vec, + pub(super) exclusive_at_start: Vec, /// Topologically sorted exclusive systems that want to be run after parallel systems but /// before the application of their command buffers. - exclusive_before_commands: Vec, + pub(super) exclusive_before_commands: Vec, /// Topologically sorted exclusive systems that want to be run at the end of the stage. - exclusive_at_end: Vec, + pub(super) exclusive_at_end: Vec, /// Topologically sorted parallel systems. - parallel: Vec, + pub(super) parallel: Vec, /// Determines if the stage was modified and needs to rebuild its graphs and orders. - systems_modified: bool, + pub(super) systems_modified: bool, /// Determines if the stage's executor was changed. executor_modified: bool, /// Newly inserted run criteria that will be initialized at the next opportunity. @@ -123,6 +99,27 @@ impl SystemStage { Self::new(Box::new(ParallelExecutor::default())) } + /// Initializes this [`SystemStage`] on a specific world, ensuring that its internal state is valid + /// + /// This method is automatically called by [`Stage::run`], but must be manually called + /// when using [`SystemStage::ambiguities`]. + pub(crate) fn initialize(&mut self, world: &mut World) { + if self.systems_modified { + self.initialize_systems(world); + self.rebuild_orders_and_dependencies(); + self.systems_modified = false; + self.executor.rebuild_cached_data(&self.parallel); + self.executor_modified = false; + // This cannot panic, so we insert the default value if it was not present + let report_level = + *world.get_resource_or_insert_with(ExecutionOrderAmbiguities::default); + self.report_ambiguities(world, report_level); + } else if self.executor_modified { + self.executor.rebuild_cached_data(&self.parallel); + self.executor_modified = false; + } + } + pub fn get_executor(&self) -> Option<&T> { self.executor.downcast_ref() } @@ -492,75 +489,6 @@ impl SystemStage { ); } - /// Logs execution order ambiguities between systems. System orders must be fresh. - fn report_ambiguities(&self, world: &World) { - debug_assert!(!self.systems_modified); - use std::fmt::Write; - fn write_display_names_of_pairs( - string: &mut String, - systems: &[impl SystemContainer], - mut ambiguities: Vec<(usize, usize, Vec)>, - world: &World, - ) { - for (index_a, index_b, conflicts) in ambiguities.drain(..) { - writeln!( - string, - " -- {:?} and {:?}", - systems[index_a].name(), - systems[index_b].name() - ) - .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); - if !(parallel.is_empty() - && at_start.is_empty() - && before_commands.is_empty() - && at_end.is_empty()) - { - 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); - } - if !at_start.is_empty() { - writeln!(string, " * Exclusive systems at start of stage:").unwrap(); - write_display_names_of_pairs( - &mut string, - &self.exclusive_at_start, - at_start, - world, - ); - } - if !before_commands.is_empty() { - writeln!(string, " * Exclusive systems before commands of stage:").unwrap(); - write_display_names_of_pairs( - &mut string, - &self.exclusive_before_commands, - before_commands, - world, - ); - } - 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); - } - info!("{}", string); - } - } - /// Checks for old component and system change ticks fn check_change_ticks(&mut self, world: &mut World) { let change_tick = world.change_tick(); @@ -684,82 +612,6 @@ fn process_systems( Ok(()) } -/// 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)> { - let mut ambiguity_set_labels = HashMap::default(); - for set in systems.iter().flat_map(|c| c.ambiguity_sets()) { - let len = ambiguity_set_labels.len(); - ambiguity_set_labels.entry(set).or_insert(len); - } - let mut all_ambiguity_sets = Vec::::with_capacity(systems.len()); - let mut all_dependencies = Vec::::with_capacity(systems.len()); - let mut all_dependants = Vec::::with_capacity(systems.len()); - for (index, container) in systems.iter().enumerate() { - let mut ambiguity_sets = FixedBitSet::with_capacity(ambiguity_set_labels.len()); - for set in container.ambiguity_sets() { - ambiguity_sets.insert(ambiguity_set_labels[set]); - } - all_ambiguity_sets.push(ambiguity_sets); - let mut dependencies = FixedBitSet::with_capacity(systems.len()); - for &dependency in container.dependencies() { - dependencies.union_with(&all_dependencies[dependency]); - dependencies.insert(dependency); - all_dependants[dependency].insert(index); - } - - all_dependants.push(FixedBitSet::with_capacity(systems.len())); - all_dependencies.push(dependencies); - } - for index in (0..systems.len()).rev() { - let mut dependants = FixedBitSet::with_capacity(systems.len()); - for dependant in all_dependants[index].ones() { - dependants.union_with(&all_dependants[dependant]); - dependants.insert(dependant); - } - all_dependants[index] = dependants; - } - let mut all_relations = all_dependencies - .drain(..) - .zip(all_dependants.drain(..)) - .enumerate() - .map(|(index, (dependencies, dependants))| { - let mut relations = FixedBitSet::with_capacity(systems.len()); - relations.union_with(&dependencies); - relations.union_with(&dependants); - relations.insert(index); - relations - }) - .collect::>(); - let mut ambiguities = Vec::new(); - let full_bitset: FixedBitSet = (0..systems.len()).collect(); - let mut processed = FixedBitSet::with_capacity(systems.len()); - for (index_a, relations) in all_relations.drain(..).enumerate() { - // TODO: prove that `.take(index_a)` would be correct here, and uncomment it if so. - for index_b in full_bitset.difference(&relations) - // .take(index_a) - { - if !processed.contains(index_b) - && all_ambiguity_sets[index_a].is_disjoint(&all_ambiguity_sets[index_b]) - { - let a_access = systems[index_a].component_access(); - let b_access = systems[index_b].component_access(); - if let (Some(a), Some(b)) = (a_access, b_access) { - let conflicts = a.get_conflicts(b); - if !conflicts.is_empty() { - ambiguities.push((index_a, index_b, conflicts)); - } - } else { - ambiguities.push((index_a, index_b, Vec::new())); - } - } - } - processed.insert(index_a); - } - ambiguities -} - impl Stage for SystemStage { fn run(&mut self, world: &mut World) { if let Some(world_id) = self.world_id { @@ -771,19 +623,7 @@ impl Stage for SystemStage { self.world_id = Some(world.id()); } - if self.systems_modified { - self.initialize_systems(world); - self.rebuild_orders_and_dependencies(); - self.systems_modified = false; - self.executor.rebuild_cached_data(&self.parallel); - self.executor_modified = false; - if world.contains_resource::() { - self.report_ambiguities(world); - } - } else if self.executor_modified { - self.executor.rebuild_cached_data(&self.parallel); - self.executor_modified = false; - } + self.initialize(world); let mut run_stage_loop = true; while run_stage_loop { @@ -943,9 +783,9 @@ mod tests { entity::Entity, query::{ChangeTrackers, Changed}, schedule::{ - BoxedSystemLabel, ExclusiveSystemDescriptorCoercion, ParallelSystemDescriptorCoercion, - RunCriteria, RunCriteriaDescriptorCoercion, ShouldRun, SingleThreadedExecutor, Stage, - SystemSet, SystemStage, + ExclusiveSystemDescriptorCoercion, ParallelSystemDescriptorCoercion, RunCriteria, + RunCriteriaDescriptorCoercion, ShouldRun, SingleThreadedExecutor, Stage, SystemSet, + SystemStage, }, system::{In, IntoExclusiveSystem, Local, Query, ResMut}, world::World, @@ -1572,393 +1412,6 @@ mod tests { stage.run(&mut world); } - #[test] - fn ambiguity_detection() { - use super::{find_ambiguities, SystemContainer}; - - fn find_ambiguities_first_str_labels( - systems: &[impl SystemContainer], - ) -> Vec<(BoxedSystemLabel, BoxedSystemLabel)> { - find_ambiguities(systems) - .drain(..) - .map(|(index_a, index_b, _conflicts)| { - ( - systems[index_a] - .labels() - .iter() - .find(|a| (&***a).type_id() == std::any::TypeId::of::<&str>()) - .unwrap() - .clone(), - systems[index_b] - .labels() - .iter() - .find(|a| (&***a).type_id() == std::any::TypeId::of::<&str>()) - .unwrap() - .clone(), - ) - }) - .collect() - } - - fn empty() {} - fn resource(_: ResMut) {} - fn component(_: Query<&mut W>) {} - - let mut world = World::new(); - - let mut stage = SystemStage::parallel() - .with_system(empty.label("0")) - .with_system(empty.label("1").after("0")) - .with_system(empty.label("2")) - .with_system(empty.label("3").after("2").before("4")) - .with_system(empty.label("4")); - stage.initialize_systems(&mut world); - stage.rebuild_orders_and_dependencies(); - assert_eq!(find_ambiguities(&stage.parallel).len(), 0); - - let mut stage = SystemStage::parallel() - .with_system(empty.label("0")) - .with_system(component.label("1").after("0")) - .with_system(empty.label("2")) - .with_system(empty.label("3").after("2").before("4")) - .with_system(component.label("4")); - stage.initialize_systems(&mut world); - stage.rebuild_orders_and_dependencies(); - let ambiguities = find_ambiguities_first_str_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(empty.label("0")) - .with_system(resource.label("1").after("0")) - .with_system(empty.label("2")) - .with_system(empty.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_str_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(empty.label("0")) - .with_system(resource.label("1").after("0")) - .with_system(empty.label("2")) - .with_system(empty.label("3").after("2").before("4")) - .with_system(component.label("4")); - stage.initialize_systems(&mut world); - stage.rebuild_orders_and_dependencies(); - assert_eq!(find_ambiguities(&stage.parallel).len(), 0); - - 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")) - .with_system(resource.label("4")); - stage.initialize_systems(&mut world); - stage.rebuild_orders_and_dependencies(); - let ambiguities = find_ambiguities_first_str_labels(&stage.parallel); - assert!( - ambiguities.contains(&(Box::new("0"), Box::new("3"))) - || ambiguities.contains(&(Box::new("3"), Box::new("0"))) - ); - assert!( - ambiguities.contains(&(Box::new("1"), Box::new("4"))) - || ambiguities.contains(&(Box::new("4"), Box::new("1"))) - ); - assert_eq!(ambiguities.len(), 2); - - let mut stage = SystemStage::parallel() - .with_system(component.label("0")) - .with_system(resource.label("1").after("0").in_ambiguity_set("a")) - .with_system(empty.label("2")) - .with_system(component.label("3").after("2").before("4")) - .with_system(resource.label("4").in_ambiguity_set("a")); - stage.initialize_systems(&mut world); - stage.rebuild_orders_and_dependencies(); - let ambiguities = find_ambiguities_first_str_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").before("2")) - .with_system(component.label("1").before("2")) - .with_system(component.label("2")); - stage.initialize_systems(&mut world); - stage.rebuild_orders_and_dependencies(); - let ambiguities = find_ambiguities_first_str_labels(&stage.parallel); - assert!( - ambiguities.contains(&(Box::new("0"), Box::new("1"))) - || ambiguities.contains(&(Box::new("1"), Box::new("0"))) - ); - assert_eq!(ambiguities.len(), 1); - - let mut stage = SystemStage::parallel() - .with_system(component.label("0")) - .with_system(component.label("1").after("0")) - .with_system(component.label("2").after("0")); - stage.initialize_systems(&mut world); - stage.rebuild_orders_and_dependencies(); - let ambiguities = find_ambiguities_first_str_labels(&stage.parallel); - assert!( - ambiguities.contains(&(Box::new("1"), Box::new("2"))) - || ambiguities.contains(&(Box::new("2"), Box::new("1"))) - ); - assert_eq!(ambiguities.len(), 1); - - let mut stage = SystemStage::parallel() - .with_system(component.label("0").before("1").before("2")) - .with_system(component.label("1")) - .with_system(component.label("2")) - .with_system(component.label("3").after("1").after("2")); - stage.initialize_systems(&mut world); - stage.rebuild_orders_and_dependencies(); - let ambiguities = find_ambiguities_first_str_labels(&stage.parallel); - assert!( - ambiguities.contains(&(Box::new("1"), Box::new("2"))) - || ambiguities.contains(&(Box::new("2"), Box::new("1"))) - ); - assert_eq!(ambiguities.len(), 1); - - let mut stage = SystemStage::parallel() - .with_system(component.label("0").before("1").before("2")) - .with_system(component.label("1").in_ambiguity_set("a")) - .with_system(component.label("2").in_ambiguity_set("a")) - .with_system(component.label("3").after("1").after("2")); - stage.initialize_systems(&mut world); - stage.rebuild_orders_and_dependencies(); - let ambiguities = find_ambiguities_first_str_labels(&stage.parallel); - assert_eq!(ambiguities.len(), 0); - - let mut stage = SystemStage::parallel() - .with_system(component.label("0").before("1").before("2")) - .with_system(component.label("1").in_ambiguity_set("a")) - .with_system(component.label("2").in_ambiguity_set("b")) - .with_system(component.label("3").after("1").after("2")); - stage.initialize_systems(&mut world); - stage.rebuild_orders_and_dependencies(); - let ambiguities = find_ambiguities_first_str_labels(&stage.parallel); - assert!( - ambiguities.contains(&(Box::new("1"), Box::new("2"))) - || ambiguities.contains(&(Box::new("2"), Box::new("1"))) - ); - assert_eq!(ambiguities.len(), 1); - - let mut stage = SystemStage::parallel() - .with_system( - component - .label("0") - .before("1") - .before("2") - .before("3") - .before("4"), - ) - .with_system(component.label("1")) - .with_system(component.label("2")) - .with_system(component.label("3")) - .with_system(component.label("4")) - .with_system( - component - .label("5") - .after("1") - .after("2") - .after("3") - .after("4"), - ); - stage.initialize_systems(&mut world); - stage.rebuild_orders_and_dependencies(); - let ambiguities = find_ambiguities_first_str_labels(&stage.parallel); - assert!( - ambiguities.contains(&(Box::new("1"), Box::new("2"))) - || ambiguities.contains(&(Box::new("2"), Box::new("1"))) - ); - assert!( - ambiguities.contains(&(Box::new("1"), Box::new("3"))) - || ambiguities.contains(&(Box::new("3"), Box::new("1"))) - ); - assert!( - ambiguities.contains(&(Box::new("1"), Box::new("4"))) - || ambiguities.contains(&(Box::new("4"), Box::new("1"))) - ); - assert!( - ambiguities.contains(&(Box::new("2"), Box::new("3"))) - || ambiguities.contains(&(Box::new("3"), Box::new("2"))) - ); - assert!( - ambiguities.contains(&(Box::new("2"), Box::new("4"))) - || ambiguities.contains(&(Box::new("4"), Box::new("2"))) - ); - assert!( - ambiguities.contains(&(Box::new("3"), Box::new("4"))) - || ambiguities.contains(&(Box::new("4"), Box::new("3"))) - ); - assert_eq!(ambiguities.len(), 6); - - let mut stage = SystemStage::parallel() - .with_system( - component - .label("0") - .before("1") - .before("2") - .before("3") - .before("4"), - ) - .with_system(component.label("1").in_ambiguity_set("a")) - .with_system(component.label("2").in_ambiguity_set("a")) - .with_system(component.label("3").in_ambiguity_set("a")) - .with_system(component.label("4").in_ambiguity_set("a")) - .with_system( - component - .label("5") - .after("1") - .after("2") - .after("3") - .after("4"), - ); - stage.initialize_systems(&mut world); - stage.rebuild_orders_and_dependencies(); - let ambiguities = find_ambiguities_first_str_labels(&stage.parallel); - assert_eq!(ambiguities.len(), 0); - - let mut stage = SystemStage::parallel() - .with_system( - component - .label("0") - .before("1") - .before("2") - .before("3") - .before("4"), - ) - .with_system(component.label("1").in_ambiguity_set("a")) - .with_system(component.label("2").in_ambiguity_set("a")) - .with_system( - component - .label("3") - .in_ambiguity_set("a") - .in_ambiguity_set("b"), - ) - .with_system(component.label("4").in_ambiguity_set("b")) - .with_system( - component - .label("5") - .after("1") - .after("2") - .after("3") - .after("4"), - ); - stage.initialize_systems(&mut world); - stage.rebuild_orders_and_dependencies(); - let ambiguities = find_ambiguities_first_str_labels(&stage.parallel); - assert!( - ambiguities.contains(&(Box::new("1"), Box::new("4"))) - || ambiguities.contains(&(Box::new("4"), Box::new("1"))) - ); - assert!( - ambiguities.contains(&(Box::new("2"), Box::new("4"))) - || ambiguities.contains(&(Box::new("4"), Box::new("2"))) - ); - assert_eq!(ambiguities.len(), 2); - - let mut stage = SystemStage::parallel() - .with_system(empty.exclusive_system().label("0")) - .with_system(empty.exclusive_system().label("1").after("0")) - .with_system(empty.exclusive_system().label("2").after("1")) - .with_system(empty.exclusive_system().label("3").after("2")) - .with_system(empty.exclusive_system().label("4").after("3")) - .with_system(empty.exclusive_system().label("5").after("4")) - .with_system(empty.exclusive_system().label("6").after("5")) - .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); - - let mut stage = SystemStage::parallel() - .with_system(empty.exclusive_system().label("0").before("1").before("3")) - .with_system(empty.exclusive_system().label("1")) - .with_system(empty.exclusive_system().label("2").after("1")) - .with_system(empty.exclusive_system().label("3")) - .with_system(empty.exclusive_system().label("4").after("3").before("5")) - .with_system(empty.exclusive_system().label("5")) - .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_str_labels(&stage.exclusive_at_start); - assert!( - ambiguities.contains(&(Box::new("1"), Box::new("3"))) - || ambiguities.contains(&(Box::new("3"), Box::new("1"))) - ); - assert!( - ambiguities.contains(&(Box::new("2"), Box::new("3"))) - || ambiguities.contains(&(Box::new("3"), Box::new("2"))) - ); - assert!( - ambiguities.contains(&(Box::new("1"), Box::new("4"))) - || ambiguities.contains(&(Box::new("4"), Box::new("1"))) - ); - assert!( - ambiguities.contains(&(Box::new("2"), Box::new("4"))) - || ambiguities.contains(&(Box::new("4"), Box::new("2"))) - ); - assert!( - ambiguities.contains(&(Box::new("1"), Box::new("5"))) - || ambiguities.contains(&(Box::new("5"), Box::new("1"))) - ); - assert!( - ambiguities.contains(&(Box::new("2"), Box::new("5"))) - || ambiguities.contains(&(Box::new("5"), Box::new("2"))) - ); - assert_eq!(ambiguities.len(), 6); - - let mut stage = SystemStage::parallel() - .with_system(empty.exclusive_system().label("0").before("1").before("3")) - .with_system(empty.exclusive_system().label("1").in_ambiguity_set("a")) - .with_system(empty.exclusive_system().label("2").after("1")) - .with_system(empty.exclusive_system().label("3").in_ambiguity_set("a")) - .with_system(empty.exclusive_system().label("4").after("3").before("5")) - .with_system(empty.exclusive_system().label("5").in_ambiguity_set("a")) - .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_str_labels(&stage.exclusive_at_start); - assert!( - ambiguities.contains(&(Box::new("2"), Box::new("3"))) - || ambiguities.contains(&(Box::new("3"), Box::new("2"))) - ); - assert!( - ambiguities.contains(&(Box::new("1"), Box::new("4"))) - || ambiguities.contains(&(Box::new("4"), Box::new("1"))) - ); - assert!( - ambiguities.contains(&(Box::new("2"), Box::new("4"))) - || ambiguities.contains(&(Box::new("4"), Box::new("2"))) - ); - assert!( - ambiguities.contains(&(Box::new("2"), Box::new("5"))) - || ambiguities.contains(&(Box::new("5"), Box::new("2"))) - ); - assert_eq!(ambiguities.len(), 4); - - let mut stage = SystemStage::parallel() - .with_system(empty.exclusive_system().label("0").in_ambiguity_set("a")) - .with_system(empty.exclusive_system().label("1").in_ambiguity_set("a")) - .with_system(empty.exclusive_system().label("2").in_ambiguity_set("a")) - .with_system(empty.exclusive_system().label("3").in_ambiguity_set("a")); - stage.initialize_systems(&mut world); - stage.rebuild_orders_and_dependencies(); - let ambiguities = find_ambiguities_first_str_labels(&stage.exclusive_at_start); - assert_eq!(ambiguities.len(), 0); - } - #[test] #[should_panic] fn multiple_worlds_same_stage() { diff --git a/crates/bevy_ecs/src/schedule/system_container.rs b/crates/bevy_ecs/src/schedule/system_container.rs index 108e391d606f8..a63efe1fe70af 100644 --- a/crates/bevy_ecs/src/schedule/system_container.rs +++ b/crates/bevy_ecs/src/schedule/system_container.rs @@ -2,13 +2,15 @@ use crate::{ component::ComponentId, query::Access, schedule::{ - BoxedAmbiguitySetLabel, BoxedRunCriteriaLabel, BoxedSystemLabel, ExclusiveSystemDescriptor, - GraphNode, ParallelSystemDescriptor, + BoxedRunCriteriaLabel, BoxedSystemLabel, ExclusiveSystemDescriptor, GraphNode, + ParallelSystemDescriptor, }, system::{ExclusiveSystem, System}, }; use std::borrow::Cow; +use super::AmbiguityDetection; + /// System metadata like its name, labels, order requirements and component access. pub trait SystemContainer: GraphNode