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 API for simple system ordering #5165

Closed

Conversation

harudagondi
Copy link
Member

@harudagondi harudagondi commented Jul 1, 2022

Objective

Solution

  • Create new traits IntoLinkedSystemSet and IntoSequentialSystemSet.
  • Using these trait methods will produce a SystemSet that can be added to App through App::add_system_set.

IntoLinkedSystemSet

  • IntoLinkedSystemSet is a trait that provides a method link().
  • IntoLinkedSystemSet is implemented on tuples of systems containing 2 to 15 elements.

Syntax:

(system_a, system_b, system_c, system_d).link()

This is equal to:

SystemSet::new()
    .with_system(system_a)
    .with_system(system_b.after(system_a))
    .with_system(system_c.after(system_b))
    .with_system(system_d.after(system_c));

IntoSequentialSystemSet

  • IntoSequentialSystemSet is trait that provides a method then(next_system)
  • IntoSequentialSystemSet is implemented on a system, and accepts a system.

Syntax:

system_a.then(system_b)

This is equal to:

SystemSet::new()
    .with_system(system_a)
    .with_system(system_b.after(system_a));

Or

(system_a, system_b).link()

Known Limitations

  • IntoSequentialSystemSet is not implemented for SystemSet. This means you can't do system_a.then(system_b).then(system_c).
  • IntoLinkedSystemSet is not implemented for tuples with more than 15 elements.

Changelog

Added

  • Allow sequential ordering of two systems by using IntoSequentialSystemSet::then and doing system_a.then(system_b).
  • Allow sequential ordering of multiple systems (up to 15) by using IntoLinkedSystemSet::link on tuples of systems.

P.S.: writing and debugging proc macros is not a very fun experience lol

@Nilirad Nilirad added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jul 1, 2022
Copy link
Contributor

@Nilirad Nilirad left a comment

Choose a reason for hiding this comment

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

Some doc nits. I'm not confident enough with macros and system internals to do a full review though.

///
/// [`.link()`]: IntoLinkedSystemSet::link
pub trait IntoLinkedSystemSet<S, P> {
/// A helper method to create system sets with automatic
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// A helper method to create system sets with automatic
/// A helper method to create system sets with sequential

///
/// [`.then()`]: IntoSequentialSystemSet::then
pub trait IntoSequentialSystemSet<S0, S1, P0, P1> {
/// A helper method to create system sets with automatic
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// A helper method to create system sets with automatic
/// A helper method to create system sets with sequential

@alice-i-cecile alice-i-cecile self-requested a review July 1, 2022 14:45
@alice-i-cecile
Copy link
Member

Nice work! I'll do a full review soon. Did you experiment with the .then() syntax as well?

@MrGVSV
Copy link
Member

MrGVSV commented Jul 1, 2022

I’m not up-to-date on bevyengine/rfcs#45 but isn't the terminology chain instead of link?

@alice-i-cecile
Copy link
Member

Yes, but to do that we'll need to first rename the existing system chaining :)

@james7132
Copy link
Member

james7132 commented Jul 2, 2022

This seems awfully similar to earlier iterations of #2381 before it was extended to support fan-in/fan-out topologies: strictly sequential, no graph ordering. While the then syntax could be logically extended to support those kinds of use cases, the link approach may not.

@harudagondi
Copy link
Member Author

Hello, sorry for the late response. I was not aware of #2381 and bevyengine/rfcs#45, whoops sorry. In hindsight, this is a very naive way of implementing sequential chains of system. I honestly think that there should be an easier way of making a linear sequential order of systems, but with the rfc I probably think it would be for the best that we should wait until stageless is implemented. However, I think the proc-macro I made for this should be useful in implementing system![].chain() or chain![].

@alice-i-cecile alice-i-cecile added S-Blocked This cannot move forward until something else changes labels Jul 4, 2022
@alice-i-cecile
Copy link
Member

Closing as this has been done in the stageless rework.

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 targeted quality-of-life change that makes Bevy easier to use S-Blocked This cannot move forward until something else changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a straightforward API for simple system ordering
5 participants