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 ambiguities of ScheduleGraph #7716

Conversation

jakobhellermann
Copy link
Contributor

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.

@james7132 james7132 added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Feb 16, 2023
@jakobhellermann
Copy link
Contributor Author

Alternative: #7719

Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

I honestly like this better than #7719 due to it's simplicity and more controlled API surface. If we need to alter the implementation in the future, this seems like a much better approach.

Comment on lines +237 to +241
/// Returns a mutable reference to the [`ScheduleGraph`].
pub fn graph_mut(&mut self) -> &mut ScheduleGraph {
&mut self.graph
}

Copy link
Member

Choose a reason for hiding this comment

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

is this needed in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's needed to call build_schedule, otherwise you won't have the ambiguities

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.

bors r+

@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 17, 2023
@alice-i-cecile alice-i-cecile added this to the 0.10 milestone Feb 17, 2023
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.
@bors bors bot changed the title expose ambiguities of ScheduleGraph [Merged by Bors] - expose ambiguities of ScheduleGraph Feb 17, 2023
@bors bors bot closed this Feb 17, 2023
@jakobhellermann jakobhellermann deleted the schedule-expose-ambiguities branch February 17, 2023 10:10
bors bot pushed a commit that referenced this pull request Feb 17, 2023
follow-up to #7716

# Objective

System access is only populated in `System::initialize`, so without calling `initialize` it's actually impossible to see most ambiguities.

## Solution


- make `initialize` public. The method is idempotent, so calling it multiple times doesn't hurt
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.
myreprise1 pushed a commit to myreprise1/bevy that referenced this pull request Feb 18, 2023
follow-up to bevyengine#7716

# Objective

System access is only populated in `System::initialize`, so without calling `initialize` it's actually impossible to see most ambiguities.

## Solution


- make `initialize` public. The method is idempotent, so calling it multiple times doesn't hurt
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-Usability A targeted quality-of-life change that makes Bevy easier to use 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.

4 participants