-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Replace multiple calls to add_system
with add_systems
#8001
Conversation
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.
Generally in favor of these changes. Most things are easier to read and understand. Had some comments around a few places where chain
could have been used.
When we start applying different restrictions to multiple systems like with
.add_system((
system1.in_set(Prepare),
system2.in_set(Prepare),
system3.in_set(Queue),
));
I think there are stylistic choices where it isn't clear what the best way to write things is. The above could also be written as:
.add_systems((system1, system2).in_set(Prepare))
.add_system(system3.in_set(Queue))
Probably not worth debating at this point, since we have some open issues around limitations of add_systems that could make this point moot.
crates/bevy_app/src/lib.rs
Outdated
)); | ||
|
||
schedule.configure_sets(( | ||
PreStartup.before(PreStartupFlush), |
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.
can this use chain
?
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.
Not really. These sets aren't being ordered with respect to each other, they're ordered with respect to *Flush
sets.
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.
schedule.configure_sets((
PreStartup,
PreStartupFlush,
Startup,
StartupFlush,
PostStartup,
PostStartupFlush
).chain());
Isn't this the same?
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.
Ooh I like that
.add_startup_system(setup_camera_fog) | ||
.add_startup_system(setup_terrain_scene) | ||
.add_startup_system(setup_instructions) | ||
.add_startup_systems((setup_camera_fog, setup_terrain_scene, setup_instructions)) |
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.
.add_startup_systems((setup_camera_fog, setup_terrain_scene, setup_instructions)) | |
.add_systems(( | |
setup_camera_fog, | |
setup_terrain_scene, | |
setup_instructions | |
).on_startup()) |
We've removed most of the other add_startup_system calls.
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.
My main issue with this syntax is that rustfmt
makes it look horrible. It'll become something like
add_systems(
(
setup_camera_fog,
setup_terrain_scene,
setup_instructions,
)
.on_startup(),
)
It's not exactly a dealbreaker, but for the time being I prefer it the way it is now.
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.
Once we get recursive SystemConfigs
working, we should be able to make this look really nice:
add_systems((
(setup_camera_fog, setup_terrain_scene, setup_instructions).on_startup(),
toggle_system,
))
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.
This in theory requires a lot more codegen than the original add_system
chain. Wonder if this has any measurable impact on cargo build --timings
.
Objective
When multiple systems are added to an app or schedule, prefer using
add_systems
. This makes the code less repetitive, and usually results in nicer formatting.Example