From 2407f06f51f2e987d88108bbb88e874b922b8124 Mon Sep 17 00:00:00 2001 From: Edgar Geier Date: Mon, 20 Feb 2023 03:14:11 +0100 Subject: [PATCH 1/6] Add `report_sets` option to `ScheduleBuildSettings` --- crates/bevy_ecs/src/schedule/schedule.rs | 45 +++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/schedule/schedule.rs b/crates/bevy_ecs/src/schedule/schedule.rs index 4475cbed11a8d..588ae11bfc666 100644 --- a/crates/bevy_ecs/src/schedule/schedule.rs +++ b/crates/bevy_ecs/src/schedule/schedule.rs @@ -1151,7 +1151,7 @@ impl ScheduleGraph { if self.settings.ambiguity_detection != LogLevel::Ignore && self.contains_conflicts(&conflicting_systems) { - self.report_conflicts(&conflicting_systems, components); + self.report_conflicts(&conflicting_systems, components, self.settings.report_sets); if matches!(self.settings.ambiguity_detection, LogLevel::Error) { return Err(ScheduleBuildError::Ambiguity); } @@ -1421,6 +1421,7 @@ impl ScheduleGraph { &self, ambiguities: &[(NodeId, NodeId, Vec)], components: &Components, + report_sets: bool, ) { let n_ambiguities = ambiguities.len(); @@ -1433,6 +1434,23 @@ impl ScheduleGraph { let name_a = self.get_node_name(system_a); let name_b = self.get_node_name(system_b); + let (name_a, name_b) = if report_sets { + ( + format!( + "{} ({})", + name_a, + self.names_of_sets_containing_node(system_a) + ), + format!( + "{} ({})", + name_b, + self.names_of_sets_containing_node(system_b) + ), + ) + } else { + (name_a, name_b) + }; + debug_assert!(system_a.is_system(), "{name_a} is not a system."); debug_assert!(system_b.is_system(), "{name_b} is not a system."); @@ -1453,6 +1471,27 @@ impl ScheduleGraph { warn!("{}", string); } + + fn traverse_sets_containing_node(&self, id: NodeId, f: &mut impl FnMut(NodeId) -> bool) { + for (set_id, _, _) in self.hierarchy.graph.edges_directed(id, Direction::Incoming) { + if f(set_id) { + self.traverse_sets_containing_node(set_id, f); + } + } + } + + fn names_of_sets_containing_node(&self, id: &NodeId) -> String { + let mut sets = HashSet::new(); + self.traverse_sets_containing_node(*id, &mut |set_id| { + !self.system_sets[set_id.index()].is_system_type() && sets.insert(set_id) + }); + let mut sets: Vec<_> = sets + .into_iter() + .map(|set_id| self.get_node_name(&set_id)) + .collect(); + sets.sort(); + sets.join(", ") + } } /// Category of errors encountered during schedule construction. @@ -1532,6 +1571,9 @@ pub struct ScheduleBuildSettings { pub hierarchy_detection: LogLevel, /// If set to true, node names will be shortened instead of the fully qualified type path. pub use_shortnames: bool, + /// If set to true, report all system sets the conflicting systems are part of. + /// Only used when reporting ambiguities. + pub report_sets: bool, } impl Default for ScheduleBuildSettings { @@ -1546,6 +1588,7 @@ impl ScheduleBuildSettings { ambiguity_detection: LogLevel::Ignore, hierarchy_detection: LogLevel::Warn, use_shortnames: false, + report_sets: false, } } } From 246e9d76a552d9157438dfa1c53621b07042f4f1 Mon Sep 17 00:00:00 2001 From: Edgar Geier Date: Mon, 20 Feb 2023 03:17:19 +0100 Subject: [PATCH 2/6] Report "(no_sets)" instead of "()" if system isn't part of a set --- crates/bevy_ecs/src/schedule/schedule.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/schedule.rs b/crates/bevy_ecs/src/schedule/schedule.rs index 588ae11bfc666..5543ff89aaad2 100644 --- a/crates/bevy_ecs/src/schedule/schedule.rs +++ b/crates/bevy_ecs/src/schedule/schedule.rs @@ -1489,8 +1489,12 @@ impl ScheduleGraph { .into_iter() .map(|set_id| self.get_node_name(&set_id)) .collect(); - sets.sort(); - sets.join(", ") + if sets.is_empty() { + "no sets".to_owned() + } else { + sets.sort(); + sets.join(", ") + } } } From 2d51c4f80d303d1fcc09f264c5a6da2c92ea01e1 Mon Sep 17 00:00:00 2001 From: Edgar Geier Date: Mon, 20 Feb 2023 05:22:59 +0100 Subject: [PATCH 3/6] Make `use_shortnames` and `report_sets` set to `true` by default. --- crates/bevy_ecs/src/schedule/schedule.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/schedule.rs b/crates/bevy_ecs/src/schedule/schedule.rs index 5543ff89aaad2..a6bacca6db2de 100644 --- a/crates/bevy_ecs/src/schedule/schedule.rs +++ b/crates/bevy_ecs/src/schedule/schedule.rs @@ -1591,8 +1591,8 @@ impl ScheduleBuildSettings { Self { ambiguity_detection: LogLevel::Ignore, hierarchy_detection: LogLevel::Warn, - use_shortnames: false, - report_sets: false, + use_shortnames: true, + report_sets: true, } } } From 754eced3a55e2e2d25cc5c8f4f6282210a112e70 Mon Sep 17 00:00:00 2001 From: Edgar Geier Date: Mon, 20 Feb 2023 05:26:09 +0100 Subject: [PATCH 4/6] Document default values of `ScheduleBuildSettings` --- crates/bevy_ecs/src/schedule/schedule.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/schedule/schedule.rs b/crates/bevy_ecs/src/schedule/schedule.rs index a6bacca6db2de..b1fcf9341b73b 100644 --- a/crates/bevy_ecs/src/schedule/schedule.rs +++ b/crates/bevy_ecs/src/schedule/schedule.rs @@ -1568,15 +1568,22 @@ pub enum LogLevel { pub struct ScheduleBuildSettings { /// Determines whether the presence of ambiguities (systems with conflicting access but indeterminate order) /// is only logged or also results in an [`Ambiguity`](ScheduleBuildError::Ambiguity) error. + /// + /// Defaults to [`LogLevel::Ignore`]. pub ambiguity_detection: LogLevel, /// Determines whether the presence of redundant edges in the hierarchy of system sets is only /// logged or also results in a [`HierarchyRedundancy`](ScheduleBuildError::HierarchyRedundancy) /// error. + /// + /// Defaults to [`LogLevel::Warn`]. pub hierarchy_detection: LogLevel, /// If set to true, node names will be shortened instead of the fully qualified type path. + /// + /// Defaults to `true`. pub use_shortnames: bool, /// If set to true, report all system sets the conflicting systems are part of. - /// Only used when reporting ambiguities. + /// + /// Defaults to `true`. pub report_sets: bool, } From ea9a3693c51e91514f7c549b1b7a929507a5bd42 Mon Sep 17 00:00:00 2001 From: Edgar Geier Date: Mon, 20 Feb 2023 05:38:59 +0100 Subject: [PATCH 5/6] Move `report_sets` logic into `get_node_name` --- crates/bevy_ecs/src/schedule/schedule.rs | 46 ++++++++++-------------- 1 file changed, 19 insertions(+), 27 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/schedule.rs b/crates/bevy_ecs/src/schedule/schedule.rs index b1fcf9341b73b..67bc14230407b 100644 --- a/crates/bevy_ecs/src/schedule/schedule.rs +++ b/crates/bevy_ecs/src/schedule/schedule.rs @@ -1151,7 +1151,7 @@ impl ScheduleGraph { if self.settings.ambiguity_detection != LogLevel::Ignore && self.contains_conflicts(&conflicting_systems) { - self.report_conflicts(&conflicting_systems, components, self.settings.report_sets); + self.report_conflicts(&conflicting_systems, components); if matches!(self.settings.ambiguity_detection, LogLevel::Error) { return Err(ScheduleBuildError::Ambiguity); } @@ -1322,7 +1322,21 @@ impl ScheduleGraph { impl ScheduleGraph { fn get_node_name(&self, id: &NodeId) -> String { let mut name = match id { - NodeId::System(_) => self.systems[id.index()].get().unwrap().name().to_string(), + NodeId::System(_) => { + let name = self.systems[id.index()].get().unwrap().name().to_string(); + if self.settings.report_sets { + let sets = self.names_of_sets_containing_node(id); + if sets.is_empty() { + format!("{name} (no sets)") + } else if sets.len() == 1 { + format!("{name} (in set {})", sets[0]) + } else { + format!("{name} (in sets {})", sets.join(", ")) + } + } else { + name + } + } NodeId::Set(_) => self.system_sets[id.index()].name(), }; if self.settings.use_shortnames { @@ -1421,7 +1435,6 @@ impl ScheduleGraph { &self, ambiguities: &[(NodeId, NodeId, Vec)], components: &Components, - report_sets: bool, ) { let n_ambiguities = ambiguities.len(); @@ -1434,23 +1447,6 @@ impl ScheduleGraph { let name_a = self.get_node_name(system_a); let name_b = self.get_node_name(system_b); - let (name_a, name_b) = if report_sets { - ( - format!( - "{} ({})", - name_a, - self.names_of_sets_containing_node(system_a) - ), - format!( - "{} ({})", - name_b, - self.names_of_sets_containing_node(system_b) - ), - ) - } else { - (name_a, name_b) - }; - debug_assert!(system_a.is_system(), "{name_a} is not a system."); debug_assert!(system_b.is_system(), "{name_b} is not a system."); @@ -1480,7 +1476,7 @@ impl ScheduleGraph { } } - fn names_of_sets_containing_node(&self, id: &NodeId) -> String { + fn names_of_sets_containing_node(&self, id: &NodeId) -> Vec { let mut sets = HashSet::new(); self.traverse_sets_containing_node(*id, &mut |set_id| { !self.system_sets[set_id.index()].is_system_type() && sets.insert(set_id) @@ -1489,12 +1485,8 @@ impl ScheduleGraph { .into_iter() .map(|set_id| self.get_node_name(&set_id)) .collect(); - if sets.is_empty() { - "no sets".to_owned() - } else { - sets.sort(); - sets.join(", ") - } + sets.sort(); + sets } } From 7d8f0da7727a60c596f106f18d014b5c8982bb4f Mon Sep 17 00:00:00 2001 From: Edgar Geier Date: Mon, 20 Feb 2023 16:16:20 +0100 Subject: [PATCH 6/6] Update crates/bevy_ecs/src/schedule/schedule.rs Co-authored-by: JoJoJet <21144246+JoJoJet@users.noreply.github.com> --- crates/bevy_ecs/src/schedule/schedule.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/schedule/schedule.rs b/crates/bevy_ecs/src/schedule/schedule.rs index 67bc14230407b..5a9dfbf1aa00c 100644 --- a/crates/bevy_ecs/src/schedule/schedule.rs +++ b/crates/bevy_ecs/src/schedule/schedule.rs @@ -1327,7 +1327,7 @@ impl ScheduleGraph { if self.settings.report_sets { let sets = self.names_of_sets_containing_node(id); if sets.is_empty() { - format!("{name} (no sets)") + name } else if sets.len() == 1 { format!("{name} (in set {})", sets[0]) } else {