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

Composable builder pattern for system construction and stage configuration #2510

Closed
alice-i-cecile opened this issue Jul 20, 2021 · 3 comments
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Duplicate This issue or PR already exists

Comments

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jul 20, 2021

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

As the number of different ways to add and configure stages grows, the number of add_system_* variants is undergoing an exponential explosion.

add_startup_system_set_to_stage is not exactly the nicest to write, it bloats our docs and code base, and it isn't composable. We've also had several issues where we've missed one of the variants, completely blocking end users for no good reason.

What solution would you like?

All system properties should be stored on the SystemDescriptor itself, added in a post-fix builder fashion in the same way that labels are.

Instead of add_startup_system_set_to_stage(my_system_set, StartupStage::PreStartup), we have add_system_set(my_system_set.startup().in_stage(StartupStage::PreStartup)).

From an implementation perspective, many of the methods on AppBuilder (see this file) will need to be deprecated.

In exchange, a number of methods will need to be added to ParallelSystemDescriptorCoercion and its exclusive system sibling.

What alternative(s) have you considered?

One nice perk of this design is that it allows the stage that a system is inserted into to be stored as static data.

This means that this work could reasonably be done as part of a refactor where we take a more structured approach to storing App and Plugin data. Clearly structured fields (rather than endless method chaining) would improve clarity / configurability / composability / interop and solve #1255.

In the interests of making these changes easier to review and implement though, I think that this should be tackled first, as it is a prerequisite to that work and should be relatively uncontroversial.

Additional context

AppBuilder is planned to be deprecated for 0.6 as part of the rendering overhaul; collapsed down to just methods on App.

@Ratysz also has plans to simplify and improve the scheduler API, beginning with bevyengine/rfcs#29.

I am unsure which order this issue should be tackled in relative to those two major overhauls mentioned.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events S-Blocked This cannot move forward until something else changes C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Blocked This cannot move forward until something else changes labels Jul 20, 2021
@alice-i-cecile
Copy link
Member Author

The initial set of things we may want to configure are:

  • stage
  • startup vs not startup

These should work for both systems and system sets.

@Nilirad
Copy link
Contributor

Nilirad commented Jul 21, 2021

Is there a way to avoid hardcoding references to the startup schedule in the API? While it is added by default by App::build, it is still possible to design an app schedule from scratch.

The API would look like this, directly referencing the stage by label:

add_system_set(
    my_system_set
        .in_stage(StartupStage::PreStartup)
)

@alice-i-cecile
Copy link
Member Author

Closing as a duplicate of #1978...

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-Duplicate This issue or PR already exists
Projects
None yet
Development

No branches or pull requests

2 participants