[Merged by Bors] - Reduce internal system order ambiguities, and add an example explaining them #7383
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alice-i-cecile
added
A-ECS
4 tasks
MinerSebas
reviewed
Jan 27, 2023
hymm
reviewed
Jan 27, 2023
alice-i-cecile
added
C-Bug
hymm
approved these changes
Jan 28, 2023
joseph-gio
approved these changes
Jan 28, 2023
joseph-gio
added
the
S-Ready-For-Final-Review
MinerSebas
suggested changes
Jan 28, 2023
alice-i-cecile
changed the title
MinerSebas
reviewed
Jan 29, 2023
|
This should be mergeable now. I decided to use AppExit: it bypasses the linked bug, but also makes for a better learning tool. |
MinerSebas
approved these changes
Jan 29, 2023
mockersf
reviewed
Jan 29, 2023
mockersf
reviewed
Jan 29, 2023
mockersf
reviewed
Jan 29, 2023
jdm
approved these changes
Jan 29, 2023
Austreelis
approved these changes
Jan 30, 2023
Co-authored-by: Austreelis <[email protected]>
joseph-gio
approved these changes
Jan 30, 2023
alice-i-cecile
added
the
S-Ready-For-Final-Review
|
May I please have one fiinal LGTM for the last round of changes before I merge? I'd rather like to unblock the stageless migration ambiguity fixing. |
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Jan 31, 2023
…ng them (#7383) # Objective - Bevy should not have any "internal" execution order ambiguities. These clutter the output of user-facing error reporting, and can result in nasty, nondetermistic, very difficult to solve bugs. - Verifying this currently involves repeated non-trivial manual work. ## Solution - [x] add an example to quickly check this - ~~[ ] ensure that this example panics if there are any unresolved ambiguities~~ - ~~[ ] run the example in CI 😈~~ There's one tricky ambiguity left, between UI and animation. I don't have the tools to fix this without system set configuration, so the remaining work is going to be left to #7267 or another PR after that. ``` 2023-01-27T18:38:42.989405Z INFO bevy_ecs::schedule::ambiguity_detection: Execution order ambiguities detected, you might want to add an explicit dependency relation between some of these systems: * Parallel systems: -- "bevy_animation::animation_player" and "bevy_ui::flex::flex_node_system" conflicts: ["bevy_transform::components::transform::Transform"] ``` ## Changelog Resolved internal execution order ambiguities for: 1. Transform propagation (ignored, we need smarter filter checking). 2. Gamepad processing (fixed). 3. bevy_winit's window handling (fixed). 4. Cascaded shadow maps and perspectives (fixed). Also fixed a desynchronized state bug that could occur when the `Window` component is removed and then added to the same entity in a single frame.
|
Pull request successfully merged into main. Build succeeded:
|
bors
bot
changed the title
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
…ng them (bevyengine#7383) # Objective - Bevy should not have any "internal" execution order ambiguities. These clutter the output of user-facing error reporting, and can result in nasty, nondetermistic, very difficult to solve bugs. - Verifying this currently involves repeated non-trivial manual work. ## Solution - [x] add an example to quickly check this - ~~[ ] ensure that this example panics if there are any unresolved ambiguities~~ - ~~[ ] run the example in CI 😈~~ There's one tricky ambiguity left, between UI and animation. I don't have the tools to fix this without system set configuration, so the remaining work is going to be left to bevyengine#7267 or another PR after that. ``` 2023-01-27T18:38:42.989405Z INFO bevy_ecs::schedule::ambiguity_detection: Execution order ambiguities detected, you might want to add an explicit dependency relation between some of these systems: * Parallel systems: -- "bevy_animation::animation_player" and "bevy_ui::flex::flex_node_system" conflicts: ["bevy_transform::components::transform::Transform"] ``` ## Changelog Resolved internal execution order ambiguities for: 1. Transform propagation (ignored, we need smarter filter checking). 2. Gamepad processing (fixed). 3. bevy_winit's window handling (fixed). 4. Cascaded shadow maps and perspectives (fixed). Also fixed a desynchronized state bug that could occur when the `Window` component is removed and then added to the same entity in a single frame.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Solution
[ ] ensure that this example panics if there are any unresolved ambiguities[ ] run the example in CI 😈There's one tricky ambiguity left, between UI and animation. I don't have the tools to fix this without system set configuration, so the remaining work is going to be left to #7267 or another PR after that.
Changelog
Resolved internal execution order ambiguities for:
Also fixed a desynchronized state bug that could occur when the
Windowcomponent is removed and then added to the same entity in a single frame.