System Lifecycle Rework: all Systems are initialized by definition #2777

cart · 2021-09-05T21:26:41Z

Note: This is just very rough experimentation. I (currently) have no intention to move this forward as-is ... I just created it because people are discussing reworking system descriptors and scheduling right now and this change is something that relates / that I've been wanting to explore for awhile.

This does the following:

Systems are "initialized" by definition. The "system initialization" lifecycle has been completely removed. IntoSystem takes a world reference and is responsible for initializing the system.
FunctionSystems now use SystemState internally, unifying them as alluded to in [Merged by Bors] - Add new SystemState and rename old SystemState to SystemMeta #2283. The impl is much smaller / simpler now
SystemDescriptors / ExclusiveSystemDescriptors now store functions that take a World and return a System.
Adding a SystemDescriptor to a Schedule therefore requires a World reference. For Apps, this means users don't need to care because we can abstract this out. For raw Schedules / Stages, naively this means they need to accept a World reference, which significantly affects ergonomics. If we were to move forward with this, we would (1) remove all IntoSystem-related methods from Schedule and Stage in favor of accepting "boxed ready-to-go" systems. This would make them much simpler / might improve compile times (2) for direct bevy_ecs users that can't use App, add relevant StageBuilder and ScheduleBuilder types that abstract out the World requirement. alternatively we could just merge bevy_app into bevy_ecs.

The things this accomplishes:

One less thing Systems need to do
Simpler / smaller implementation that allows code reuse
Systems no longer need to store their param state in an Option or unwrap it on every run
Systems no longer need to store their Config indefinitely. It is only in memory when the app is being built.
Things running systems and run criteria (like the parallel executor) no longer need to worry about ensuring everything is initialized, which makes the code smaller/simpler/mistake proof.

This comes at the cost of:

Systems types cannot be created and stored without a World. This makes some patterns harder (ex: see the changes made to the old renderer's graph init ... this was quick and dirty. Fortunately we dont need that pattern in the new renderer). People that really need this pattern (which honestly should be discouraged generally) can just use SystemDescriptors instead of boxed Systems.
Schedules and Stages cannot add new systems without a World reference. This makes sense to me because they are the "baked and ready to go" types. In situations where a world isn't available, we could have intermediate types like StageDescriptor. But I expect App to meet basically everyone's needs in practice.

cart · 2021-09-05T21:31:11Z

This touches a lot of stuff and most of it is just noise. The most relevant changes are in function_system.rs, the System trait, system descriptors, and the executor.

alice-i-cecile · 2021-09-05T21:45:11Z

alternatively we could just merge bevy_app into bevy_ecs.

I'd prefer not to do this. The standalone bevy_ecs users that I've talked to tend to have very unusual data-flow patterns and the current Schedule abstractions tends not to work well for them.

They also tend to care about compile sizes and dependencies in ways that other Bevy users don't.

alice-i-cecile · 2021-09-05T21:48:05Z

crates/bevy_core/src/time/fixed_timestep.rs

@@ -104,6 +97,27 @@ impl FixedTimestep {
    }
 }

+impl IntoSystem<(), ShouldRun, ()> for FixedTimestep {


I love the reduced boilerplate needed here.

alice-i-cecile · 2021-09-05T21:54:39Z

crates/bevy_ecs/src/system/system.rs

@@ -90,3 +88,47 @@ pub(crate) fn check_system_change_tick(
        *last_change_tick = change_tick.wrapping_sub(MAX_DELTA);
    }
 }
+
+// TODO: This impl could result in multi-boxed systems, but it also enables things like the new FixedTimestep impl
+// maybe fine generally? who boxes their systems on insert?


Yeah, that seems like a weirdly pathological thing for users to do.

alice-i-cecile · 2021-09-05T21:56:41Z

crates/bevy_ui/src/render/mod.rs

-    let mut shaders = world.get_resource_mut::<Assets<Shader>>().unwrap();
-    let mut active_cameras = world.get_resource_mut::<ActiveCameras>().unwrap();
-    let msaa = world.get_resource::<Msaa>().unwrap();
+    world.resource_scope(|world, mut graph: Mut<RenderGraph>| {


Deeply nested resource_scope's seems a bit suspicious. Is this driven by the other changes in this PR?

This was the "quick and dirty" solution I called out in the description. The better fix would be to adapt the Render Graph to account for the System lifecycle, but thats more work than I wanted to invest in a proof of concept.

# Objective - Fix the ugliness of the `config` api. - Supercedes #2440, #2463, #2491 ## Solution - Since #2398, capturing closure systems have worked. - Use those instead where we needed config before - Remove the rest of the config api. - Related: #2777

# Objective - Fix the ugliness of the `config` api. - Supercedes bevyengine#2440, bevyengine#2463, bevyengine#2491 ## Solution - Since bevyengine#2398, capturing closure systems have worked. - Use those instead where we needed config before - Remove the rest of the config api. - Related: bevyengine#2777

cart · 2022-11-20T19:35:44Z

Worth considering but this is way out of date and would need significant reworks at this point.

System Lifecycle Rework: all Systems are initialized by definition

27b7960

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Sep 5, 2021

cart added A-Rendering Drawing game state to the screen A-ECS Entities, components, systems, and events and removed S-Needs-Triage This issue needs to be labelled A-Rendering Drawing game state to the screen labels Sep 5, 2021

alice-i-cecile reviewed Sep 5, 2021

View reviewed changes

alice-i-cecile added the S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged label Sep 22, 2021

DJMcNab mentioned this pull request Jan 10, 2022

[Merged by Bors] - Remove the config api #3633

Closed

alice-i-cecile mentioned this pull request Feb 16, 2022

One-shot systems via Commands for scripting-like logic #2192

Closed

maniwani mentioned this pull request Mar 31, 2022

Detecting system order ambiguities requires a specific world #4364

Open

alice-i-cecile mentioned this pull request Apr 18, 2022

Fast and correct one-shot systems with a system registry resource #4090

Closed

7 tasks

cart closed this Nov 20, 2022

Pascualex mentioned this pull request Mar 10, 2023

Fast and correct one-shot systems with a system registry resource #7999

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

System Lifecycle Rework: all Systems are initialized by definition #2777

System Lifecycle Rework: all Systems are initialized by definition #2777

cart commented Sep 5, 2021 •

edited

Loading

cart commented Sep 5, 2021

alice-i-cecile commented Sep 5, 2021

alice-i-cecile Sep 5, 2021

alice-i-cecile Sep 5, 2021

alice-i-cecile Sep 5, 2021

cart Sep 5, 2021

cart commented Nov 20, 2022

System Lifecycle Rework: all Systems are initialized by definition #2777

System Lifecycle Rework: all Systems are initialized by definition #2777

Conversation

cart commented Sep 5, 2021 • edited Loading

cart commented Sep 5, 2021

alice-i-cecile commented Sep 5, 2021

alice-i-cecile Sep 5, 2021

Choose a reason for hiding this comment

alice-i-cecile Sep 5, 2021

Choose a reason for hiding this comment

alice-i-cecile Sep 5, 2021

Choose a reason for hiding this comment

cart Sep 5, 2021

Choose a reason for hiding this comment

cart commented Nov 20, 2022

cart commented Sep 5, 2021 •

edited

Loading