Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Merged by Bors] - expose ScheduleGraph for third party dependencies #7522

Conversation

jakobhellermann
Copy link
Contributor

Objective

The ScheduleGraph should be expose so that crates like bevy_mod_debugdump can access useful information.

Solution

  • expose ScheduleGraph, NodeId, BaseSetMembership, Dag
  • add accessor functions for sets and systems

Changelog

  • expose ScheduleGraph for use in third party tools

This does expose our use of petgraph as a graph library, so we can only change that as a breaking change.

@jakobhellermann jakobhellermann added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events labels Feb 6, 2023
@@ -751,7 +878,13 @@ impl ScheduleGraph {
Ok(base_set)
}

fn build_schedule(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of making this (and SystemSchedule) public, we could have a method

pub fn ScheduleGraph::compute_info(&mut self) -> ScheduleGraphInfo {
  // compute ambiguities, base sets, cycles, ...
  // caches dependency_flattened etc.
}
fn ScheduleGraph::build_schedule(&self, &Components, ScheduleGraphInfo) -> Result<SystemSchedule, _> {}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that overall I like the direction here: I can imagine future use cases where manually controlling schedule builds is helpful.

@alice-i-cecile alice-i-cecile added this to the 0.10 milestone Feb 6, 2023
@hymm hymm requested review from hymm and maniwani February 6, 2023 17:11
@jakobhellermann jakobhellermann force-pushed the stageless-expose-schedule-graph branch from 19f7d37 to c5ca453 Compare February 13, 2023 20:54
@jakobhellermann jakobhellermann force-pushed the stageless-expose-schedule-graph branch from c5ca453 to baeabd6 Compare February 13, 2023 20:55
Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is the right way of exposing this functionality. But I'm going to approve since it's relatively unintrusive and I think it's important that bevy_mod_debugdump works for 0.10.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seconding WrongShoe's comment above, but I don't mind this approach for now.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 13, 2023
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Feb 16, 2023
# Objective

The `ScheduleGraph` should be expose so that crates like [bevy_mod_debugdump](https://github.com/jakobhellermann/bevy_mod_debugdump/blob/stageless/docs/README.md) can access useful information. 

## Solution

- expose `ScheduleGraph`, `NodeId`, `BaseSetMembership`, `Dag`
- add accessor functions for sets and systems

## Changelog

- expose `ScheduleGraph` for use in third party tools

This does expose our use of `petgraph` as a graph library, so we can only change that as a breaking change.
@bors bors bot changed the title expose ScheduleGraph for third party dependencies [Merged by Bors] - expose ScheduleGraph for third party dependencies Feb 16, 2023
@bors bors bot closed this Feb 16, 2023
myreprise1 pushed a commit to myreprise1/bevy that referenced this pull request Feb 16, 2023
# Objective

The `ScheduleGraph` should be expose so that crates like [bevy_mod_debugdump](https://github.com/jakobhellermann/bevy_mod_debugdump/blob/stageless/docs/README.md) can access useful information. 

## Solution

- expose `ScheduleGraph`, `NodeId`, `BaseSetMembership`, `Dag`
- add accessor functions for sets and systems

## Changelog

- expose `ScheduleGraph` for use in third party tools

This does expose our use of `petgraph` as a graph library, so we can only change that as a breaking change.
bors bot pushed a commit that referenced this pull request Feb 17, 2023
# Objective

- other tools (bevy_mod_debugdump) would like to have access to the ambiguities so that they can do their own reporting/visualization

## Solution

- store `conflicting_systems` in `ScheduleGraph` after calling `build_schedule`

The solution isn't very pretty and as pointed out it #7522, there may be a better way of exposing this, but this is the quick and dirty way if we want to have this functionality exposed in 0.10.
myreprise1 pushed a commit to myreprise1/bevy that referenced this pull request Feb 18, 2023
# Objective

- other tools (bevy_mod_debugdump) would like to have access to the ambiguities so that they can do their own reporting/visualization

## Solution

- store `conflicting_systems` in `ScheduleGraph` after calling `build_schedule`

The solution isn't very pretty and as pointed out it bevyengine#7522, there may be a better way of exposing this, but this is the quick and dirty way if we want to have this functionality exposed in 0.10.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants