Skip to content

Commit 9129cb1

Browse files
authored
Replace the bespoke BRP schedule.graph implementation with ScheduleData. (#23733)
# Objective - A possible step towards #10981. - Followup to #22520. - We accidentally have two implementations of the same thing! We created a way to collect schedule data in `bevy_dev_tools` and in `bevy_remote`. `ScheduleData` is a more complete implementation of collecting schedule data, and is less tied to the internals of BRP (e.g., it supports serializing to disk instead of only through the BRP API), so we switch to that. ## Solution - Replace the implementation of `schedule.graph` with the `ScheduleData` API. A disadvantage is that we now need to wait for the schedules to be initialized before we can read them. Since users have to connect with BRP though, it is almost certain that the schedules will be initialized by the time they request the schedules. This may not be true of schedules like state transitions though - since these only run rarely. In a future PR, we can build the schedules on-demand instead. ## Testing - Updated the test for this. - Ran the same test as #23452 - Terminal 1: `cargo r --example server --features=bevy_remote` - Terminal 2: `curl -d'{"jsonrpc":"2.0","method":"schedule.graph","id":1,"params":{"schedule_label":"First"}}' -X POST -H "Accept: applcation/json" -H "Content-Type: application/json" http://127.0.0.1:15702` - It dumped out a whole load of JSON that looked expected! - I don't have a visualization of the schedule data yet, so I can't validate that it's correct, but there's no reason to believe its wrong given the existing `ScheduleData` tests.
1 parent 9e7355d commit 9129cb1

File tree

2 files changed

+32
-63
lines changed

2 files changed

+32
-63
lines changed

crates/bevy_remote/Cargo.toml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,9 @@ bevy_render = ["dep:bevy_render"]
2525
bevy_app = { path = "../bevy_app", version = "0.19.0-dev" }
2626
bevy_color = { path = "../bevy_color", version = "0.19.0-dev" }
2727
bevy_derive = { path = "../bevy_derive", version = "0.19.0-dev" }
28+
bevy_dev_tools = { path = "../bevy_dev_tools", version = "0.19.0-dev", features = [
29+
"schedule_data",
30+
] }
2831
bevy_ecs = { path = "../bevy_ecs", version = "0.19.0-dev", features = [
2932
"serialize",
3033
] }

crates/bevy_remote/src/builtin_methods.rs

Lines changed: 29 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
use core::any::TypeId;
44

55
use anyhow::{anyhow, Result as AnyhowResult};
6+
use bevy_dev_tools::schedule_data::serde::ScheduleData;
67
use bevy_ecs::{
78
component::ComponentId,
89
entity::Entity,
@@ -526,25 +527,10 @@ pub struct BrpScheduleListResponse {
526527
/// If a system (f2) is placed in a developer-created set (S1), then there is a hierarchy edge (S1 -> f2).
527528
///
528529
/// If a schedule adds a condition f1.after(S1) , then a dependency edge is added (S1 -> f1)
529-
#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Default)]
530+
#[derive(Debug, Serialize, Deserialize, Clone)]
530531
pub struct BrpScheduleGraphResponse {
531-
systemsets: Vec<BrpSystemSet>,
532-
533-
hierarchy_nodes: Vec<String>,
534-
hierarchy_edges: Vec<(String, String)>,
535-
536-
dependency_nodes: Vec<String>,
537-
dependency_edges: Vec<(String, String)>,
538-
}
539-
540-
/// Details on a system set
541-
///
542-
/// The `key` is used in the nodes and edges in [`BrpScheduleGraphResponse`]
543-
/// The `method` is either the fully qualified system name, or the system set name
544-
#[derive(Debug, Serialize, Deserialize, Clone, PartialEq)]
545-
pub struct BrpSystemSet {
546-
key: String,
547-
method: String,
532+
/// The extracted data for the requested schedule.
533+
pub schedule_data: ScheduleData,
548534
}
549535

550536
/// One query match result: a single entity paired with the requested components.
@@ -1627,50 +1613,29 @@ pub fn schedule_graph(In(params): In<Option<Value>>, world: &mut World) -> BrpRe
16271613

16281614
let schedules = world.resource::<Schedules>();
16291615

1630-
let matching_schedule = schedules
1616+
let Some((label, schedule)) = schedules
16311617
.iter()
1632-
.find(|(label, _schedule)| format!("{:?}", label) == schedule_label);
1633-
1634-
if matching_schedule.is_none() {
1618+
.find(|(label, _schedule)| format!("{:?}", label) == schedule_label)
1619+
else {
16351620
return Err(BrpError::resource_error(format!(
1636-
"Schedule with label={:} not found",
1621+
"Schedule with label={:} not found. This may be because this schedule is currently running",
16371622
schedule_label
16381623
)));
1639-
}
1640-
1641-
let mut response: BrpScheduleGraphResponse = BrpScheduleGraphResponse::default();
1642-
1643-
let (_label, schedule) = matching_schedule.unwrap();
1644-
1645-
let g = schedule.graph();
1646-
for (systemsetkey, method, _b) in g.system_sets.iter() {
1647-
response.systemsets.push(BrpSystemSet {
1648-
key: format!("{:?}", systemsetkey),
1649-
method: format!("{:?}", method),
1650-
});
1651-
}
1652-
1653-
let hie = g.hierarchy();
1654-
for node in hie.nodes() {
1655-
response.hierarchy_nodes.push(format!("{:?}", node));
1656-
}
1657-
for (n1, n2) in hie.all_edges() {
1658-
response
1659-
.hierarchy_edges
1660-
.push((format!("{:?}", n1), format!("{:?}", n2)));
1661-
}
1624+
};
16621625

1663-
let dep = g.dependency();
1664-
for node in dep.nodes() {
1665-
response.dependency_nodes.push(format!("{:?}", node));
1666-
}
1667-
for (n1, n2) in dep.all_edges() {
1668-
response
1669-
.dependency_edges
1670-
.push((format!("{:?}", n1), format!("{:?}", n2)));
1671-
}
1626+
// TODO: We should consider saving all the `ScheduleBuilt` events when BRP is enabled, and
1627+
// looking it up here (or even building the schedule here to get the metadata to fill it in).
1628+
let schedule_data = match ScheduleData::from_schedule(schedule, world.components(), None) {
1629+
Ok(schedule_data) => schedule_data,
1630+
Err(err) => {
1631+
return Err(BrpError::internal(format!(
1632+
"Failed to collect the schedule data for {:?}: {err}",
1633+
label
1634+
)));
1635+
}
1636+
};
16721637

1673-
serde_json::to_value(response).map_err(BrpError::internal)
1638+
serde_json::to_value(BrpScheduleGraphResponse { schedule_data }).map_err(BrpError::internal)
16741639
}
16751640

16761641
/// Immutably retrieves an entity from the [`World`], returning an error if the
@@ -2215,20 +2180,21 @@ mod tests {
22152180
// Each system creates a corresponding system set.
22162181
// In the below notation we use f1 for the system, and F1 for the corresponding system set.
22172182
// Above schedule should have the following layout:
2218-
// - 4 systems (f1, f2, f3, f4)
2183+
// - 5 systems (f1, f2, f3, f4, apply_deferred)
22192184
// - 6 system sets (F1, F2, F3, F4, S1, S2)
2220-
// - 10 hierarchy nodes and 10 dependency nodes (4 + 6)
22212185
// - 6 hierarchy edges: F1 -> f1, F2 -> f2, F3 -> f3, F4 -> f4, S1 -> f3, S2 -> f4
22222186
// - 2 dependency edges: f1 -> f2, S1 -> f4
22232187

22242188
let res = schedule_graph(In(Some(params)), &mut world);
22252189
let res2 = res.expect("expect to work");
22262190
let res3 = serde_json::from_value::<BrpScheduleGraphResponse>(res2).unwrap();
22272191

2228-
assert_eq!(res3.systemsets.len(), 6);
2229-
assert_eq!(res3.hierarchy_nodes.len(), 10);
2230-
assert_eq!(res3.dependency_nodes.len(), 10);
2231-
assert_eq!(res3.hierarchy_edges.len(), 6);
2232-
assert_eq!(res3.dependency_edges.len(), 2);
2192+
assert_eq!(res3.schedule_data.systems.len(), 5,);
2193+
assert_eq!(res3.schedule_data.system_sets.len(), 6);
2194+
assert_eq!(res3.schedule_data.hierarchy.len(), 6);
2195+
assert_eq!(res3.schedule_data.dependency.len(), 2);
2196+
// Components are only currently recorded for conflicts - this may change in the future.
2197+
assert_eq!(res3.schedule_data.components.len(), 0);
2198+
assert_eq!(res3.schedule_data.conflicts.len(), 0);
22332199
}
22342200
}

0 commit comments

Comments
 (0)