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

Add a straightforward API for simple system ordering #4220

Closed
alice-i-cecile opened this issue Mar 15, 2022 · 6 comments
Closed

Add a straightforward API for simple system ordering #4220

alice-i-cecile opened this issue Mar 15, 2022 · 6 comments
Labels
A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use
Milestone

Comments

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Mar 15, 2022

What problem does this solve or what need does it fill?

Creating simple orderings between systems is harder than it should be. Users must define a label, and then use a .before or .after method.

In many cases, we have two (or maybe a few more) systems that need to be linearly ordered, which are defined in the same plugin.

This is both verbose and harder to read / reason about

What solution would you like?

app.add_system_set(system_a.then(system_b))

The .then method on IntoSystemDescriptor converts a system function into a SystemSet.

What alternative(s) have you considered?

System set chaining

app.add_system_set(systems![
  my_system,
  my_other_system
].chain())

Adapted from https://github.com/bevyengine/rfcs/pull/45/files#diff-d319c5378a207f7eae8b553b1df6b026d7bfc885b016f776a64b48ed86935d5fR769

System graph

We could use a full system graph API, as defined in #2381 (and the bevy_system_graph crate that came out of this).

This is probably too complex and unwieldy for this very simple but common case, but may be desirable later regardless.

add_system .then

Naively, a mechanism that works like the current .chain seems desirable.

app.add_system(system_a.then(my_other_system))

Unfortunately, this won't work under the hood: add_system expects a single system.

Additional context

Related to #4219, which proposes additional ergonomics improvements.

We may want to rename add_system_set to add_systems, following the suggestions in the Stageless RFC, to improve ergonomics. This should probably be done in a follow-up PR to improve reviewability.

I also really really want label-less ordering apis
Something like system_a.after(system_b)
would cover like 95% of system ordering use cases

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use labels Mar 15, 2022
@MrGVSV
Copy link
Member

MrGVSV commented Mar 15, 2022

Should/could this work within a SystemSet itself?

For example:

app.add_system_set(SystemSet::on_update(MyState::InGame).with_system(system_a.then(system_b));

@alice-i-cecile
Copy link
Member Author

Should/could this work within a SystemSet itself?

I don't think this is a super important use case, but I think that it'll just happen automatically. If my intuition is right, I think it's fine to allow it to continue to work, rather than to try to explicitly forbid it.

@james7132
Copy link
Member

We could use a full system graph API, as defined in #2381 (and the bevy_system_graph crate that came out of this).
This is probably too complex and unwieldy for this very simple but common case, but may be desirable later regardless.

One idea that came out of the Discord discussion that spawned this is a IntoSystemGraph coercion trait implemented on FunctionSystem or something similar that supports then and fork APIs proposed. There would need to be a similar coercion for getting it to insert the entire graph in at once too.

bors bot pushed a commit that referenced this issue Mar 23, 2022
This adds the concept of "default labels" for systems (currently scoped to "parallel systems", but this could just as easily be implemented for "exclusive systems"). Function systems now include their function's `SystemTypeIdLabel` by default.

This enables the following patterns:

```rust
// ordering two systems without manually defining labels
app
  .add_system(update_velocity)
  .add_system(movement.after(update_velocity))

// ordering sets of systems without manually defining labels
app
  .add_system(foo)
  .add_system_set(
    SystemSet::new()
      .after(foo)
      .with_system(bar)
      .with_system(baz)
  )
```

Fixes: #4219
Related to: #4220 

Credit to @aevyrie @alice-i-cecile @DJMcNab (and probably others) for proposing (and supporting) this idea about a year ago. I was a big dummy that both shut down this (very good) idea and then forgot I did that. Sorry. You all were right!
@alice-i-cecile alice-i-cecile added D-Trivial Nice and easy! A great choice to get started with Bevy and removed D-Trivial Nice and easy! A great choice to get started with Bevy labels May 12, 2022
aevyrie pushed a commit to aevyrie/bevy that referenced this issue Jun 7, 2022
This adds the concept of "default labels" for systems (currently scoped to "parallel systems", but this could just as easily be implemented for "exclusive systems"). Function systems now include their function's `SystemTypeIdLabel` by default.

This enables the following patterns:

```rust
// ordering two systems without manually defining labels
app
  .add_system(update_velocity)
  .add_system(movement.after(update_velocity))

// ordering sets of systems without manually defining labels
app
  .add_system(foo)
  .add_system_set(
    SystemSet::new()
      .after(foo)
      .with_system(bar)
      .with_system(baz)
  )
```

Fixes: bevyengine#4219
Related to: bevyengine#4220 

Credit to @aevyrie @alice-i-cecile @DJMcNab (and probably others) for proposing (and supporting) this idea about a year ago. I was a big dummy that both shut down this (very good) idea and then forgot I did that. Sorry. You all were right!
@alice-i-cecile
Copy link
Member Author

Took a crack at the .then API. It's not going to be pretty, because of the strange architecture of all of our scheduling traits. If you want to take a look / build off it, feel free to steal from https://github.com/alice-i-cecile/bevy/tree/then-finally.

I don't think it's very feasible until #4391 (or equivalent cleanup) occurs.

ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
This adds the concept of "default labels" for systems (currently scoped to "parallel systems", but this could just as easily be implemented for "exclusive systems"). Function systems now include their function's `SystemTypeIdLabel` by default.

This enables the following patterns:

```rust
// ordering two systems without manually defining labels
app
  .add_system(update_velocity)
  .add_system(movement.after(update_velocity))

// ordering sets of systems without manually defining labels
app
  .add_system(foo)
  .add_system_set(
    SystemSet::new()
      .after(foo)
      .with_system(bar)
      .with_system(baz)
  )
```

Fixes: bevyengine#4219
Related to: bevyengine#4220 

Credit to @aevyrie @alice-i-cecile @DJMcNab (and probably others) for proposing (and supporting) this idea about a year ago. I was a big dummy that both shut down this (very good) idea and then forgot I did that. Sorry. You all were right!
@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Feb 16, 2023
@alice-i-cecile
Copy link
Member Author

This wasn't added in the stageless changes, but is now unblocked.

@cart
Copy link
Member

cart commented Feb 16, 2023

I feel like requiring chain for this might be the right call ((which we already have thanks to stageless). Adding multiple ways to express the same thing (and dealing with the likely after(foo) vs then(foo) confusion) seems like it might not be worth it.

@james7132 james7132 modified the milestones: 0.11, 0.10 Mar 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants