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] - Resolve most remaining execution-order ambiguities #6341

Closed
wants to merge 18 commits into from

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Oct 22, 2022

Objective

Bevy's internal plugins have lots of execution-order ambiguities, which makes the ambiguity detection tool very noisy for our users.

Solution

Silence every last ambiguity that can currently be resolved.
Each time an ambiguity is silenced, it is accompanied by a comment describing why it is correct. This description should be based on the public API of the respective systems. Thus, I have added documentation to some systems describing how they use some resources.

Future work

Some ambiguities remain, due to issues out of scope for this PR.

  • The ambiguity checker does not respect Without<> filters, leading to false positives.
  • Ambiguities between bevy_ui and bevy_animation cannot be resolved, since neither crate knows that the other exists. We will need a general solution to this problem.

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change labels Oct 22, 2022
@JoJoJet JoJoJet marked this pull request as ready for review October 24, 2022 14:05
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.

Needing to suppress Assets<Image> so many times makes me worried about how ambiguity detection will work at a larger scale. Hopefully won't be too bad as most games won't need to get mutable access to Assets too much. Either way not the concern of this PR.

crates/bevy_render/src/camera/projection.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/camera/projection.rs Outdated Show resolved Hide resolved
Co-authored-by: Mike <mike.hsu@gmail.com>
@JoJoJet JoJoJet closed this Oct 25, 2022
@JoJoJet JoJoJet reopened this Oct 25, 2022
@JoJoJet
Copy link
Member Author

JoJoJet commented Oct 25, 2022

Misclick, my bad.

@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 Oct 25, 2022
@alice-i-cecile
Copy link
Member

Yeah the problem with Assets is that we need to have a pattern for "order independent writes".

@alice-i-cecile alice-i-cecile added this to the Bevy 0.9 milestone Oct 25, 2022
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Oct 27, 2022
# Objective

Bevy's internal plugins have lots of execution-order ambiguities, which makes the ambiguity detection tool very noisy for our users.

## Solution

Silence every last ambiguity that can currently be resolved.
Each time an ambiguity is silenced, it is accompanied by a comment describing why it is correct. This description should be based on the public API of the respective systems. Thus, I have added documentation to some systems describing how they use some resources.

# Future work

Some ambiguities remain, due to issues out of scope for this PR. 

* The ambiguity checker does not respect `Without<>` filters, leading to false positives.
* Ambiguities between `bevy_ui` and `bevy_animation` cannot be resolved, since neither crate knows that the other exists. We will need a general solution to this problem.
@bors bors bot changed the title Resolve most remaining execution-order ambiguities [Merged by Bors] - Resolve most remaining execution-order ambiguities Oct 27, 2022
@bors bors bot closed this Oct 27, 2022
@JoJoJet JoJoJet deleted the fix-ambiguities branch October 27, 2022 13:18
@mockersf mockersf added the hacktoberfest-accepted A PR that was accepted for Hacktoberfest, an annual open source event label Oct 27, 2022
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

Bevy's internal plugins have lots of execution-order ambiguities, which makes the ambiguity detection tool very noisy for our users.

## Solution

Silence every last ambiguity that can currently be resolved.
Each time an ambiguity is silenced, it is accompanied by a comment describing why it is correct. This description should be based on the public API of the respective systems. Thus, I have added documentation to some systems describing how they use some resources.

# Future work

Some ambiguities remain, due to issues out of scope for this PR. 

* The ambiguity checker does not respect `Without<>` filters, leading to false positives.
* Ambiguities between `bevy_ui` and `bevy_animation` cannot be resolved, since neither crate knows that the other exists. We will need a general solution to this problem.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

Bevy's internal plugins have lots of execution-order ambiguities, which makes the ambiguity detection tool very noisy for our users.

## Solution

Silence every last ambiguity that can currently be resolved.
Each time an ambiguity is silenced, it is accompanied by a comment describing why it is correct. This description should be based on the public API of the respective systems. Thus, I have added documentation to some systems describing how they use some resources.

# Future work

Some ambiguities remain, due to issues out of scope for this PR. 

* The ambiguity checker does not respect `Without<>` filters, leading to false positives.
* Ambiguities between `bevy_ui` and `bevy_animation` cannot be resolved, since neither crate knows that the other exists. We will need a general solution to this problem.
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 A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change hacktoberfest-accepted A PR that was accepted for Hacktoberfest, an annual open source event 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.

None yet

4 participants