diff --git a/crates/bevy_ecs/src/schedule/ambiguity_detection.rs b/crates/bevy_ecs/src/schedule/ambiguity_detection.rs index a9aa1de58b231..3cc01961d2de3 100644 --- a/crates/bevy_ecs/src/schedule/ambiguity_detection.rs +++ b/crates/bevy_ecs/src/schedule/ambiguity_detection.rs @@ -1,4 +1,5 @@ use bevy_utils::tracing::info; +use bevy_utils::HashMap; use fixedbitset::FixedBitSet; use crate::component::ComponentId; @@ -8,10 +9,8 @@ use crate::world::World; #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] struct SystemOrderAmbiguity { segment: SystemStageSegment, - // Note: In order for comparisons to work correctly, - // `system_names` and `conflicts` must be sorted at all times. - system_names: [String; 2], conflicts: Vec, + system_names: Vec, } /// Which part of a [`SystemStage`] was a [`SystemOrderAmbiguity`] detected in? @@ -36,70 +35,6 @@ impl SystemStageSegment { } } -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 mut system_names = [system_a_name.to_string(), system_b_name.to_string()]; - system_names.sort(); - - let mut conflicts: Vec<_> = component_ids - .iter() - .map(|id| world.components().get_info(*id).unwrap().name().to_owned()) - .collect(); - conflicts.sort(); - - Self { - system_names, - conflicts, - segment, - } - } -} - impl SystemStage { /// Logs execution order ambiguities between systems. /// @@ -113,23 +48,30 @@ impl SystemStage { add an explicit dependency relation between some of these systems:\n" .to_owned(); - let mut last_segment_kind = None; - for SystemOrderAmbiguity { - system_names: [system_a, system_b], - conflicts, - segment, - } in &ambiguities - { - // If the ambiguity occurred in a different segment than the previous one, write a header for the segment. - if last_segment_kind != Some(segment) { - writeln!(string, " * {}:", segment.desc()).unwrap(); - last_segment_kind = Some(segment); - } + for (i, ambiguity) in ambiguities.iter().enumerate() { + let SystemOrderAmbiguity { + segment, + conflicts, + system_names, + .. + } = ambiguity; + + writeln!(string).unwrap(); + writeln!( + string, + "({i}) Ambiguous system ordering - {}", + segment.desc() + ) + .unwrap(); - writeln!(string, " -- {:?} and {:?}", system_a, system_b).unwrap(); + for name in system_names { + writeln!(string, " * {name}").unwrap(); + } - if !conflicts.is_empty() { - writeln!(string, " conflicts: {conflicts:?}").unwrap(); + writeln!(string).unwrap(); + writeln!(string, " Data access conflicts:").unwrap(); + for conflict in conflicts { + writeln!(string, " * {conflict}").unwrap(); } } @@ -147,65 +89,51 @@ impl SystemStage { /// /// The result may be incorrect if this stage has not been initialized with `world`. fn ambiguities(&self, world: &World) -> Vec { - let parallel = find_ambiguities(&self.parallel).into_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, - ) - }, - ); - - let at_start = find_ambiguities(&self.exclusive_at_start).into_iter().map( - |(system_a_index, system_b_index, component_ids)| { - SystemOrderAmbiguity::from_raw( - system_a_index, - system_b_index, - component_ids, - SystemStageSegment::ExclusiveAtStart, - self, - world, - ) - }, - ); - - let before_commands = find_ambiguities(&self.exclusive_before_commands) - .into_iter() - .map(|(system_a_index, system_b_index, component_ids)| { - SystemOrderAmbiguity::from_raw( - system_a_index, - system_b_index, - component_ids, - SystemStageSegment::ExclusiveBeforeCommands, - self, - world, - ) - }); - - let at_end = find_ambiguities(&self.exclusive_at_end).into_iter().map( - |(system_a_index, system_b_index, component_ids)| { - SystemOrderAmbiguity::from_raw( - system_a_index, - system_b_index, - component_ids, - SystemStageSegment::ExclusiveAtEnd, - self, - world, - ) - }, - ); + fn find_ambiguities_in<'a>( + segment: SystemStageSegment, + container: &'a [impl SystemContainer], + world: &'a World, + ) -> impl Iterator + 'a { + let info = AmbiguityInfo::delineate(find_ambiguities(container)); + info.into_iter().map( + move |AmbiguityInfo { + conflicts, systems, .. + }| { + let conflicts = conflicts + .iter() + .map(|id| world.components().get_info(*id).unwrap().name().to_owned()) + .collect(); + let mut system_names: Vec<_> = systems + .iter() + .map(|&SystemIndex(i)| container[i].name().to_string()) + .collect(); + system_names.sort(); + SystemOrderAmbiguity { + segment, + conflicts, + system_names, + } + }, + ) + } - let mut ambiguities: Vec<_> = at_start - .chain(parallel) - .chain(before_commands) - .chain(at_end) - .collect(); - ambiguities.sort(); - ambiguities + find_ambiguities_in(SystemStageSegment::Parallel, &self.parallel, world) + .chain(find_ambiguities_in( + SystemStageSegment::ExclusiveAtStart, + &self.exclusive_at_start, + world, + )) + .chain(find_ambiguities_in( + SystemStageSegment::ExclusiveBeforeCommands, + &self.exclusive_before_commands, + world, + )) + .chain(find_ambiguities_in( + SystemStageSegment::ExclusiveAtEnd, + &self.exclusive_at_end, + world, + )) + .collect() } /// Returns the number of system order ambiguities between systems in this stage. @@ -213,14 +141,104 @@ impl SystemStage { /// The result may be incorrect if this stage has not been initialized with `world`. #[cfg(test)] fn ambiguity_count(&self, world: &World) -> usize { - self.ambiguities(world).len() + fn binomial_coefficient(n: usize, k: usize) -> usize { + (0..k).fold(1, |x, i| x * (n - i) / (i + 1)) + } + + self.ambiguities(world) + .iter() + .map(|a| binomial_coefficient(a.system_names.len(), 2)) + .sum() + } +} + +/// A set of systems that are all reported to be ambiguous with one another. +#[derive(PartialEq, Eq, PartialOrd, Ord)] +struct AmbiguityInfo { + // INVARIANT: `conflicts` is always sorted. + conflicts: Vec, + systems: Vec, +} + +#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +struct SystemIndex(usize); + +impl AmbiguityInfo { + fn delineate( + pairs: impl IntoIterator)>, + ) -> Vec { + // Stores the pairs of system indices associated with each set of conflicts. + let mut pairs_by_conflicts = HashMap::new(); + for (system_a_index, system_b_index, mut conflicts) in pairs { + conflicts.sort(); + pairs_by_conflicts + .entry(conflicts) + .or_insert_with(Vec::new) + .push([system_a_index, system_b_index]); + } + + let mut ambiguity_sets = Vec::new(); + for (conflicts, pairs) in pairs_by_conflicts { + // Find all unique systems that have the same conflicts. + // Note that this does *not* mean they all conflict with one another. + let mut in_set: Vec<_> = pairs.iter().copied().flatten().collect(); + in_set.sort(); + in_set.dedup(); + + // adjacency marix for the entries of `in_set` + let mut adj: Vec = (0..in_set.len()) + .map(|i| { + let mut bitset = FixedBitSet::with_capacity(in_set.len()); + // enable the main diagonal + bitset.set(i, true); + bitset + }) + .collect(); + // the value `pairs` mapped as indices in `in_set`. + let mut pairs_as_indices = Vec::new(); + for &[a, b] in &pairs { + let a_index = in_set.iter().position(|&i| i == a).unwrap(); + let b_index = in_set.iter().position(|&i| i == b).unwrap(); + pairs_as_indices.push([a_index, b_index]); + adj[a_index].set(b_index, true); + adj[b_index].set(a_index, true); + } + + // Find sets of systems that are all ambiguous with one another. + let mut subgraphs = Vec::new(); + for [a_index, b_index] in pairs_as_indices { + let intersection: FixedBitSet = adj[a_index].intersection(&adj[b_index]).collect(); + // If this pair has been included in another set, skip it. + if intersection.count_ones(..) <= 1 { + continue; + } + + for i in intersection.ones() { + adj[i].difference_with(&intersection); + // don't unset the main diagonal + adj[i].set(i, true); + } + + subgraphs.push(intersection); + } + + for subgraph in subgraphs { + ambiguity_sets.push(AmbiguityInfo { + conflicts: conflicts.clone(), + systems: subgraph.ones().map(|i| in_set[i]).collect(), + }); + } + } + ambiguity_sets } } /// 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], +) -> Vec<(SystemIndex, SystemIndex, Vec)> { 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() { @@ -268,10 +286,10 @@ fn find_ambiguities(systems: &[impl SystemContainer]) -> Vec<(usize, usize, Vec< 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)); + ambiguities.push((SystemIndex(index_a), SystemIndex(index_b), conflicts)); } } else { - ambiguities.push((index_a, index_b, Vec::new())); + ambiguities.push((SystemIndex(index_a), SystemIndex(index_b), Vec::new())); } } } @@ -282,6 +300,7 @@ fn find_ambiguities(systems: &[impl SystemContainer]) -> Vec<(usize, usize, Vec< #[cfg(test)] mod tests { + use super::*; // Required to make the derive macro behave use crate as bevy_ecs; use crate::event::Events; @@ -466,17 +485,29 @@ mod tests { let mut test_stage = SystemStage::parallel(); test_stage - // All 3 of these conflict with each other + // All 3 of these conflict with each other (`.at_start()` is the default configuration) .add_system(write_world_system.exclusive_system()) - .add_system(write_world_system.exclusive_system().at_end()) + .add_system(write_world_system.exclusive_system().at_start()) .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().at_end()) .add_system(write_world_system.exclusive_system().before_commands()); test_stage.run(&mut world); assert_eq!(test_stage.ambiguity_count(&world), 3); + assert_eq!( + test_stage.ambiguities(&world), + vec![SystemOrderAmbiguity { + segment: SystemStageSegment::ExclusiveAtStart, + conflicts: vec![], + system_names: vec![ + "bevy_ecs::schedule::ambiguity_detection::tests::res_system".to_owned(), + "bevy_ecs::schedule::ambiguity_detection::tests::write_world_system".to_owned(), + "bevy_ecs::schedule::ambiguity_detection::tests::write_world_system".to_owned(), + ] + }], + ); } // Tests for silencing and resolving ambiguities