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

Relaxed (if-needed) ordering constraints #47

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

alice-i-cecile
Copy link
Member

RENDERED

Extracted from #45.

Summary

If-needed ordering constraints allow for safer, non-blocking constraints betweeen groups of systems.
If they would have no effect, they're completely ignored.
If they have an effect, they behave identically to strict ordering constraints.

Motivation

The stageless rework (RFC #45) allows users to configure entire labels, creating ordering dependencies between entire subgraphs.
However, in many cases, pairs of systems ordered in this way have no logical connection between them (and do not even access the same data).

The current ordering, called strict ordering, will still force these pairs of systems to run according to the specified ordering, pointlessly restricting scheduling flexibility and thus performance.

@alice-i-cecile alice-i-cecile changed the title Port explanations from Stageless RFC If-needed ordering constraints Jan 20, 2022
Co-authored-by: MiniaczQ <[email protected]>
@MiniaczQ
Copy link

Can there be a situation where 2 systems need a relative ordering if they share no common Params?
Perhaps strict ordering can be omitted between systems of such nature.

@alice-i-cecile
Copy link
Member Author

Yes; interior mutability and transitive dependencies.

@MiniaczQ
Copy link

I was going to propose something similar after discord discussion of similar topic, so I'm glad I stumbled here.
Definitely a great idea to make this the default way of adding systems (I only thought of warning about too restrictive ordering).
The only thing that I'd change is the naming, which didn't immediately click for me.
I propose renaming:

  • strict to forced, since it'd be forcing an ordering between 'unrelated' systems.
  • if-needed to lax or relaxed, since it's not a binary problem whether the constraint exists, transitivity can keep the constraint, but change the systems involved.

@alice-i-cecile
Copy link
Member Author

I like "relaxed" quite a bit: it's a good contrast to "strict", and much more approachable.

I don't think "forced" explains the concept any better: I personally think "strict" captures the essence a bit better: the idea is "yes, for real, I know better than you".

@alice-i-cecile alice-i-cecile changed the title If-needed ordering constraints Relaxed (if-needed) ordering constraints Mar 13, 2022
@JoJoJet
Copy link
Member

JoJoJet commented Jun 28, 2022

My bikeshed opinion: how about "fixed" and "relaxed"? If someone explicitly declares an ordering between two systems, I think that by default we should adhere to that, so we shouldn't be using negative language like "strict" or "forced". In cases when a label is only there to clear up potential ambiguities, you could explicitly mark it as "relaxed" to allow for faster scheduling.

@alice-i-cecile
Copy link
Member Author

Strict has a pretty standard (non-negative) meaning in math and computer science: it typically contrasts to relaxed, partial, or loose and has an implication of being the more maximal version of this. I'm not sure that it's immediately clear what a "fixed ordering constraint" does, while it feels like a "strict ordering constraint" is clearer.

I would be happy with "strong / weak ordering constraints" instead though. Is that clearer to you?

@JoJoJet
Copy link
Member

JoJoJet commented Jun 29, 2022

Seems reasonable, "strict/relaxed" works. Definitely prefer it over "strong/weak".

Comment on lines +152 to +153
4. `apply_player_movement` is after `collision_handling`
1. Spurious: these systems are compatible.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these systems compatible? Since you cannot reason about what apply_player_movement exactly does to Velocity, it should be treated as "reads and writes Velocity".
Imagine the following:

fn apply_player_movement(_: Res<&PlayerIntent>, velocity: Query<(&mut Velocity, With<Player>)>) {
  if velocity > 5 {
    // doesn't really matter what happens here,
    // it branches based on potentially stale data if this is considered spurious
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Open PRs
Development

Successfully merging this pull request may close these issues.

None yet

4 participants