[Merged by Bors] - Resolve (most) internal system ambiguities #1606
Conversation
cart
commented
Mar 10, 2021
•
cart
commented
- Adds labels and orderings to systems that need them (uses the new many-to-many labels for InputSystem)
- Removes the Event, PreEvent, Scene, and Ui stages in favor of First, PreUpdate, and PostUpdate (there is more collapsing potential, such as the Asset stages and maybe removing First, but those have more nuance so they should be handled separately)
- Ambiguity detection now prints component conflicts
- Removed broken change filters from flex calculation (which implicitly relied on the z-update system always modifying translation.z). This will require more work to make it behave as expected so i just removed it (and it was already doing this work every frame).
cart
force-pushed
the
plugin-system-order
branch
from
330ad00
to
ab242e5
Compare
|
The remaining internal plugin ambiguities are now, without exception, "just noise". Resolving them does not fix any bugs. Additionally, resolving them increases implementation complexity (by sprinkling ambiguity sets everywhere), so I am personally inclined to not resolve them. |
Out of curiosity, do you have the output of this available? I'd like to see what remains, to improve my mental model of this problem space. |
There was a problem hiding this comment.
Additionally, resolving them increases implementation complexity (by sprinkling ambiguity sets everywhere), so I am personally inclined to not resolve them.
Ambiguity sets are not for "resolving" anything, they are strictly a measure to reduce the noise of the ambiguity checker.
I know you don't like them - I, frankly, don't like them right now either - but if we want to keep the checker and have it be not uselessly noisy, we have to reduce the noise somehow. Ambiguity sets are just the simplest tool for it that we can have right now: it's crude and it adds a layer of complexity (however uncomplicated it may be), but it does the job; it also doesn't have to be permanent.
I'm hoping for one of these possibilities:
- At some point during further API improvements (Simple improvements to ambiguity reporting clarity #1484, System organization overhaul and the road to a stageless schedule #1375) we stumble upon a way of doing this that isn't as unpleasant. I'm banking on mass label assignment: being able to say "I've verified everything in this plugin, don't bother checking it for internal ambiguities" with one or two lines seems sufficiently nice.
- After initial adoption period of 0.5 has passed we discover that the fears that lead to ambiguity checker's creation were completely unfounded, and the average user has no trouble grokking implications of "execution order is not defined unless you define it". Which means we can throw away this incarnation of the checker completely and stop worrying about it.
Out of curiosity, do you have the output of this available? I'd like to see what remains, to improve my mental model of this problem space.
(You should be able to generate that yourself: checkout this branch, .insert_resource(bevy::ecs::schedule::ReportExecutionOrderAmbiguities) on whichever example's app builder, run.)
It's... quite a few. We do have to reduce the noise.
Co-authored-by: Alexander Sepity <[email protected]>
I agree with every part of this comment :p Those warnings from breakout are very noisy, and can't be fixed by the end user without forking Bevy.
Once #68 is solved this seems much more likely thankfully. Frame delays aren't great, but don't completely break games in the way that missing changes does. |
I don't know. Sure, not breaking is great, but a hidden loss of resolution speed is, well, a pretty insidious issue. It's one thing when users' own systems are doing it (user error, define explicit order), but engine's systems telling you that it's fine when it's not always fine is another. |
|
Most of the remaining ambiguous systems are a class of "thing" where a mutation occurs across systems, but the order doesn't matter. This could be any set-like operation (HashMap, Assets, RenderPipelines, etc), a work queue where the queue order doesn't matter (SharedBuffers), or any other "commutative" operation. Manually ordering these types of systems is a non-starter for me. Users (and internal Bevy developers) using Likewise, ambiguity sets as a permanent solution are also a non-starter for me. Users shouldn't need to add every As a short term solution, I'd be fine with adding ambiguity sets here. But I honestly don't doubt that the ambiguity checker has value as a debugging tool. I've found it useful while resolving ambiguities and users have already self-reported its value. If the assertion is that "the ambiguity checker cannot have internal bevy noise", and the ambiguity checker is a useful debugging tool, then we are implicitly committing to continue resolving all ambiguities (which I do have a problem with). |
Can we split what the ambiguity reporter reports up by origin? E.g. all of the Bevy internal conflicts in one pile, and all of the user conflicts in another, and all of the user-Bevy conflicts in a third? These could either be shown in separate regions of the report, or completely silenced en-masse by modifying the value of the "enable ambiguity checker resource". |
|
Was about to voice that idea. It seems like it'll let us both keep the checker and Re: this PR: I think it can be merged as-is. The only solution to silencing internal ambiguities we can apply right now seems rather painful; might be best to revisit it separately, with more tools - I'm already working on most of those I've mentioned. |
Haha one of the hazards of a modular design is that theres no real distinction between internal systems and third-party systems. I guess we could just add a list of crates and do a str::starts_with on the system name? Anyway, yeah I'm inclined to merge this as-is and continue the "ambiguity noise" discussion elsewhere. I'll resolve the remaining comments. |
|
bors r+ |
* Adds labels and orderings to systems that need them (uses the new many-to-many labels for InputSystem) * Removes the Event, PreEvent, Scene, and Ui stages in favor of First, PreUpdate, and PostUpdate (there is more collapsing potential, such as the Asset stages and _maybe_ removing First, but those have more nuance so they should be handled separately) * Ambiguity detection now prints component conflicts * Removed broken change filters from flex calculation (which implicitly relied on the z-update system always modifying translation.z). This will require more work to make it behave as expected so i just removed it (and it was already doing this work every frame).
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
# Objective Previously, the gilrs system had no explicit relationship to the input systems. This could potentially cause it to be scheduled after the input systems, leading to a one-frame lag in gamepad inputs. This was a regression introduced in #1606 which removed the `PreEvent` stage ## Solution This adds an explicit `before` relationship to the gilrs plugin, ensuring that raw gamepad events will be processed on the same frame that they are generated.
