-
Notifications
You must be signed in to change notification settings - Fork 66
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
System builder syntax rework #31
Closed
Closed
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
# Feature Name: (fill me in with a unique ident, `system_builder_syntax`) | ||
|
||
## Summary | ||
|
||
Rewrite the system creation workflow to remove the excess of `add_startup_system_to_stage`-like methods and instead use the `system.stage().label().startup()` builder-like system. | ||
|
||
## Motivation | ||
|
||
Mostly described in [#1978](https://github.com/bevyengine/bevy/issues/1978). | ||
|
||
- Allows us to unify the existing `.label()`, `.after()`, etc. methods and the stage positioning of systems. | ||
- App would only have 3 methods to add systems: `add_system`, `add_exclusive` and `add_system_set`. | ||
- Arguably more intuitive design (somewhat backed up by those who commented on #1978). | ||
|
||
## User-facing explanation | ||
|
||
- `add_startup_system(system)` becomes `add_system(system.startup())` | ||
- `add_system_to_stage(stage, system)` becomes `add_system(system.stage(stage))` | ||
- `add_startup_system_to_stage(stage, system)` becomes `add_system(system.startup().stage(stage))` | ||
- `add_system(system.exclusive_system())` becomes `add_exclusive(system.exclusive_system())` | ||
- `add_startup_system(system.exclusive_system())` becomes `add_exclusive(system.exclusive_system().startup())` | ||
- `add_startup_system(system.label(x).before(y))` becomes `add_system(system.startup().label(x).before(y))` | ||
- `add_startup_system_set_to_stage(stage, set.label(x).before(y))` becomes `add_system_set(set.startup().stage(stage).label(x).before(y))` | ||
|
||
- `add_system(system)` remains unchanged | ||
- `add_system_set(SystemSet::on_enter(State).with_system(system))` remains unchanged (any config written to systems in a set will get overwritten by the config on the set when baked) | ||
|
||
The rest should follow. | ||
thechubbypanda marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
## Implementation strategy | ||
|
||
This has already been implemented in pr [#2736](https://github.com/bevyengine/bevy/pull/2736). | ||
|
||
Similar to what was suggested by @cart in #1978: | ||
|
||
``` | ||
trait System { | ||
/* same functions as before */ | ||
fn config(&mut self) -> &mut SystemConfig; | ||
} | ||
|
||
trait LabelSystem { | ||
fn label(self, label: impl SystemLabel) -> Self | ||
} | ||
|
||
impl<T: System> LabelSystem for T { | ||
fn label(self, label: impl SystemLabel) -> Self { | ||
self.config().add_label(label); | ||
self | ||
} | ||
} | ||
``` | ||
|
||
- Each system impl stores a `struct SystemConfig` which stores data such as stage labels, run criteria, insertion points, etc. | ||
- A trait is created for each type of Config(urability) such as a `trait StageConfig` which has a `stage(StageLabel)` function that sets the given StageLabel in the system's config struct. | ||
- All config types are added to the prelude so that no more imports are required in the examples at the very least. | ||
- All legacy system adding functions are removed and replaced with the basic ones described in the motivation. They will handle everything the same as the original ones did but in fewer functions. | ||
|
||
## Drawbacks | ||
|
||
- Requires changes over a large surface area. | ||
- Changes the way end users interact with the API, requires a (short) adjustment period. | ||
- Removes the current system descriptor system (existing prs may need reworking) | ||
- Adds a minor memory overhead to each system. | ||
- Minor boilerplate increase for each system impl. | ||
- "Forces systems to own "schedule config", which muddles the "dependency tree" a bit. Systems currently exist a level "below" shcedules, which means other schedule apis could theoretically be built on top. This "mixes" the levels a bit." - @cart #1978 | ||
|
||
## Rationale and alternatives | ||
|
||
- Alternative: Somehow write this on top of the existing system descriptors. | ||
- Would be signifcantly less "clean", where clean is a mix of line count and code "layers". | ||
- Not implementing this rfc would likely hinder future users and devs of bevy in the long run since this improvement is more modular and can be built upon easily. | ||
|
||
## Unresolved questions | ||
|
||
- Are we happy to fundamentally alter the end user experience of Bevy and basically make all existing docs out of date. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced that giving exclusive systems their own api / making them more separate is the direction we should be going in (at least, not yet). Exclusive systems are just systems with arbitrary side effects (new archetypes, new components, etc). Our plans for "stageless schedules" need to have some way to accommodate "normal system" side effects (ex: applying Commands). I personally think solving that problem well will involve blurring the lines between exclusive and normal systems, potentially to the point that distinguishing between the two isn't worthwhile. Given that we don't yet know what this will look like in practice, I'd rather stick with what we currently have (which also happens to align better with the future that I think is coming). @Ratysz what are your thoughts on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think separation is absolutely the right way to go. In fact, this PR doesn't go as far as it probably should - a lot of the weird API and implementation somersaults had to happen precisely to accomodate exclusive systems.
We should stop trying to merge parallel and exclusive systems until we actually have the ability to do so (which is, indeed, a part of stageless), because right now we definitely can't blur the lines. Or so I keep saying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to mention: we tried here. It looks like a Rust limitation, it refuses to infer types across one of the boundaries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha I figured you'd take that stance. I don't have strong opinions here because exclusive systems are relatively uncommon and there are definitely good reasons to keep them separate in the current implementation. This change just creates more churn for existing exclusive system users and creates a situation where in the future we'll probably break them again in favor of something that will probably be closer to the current api. To me the (possibly temporary) marginal wins we get from this separation don't outweigh the cost of breaking everyone, but I won't push back too hard on this if everyone else thinks differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For what its worth, I made some changes to exclusive systems in #2777 that probably enable this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't get me wrong, I also subscribe to this line of thinking; it's too early to merge them, too late to not merge them - hence the dredged-up post. It's hard to not be a little bitter after time and time again seeing a shiny new API get completely blown up and polluted with faux specialisation and pseudo-HKT noise as soon as it needs to account for exclusive systems.
Unlikely, the issue is the same that lead to
enum SystemDescriptor
: we need to have a concrete type somewhere between generic system and its consumer. Only this time the boundary is smeared across half a dozen traits with 3-4 implementors each.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup I totally get that and concede the point that the current implementation would be much cleaner if we embraced a hard split. But from my perspective its hard not to be bitter that we keep investing in a "detour" in the interest of simplicity of the current executor. We wouldn't need to care about ugly faux specialization and pseudo-HKT noise if we just decided to treat exclusive systems as "normal systems" like we did in the old executor. Which I'm relatively certain is where we are headed in the future too. I pushed back pretty hard on the exclusive system split when the new executor landed and honestly its becoming clear to me that this split really is a "detour" that optimizes for a specific executor implementation that is lacking a feature that we all know we want. If cleanness and simplicity are the goals, imo our time is best spent on re-unifying exclusive systems and normal systems / formally acknowledging that Systems can have side effects (which again, is what the old impl did and what I pushed for in the new executor).
Im being a bit overly dramatic here. Not trying to be combative / turn this into an argument. I'm very happy with the improvements the new executor brought and we may very well have been better served by embracing fully separate exclusive systems in the short term. Just trying to illustrate that I also have some strong feelings about this situation / trying to qualify why I think investing further in the system/exclusive system split isn't what I want personally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the most part, I agree; I don't think being conservative about splitting is unreasonable. I keep trying to advocate for it because I believe that the amount of headache it saves us in the short term is far, far greater than the amount of headache we will endure when mending this schism further down the line.