Skip to content

Commit

Permalink
Fix anonymous set name stack overflow (#9650)
Browse files Browse the repository at this point in the history
# Objective

- Fixes #9641
- Anonymous sets are named by their system members. When
`ScheduleBuildSettings::report_sets` is on, systems are named by their
sets. So when getting the anonymous set name this would cause an
infinite recursion.

## Solution
- When getting the anonymous system set name, don't get their system's
names with the sets the systems belong to.

## Other Possible solutions
- An alternate solution might be to skip anonymous sets when getting the
system's name for an anonymous set's name.
  • Loading branch information
hymm authored Aug 31, 2023
1 parent c2b85f9 commit 02025ef
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 2 deletions.
33 changes: 33 additions & 0 deletions crates/bevy_ecs/src/schedule/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1045,5 +1045,38 @@ mod tests {
assert!(ambiguities.contains(entry));
}
}

// Test that anonymous set names work properly
// Related issue https://github.com/bevyengine/bevy/issues/9641
#[test]
fn anonymous_set_name() {
use super::*;

#[derive(ScheduleLabel, Hash, PartialEq, Eq, Debug, Clone)]
struct TestSchedule;

let mut schedule = Schedule::new(TestSchedule);
schedule.add_systems((resmut_system, resmut_system).run_if(|| true));

let mut world = World::new();
schedule.graph_mut().initialize(&mut world);
let _ = schedule
.graph_mut()
.build_schedule(world.components(), &TestSchedule.dyn_clone());

let ambiguities: Vec<_> = schedule
.graph()
.conflicts_to_string(schedule.graph().conflicting_systems(), world.components())
.collect();

assert_eq!(
ambiguities[0],
(
"resmut_system (in set (resmut_system, resmut_system))".to_string(),
"resmut_system (in set (resmut_system, resmut_system))".to_string(),
vec!["bevy_ecs::schedule::tests::system_ambiguity::R"],
)
);
}
}
}
10 changes: 8 additions & 2 deletions crates/bevy_ecs/src/schedule/schedule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1257,10 +1257,15 @@ enum ReportCycles {
// methods for reporting errors
impl ScheduleGraph {
fn get_node_name(&self, id: &NodeId) -> String {
self.get_node_name_inner(id, self.settings.report_sets)
}

#[inline]
fn get_node_name_inner(&self, id: &NodeId, report_sets: bool) -> String {
let mut name = match id {
NodeId::System(_) => {
let name = self.systems[id.index()].get().unwrap().name().to_string();
if self.settings.report_sets {
if report_sets {
let sets = self.names_of_sets_containing_node(id);
if sets.is_empty() {
name
Expand Down Expand Up @@ -1294,7 +1299,8 @@ impl ScheduleGraph {
self.hierarchy
.graph
.edges_directed(*id, Direction::Outgoing)
.map(|(_, member_id, _)| self.get_node_name(&member_id))
// never get the sets of the members or this will infinite recurse when the report_sets setting is on.
.map(|(_, member_id, _)| self.get_node_name_inner(&member_id, false))
.reduce(|a, b| format!("{a}, {b}"))
.unwrap_or_default()
)
Expand Down

0 comments on commit 02025ef

Please sign in to comment.