Remove all existing system order ambiguities in DefaultPlugins
#15031
Conversation
alice-i-cecile
added
C-Bug
alice-i-cecile
requested review from
mockersf,
janhohenheim,
NthTensor,
Vrixyz and
tychedelia
| app.ignore_ambiguity( | ||
| bevy_app::PostUpdate, | ||
| bevy_animation::animate_targets, | ||
| bevy_ui::ui_layout_system, | ||
| ); |
There was a problem hiding this comment.
I appreciate the previous context comments, that said I can understand they are difficult to balance for maintainability and correctness.
does that refer to all external ambiguities silenced ?
There was a problem hiding this comment.
This was justified using the comments just a few lines up (line 85), which state that UI cannot be animated :) I'd like to change that eventually of course!
There was a problem hiding this comment.
Very nice that you were able to get this to zero! I looked over it and all of this makes sense. Only thing I dislike about this is that allowing ambiguities between concrete systems seems somewhat brittle and inelegant, like using systems for ordering. Would it be feasible to change the ambiguity ignores to system sets? I'm also fine with leaving this for a follow up.
Re CI failure: the CTRL-C handler should probably be last
| @@ -226,7 +228,8 @@ fn build_text_interop(app: &mut App) { | |||
| .in_set(UiSystem::PostLayout) | |||
| .after(bevy_text::remove_dropped_font_atlas_sets) | |||
| // Text2d and bevy_ui text are entirely on separate entities | |||
| .ambiguous_with(bevy_text::update_text2d_layout), | |||
| .ambiguous_with(bevy_text::update_text2d_layout) | |||
| .ambiguous_with(bevy_text::calculate_bounds_text2d), | |||
There was a problem hiding this comment.
Maybe these two should be in a single set to ignore?
There was a problem hiding this comment.
Possibly: I wanted to keep the changes relatively minimal though.
| /// System order ambiguities between systems in this set are ignored: | ||
| /// each [`build_directional_light_cascades`] system is independent of the others, | ||
| /// and should operate on distinct sets of entities. | ||
| UpdateDirectionalLightCascades, |
There was a problem hiding this comment.
(out of scope for this PR)
is this an opportunity to add a macro to mark that ambiguity is allowed there ? (for better type safety, avoid outdated comments)
There was a problem hiding this comment.
Yeah, possibly. "Ignore all ambiguities with self in set" isn't a common pattern, but it's not a rare one.
I'm actually pretty happy with most of these fixes! The rendering sets are a bit weird and implicit, but I think ultimately correct. I'd like to make text and text2d play nicer with each other / be less distinct, but that's another big project. Same with making UI animatable. My main complaint is around |
alice-i-cecile
added
S-Ready-For-Final-Review
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Objective
As discussed in #7386, system order ambiguities within
DefaultPluginsare a source of bugs in the engine and badly pollute diagnostic output for users.We should eliminate them!
This PR is an alternative to #15027: with all external ambiguities silenced, this should be much less prone to merge conflicts and the test output should be much easier for authors to understand.
Note that system order ambiguities are still permitted in the
RenderApp: these need a bit of thought in terms of how to test them, and will be fairly involved to fix. While these aren't good, they'll generally only cause graphical bugs, not logic ones.Solution
All remaining system order ambiguities have been resolved.
Review this PR commit-by-commit to see how each of these problems were fixed.
Testing
cargo run --example ambiguity_detectionpasses with no panics or logging!