[Merged by Bors] - Remove ambiguity sets #5916

alice-i-cecile · 2022-09-08T19:03:36Z

Objective

Ambiguity sets are used to ignore system order ambiguities between groups of systems. However, they are not very useful: they are clunky, poorly integrated, and generally hampered by the difficulty using (or discovering) the ambiguity detector.

As a first step to the work in #4299, we're removing them.

Migration Guide

Ambiguity sets have been removed.

alice-i-cecile · 2022-09-08T19:04:27Z

crates/bevy_ecs/src/schedule/stage.rs

@@ -1607,392 +1594,6 @@ mod tests {
        stage.run(&mut world);
    }

-    #[test]
-    fn ambiguity_detection() {


This test was removed, rather than fixed, because it was generally a disaster to debug / refactor / use. More tests will be added in follow-up PRs, promise.

that makes me quite sad

alice-i-cecile · 2022-09-08T19:05:31Z

Note to reviewers: this PR is split into two commits. The first is trivial compiler-error chasing; the second is actually tweaking the algorithm.

mockersf

I think it would have been better to keep some kind of tests in this PR, but ambiguity detection is not critical enough to block on this

hymm

agree that it's unfortunate that all the tests for ambiguity detection is gone, but not going to block on it.

alice-i-cecile · 2022-09-09T17:21:34Z

bors r+

# Objective Ambiguity sets are used to ignore system order ambiguities between groups of systems. However, they are not very useful: they are clunky, poorly integrated, and generally hampered by the difficulty using (or discovering) the ambiguity detector. As a first step to the work in #4299, we're removing them. ## Migration Guide Ambiguity sets have been removed.

bors · 2022-09-09T17:37:37Z

Pull request successfully merged into main.

Build succeeded:

# Objective This code is very disjoint, and the `stage.rs` file that it's in is already very long. All I've done is move the code and clean up the compiler errors that result. Followup to #5916, split out from #4299.

# Objective Ambiguity sets are used to ignore system order ambiguities between groups of systems. However, they are not very useful: they are clunky, poorly integrated, and generally hampered by the difficulty using (or discovering) the ambiguity detector. As a first step to the work in bevyengine#4299, we're removing them. ## Migration Guide Ambiguity sets have been removed.

# Objective This code is very disjoint, and the `stage.rs` file that it's in is already very long. All I've done is move the code and clean up the compiler errors that result. Followup to bevyengine#5916, split out from bevyengine#4299.

# Background Incremental implementation of #4299. The code is heavily borrowed from that PR. # Objective The execution order ambiguity checker often emits false positives, since bevy is not aware of invariants upheld by the user. ## Solution Title --- ## Changelog + Added methods `SystemDescriptor::ignore_all_ambiguities` and `::ambiguous_with`. These allow you to silence warnings for specific system-order ambiguities. ## Migration Guide ***Note for maintainers**: This should replace the migration guide for #5916* Ambiguity sets have been replaced with a simpler API. ```rust // These systems technically conflict, but we don't care which order they run in. fn jump_on_click(mouse: Res<Input<MouseButton>>, mut transforms: Query<&mut Transform>) { ... } fn jump_on_spacebar(keys: Res<Input<KeyCode>>, mut transforms: Query<&mut Transform>) { ... } // // Before #[derive(AmbiguitySetLabel)] struct JumpSystems; app .add_system(jump_on_click.in_ambiguity_set(JumpSystems)) .add_system(jump_on_spacebar.in_ambiguity_set(JumpSystems)); // // After app .add_system(jump_on_click.ambiguous_with(jump_on_spacebar)) .add_system(jump_on_spacebar); ```

…#6158) # Background Incremental implementation of bevyengine#4299. The code is heavily borrowed from that PR. # Objective The execution order ambiguity checker often emits false positives, since bevy is not aware of invariants upheld by the user. ## Solution Title --- ## Changelog + Added methods `SystemDescriptor::ignore_all_ambiguities` and `::ambiguous_with`. These allow you to silence warnings for specific system-order ambiguities. ## Migration Guide ***Note for maintainers**: This should replace the migration guide for bevyengine#5916* Ambiguity sets have been replaced with a simpler API. ```rust // These systems technically conflict, but we don't care which order they run in. fn jump_on_click(mouse: Res<Input<MouseButton>>, mut transforms: Query<&mut Transform>) { ... } fn jump_on_spacebar(keys: Res<Input<KeyCode>>, mut transforms: Query<&mut Transform>) { ... } // // Before #[derive(AmbiguitySetLabel)] struct JumpSystems; app .add_system(jump_on_click.in_ambiguity_set(JumpSystems)) .add_system(jump_on_spacebar.in_ambiguity_set(JumpSystems)); // // After app .add_system(jump_on_click.ambiguous_with(jump_on_spacebar)) .add_system(jump_on_spacebar); ```

# Objective Ambiguity sets are used to ignore system order ambiguities between groups of systems. However, they are not very useful: they are clunky, poorly integrated, and generally hampered by the difficulty using (or discovering) the ambiguity detector. As a first step to the work in bevyengine#4299, we're removing them. ## Migration Guide Ambiguity sets have been removed.

# Objective This code is very disjoint, and the `stage.rs` file that it's in is already very long. All I've done is move the code and clean up the compiler errors that result. Followup to bevyengine#5916, split out from bevyengine#4299.

…#6158) # Background Incremental implementation of bevyengine#4299. The code is heavily borrowed from that PR. # Objective The execution order ambiguity checker often emits false positives, since bevy is not aware of invariants upheld by the user. ## Solution Title --- ## Changelog + Added methods `SystemDescriptor::ignore_all_ambiguities` and `::ambiguous_with`. These allow you to silence warnings for specific system-order ambiguities. ## Migration Guide ***Note for maintainers**: This should replace the migration guide for bevyengine#5916* Ambiguity sets have been replaced with a simpler API. ```rust // These systems technically conflict, but we don't care which order they run in. fn jump_on_click(mouse: Res<Input<MouseButton>>, mut transforms: Query<&mut Transform>) { ... } fn jump_on_spacebar(keys: Res<Input<KeyCode>>, mut transforms: Query<&mut Transform>) { ... } // // Before #[derive(AmbiguitySetLabel)] struct JumpSystems; app .add_system(jump_on_click.in_ambiguity_set(JumpSystems)) .add_system(jump_on_spacebar.in_ambiguity_set(JumpSystems)); // // After app .add_system(jump_on_click.ambiguous_with(jump_on_spacebar)) .add_system(jump_on_spacebar); ```

# Objective Ambiguity sets are used to ignore system order ambiguities between groups of systems. However, they are not very useful: they are clunky, poorly integrated, and generally hampered by the difficulty using (or discovering) the ambiguity detector. As a first step to the work in bevyengine#4299, we're removing them. ## Migration Guide Ambiguity sets have been removed.

# Objective This code is very disjoint, and the `stage.rs` file that it's in is already very long. All I've done is move the code and clean up the compiler errors that result. Followup to bevyengine#5916, split out from bevyengine#4299.

…#6158) # Background Incremental implementation of bevyengine#4299. The code is heavily borrowed from that PR. # Objective The execution order ambiguity checker often emits false positives, since bevy is not aware of invariants upheld by the user. ## Solution Title --- ## Changelog + Added methods `SystemDescriptor::ignore_all_ambiguities` and `::ambiguous_with`. These allow you to silence warnings for specific system-order ambiguities. ## Migration Guide ***Note for maintainers**: This should replace the migration guide for bevyengine#5916* Ambiguity sets have been replaced with a simpler API. ```rust // These systems technically conflict, but we don't care which order they run in. fn jump_on_click(mouse: Res<Input<MouseButton>>, mut transforms: Query<&mut Transform>) { ... } fn jump_on_spacebar(keys: Res<Input<KeyCode>>, mut transforms: Query<&mut Transform>) { ... } // // Before #[derive(AmbiguitySetLabel)] struct JumpSystems; app .add_system(jump_on_click.in_ambiguity_set(JumpSystems)) .add_system(jump_on_spacebar.in_ambiguity_set(JumpSystems)); // // After app .add_system(jump_on_click.ambiguous_with(jump_on_spacebar)) .add_system(jump_on_spacebar); ```

alice-i-cecile added 2 commits September 8, 2022 14:59

Remove basic machinery

5d5900c

Remove ambiguity sets from algorithm

edd8129

alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Sep 8, 2022

alice-i-cecile commented Sep 8, 2022

View reviewed changes

maniwani self-requested a review September 8, 2022 19:13

alice-i-cecile requested a review from hymm September 8, 2022 20:05

alice-i-cecile mentioned this pull request Sep 8, 2022

[Merged by Bors] - Move ambiguity detection into its own file #5918

Closed

alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Sep 8, 2022

mockersf approved these changes Sep 8, 2022

View reviewed changes

hymm approved these changes Sep 9, 2022

View reviewed changes

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 Sep 9, 2022

bors bot changed the title ~~Remove ambiguity sets~~ [Merged by Bors] - Remove ambiguity sets Sep 9, 2022

bors bot closed this Sep 9, 2022

joseph-gio mentioned this pull request Oct 3, 2022

[Merged by Bors] - Add methods for silencing system-order ambiguity warnings #6158

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Remove ambiguity sets #5916

[Merged by Bors] - Remove ambiguity sets #5916

alice-i-cecile commented Sep 8, 2022 •

edited

Loading

alice-i-cecile Sep 8, 2022

mockersf Sep 8, 2022

alice-i-cecile commented Sep 8, 2022

mockersf left a comment

hymm left a comment

alice-i-cecile commented Sep 9, 2022

bors bot commented Sep 9, 2022

[Merged by Bors] - Remove ambiguity sets #5916

[Merged by Bors] - Remove ambiguity sets #5916

Conversation

alice-i-cecile commented Sep 8, 2022 • edited Loading

Objective

Migration Guide

alice-i-cecile Sep 8, 2022

Choose a reason for hiding this comment

mockersf Sep 8, 2022

Choose a reason for hiding this comment

alice-i-cecile commented Sep 8, 2022

mockersf left a comment

Choose a reason for hiding this comment

hymm left a comment

Choose a reason for hiding this comment

alice-i-cecile commented Sep 9, 2022

bors bot commented Sep 9, 2022

alice-i-cecile commented Sep 8, 2022 •

edited

Loading