System param config

[PixelDust22]

Objective

Frequently it is desirable to modify the state of a system after the system was already created. One common example is the Local SystemParam. By default, the value of a Local was initialized using the FromWorld trait. What if we want to reuse the same system, but have that Local SystemParam be something else?

Some existing solutions can address this issue in some situations. For example, #18067 addressed this by having the user pass in the needed data using system input.

Typically Local is used for this type of thing, but its not generally feasible or possible to configure/set the underlying T data for locals.

But what if the system already has an input? What if by the time that the additional data becomes available, the system has already been created? For example, one might want to modify the system state in a ScheduleBuildPass (#11094). None of the existing solutions allow us to do this.

Solution

Add System::configurate and SystemParam::configurate which takes a &mut dyn Any as input. We call this &mut dyn Any a "configuration token". Systems and system params to decide what to do with it.

For example, in the case of Local, we added a config token LocalConfig. LocalConfig<T> changes the value of the first uninitialized Local<T>.

Another example. In my application I can use a ScheduleBuildPass to modify system states such that some systems have their ResMut<MyState> SystemParam point to an entirely different instance if they're located in a special SystemSet.

Note that because configurate may be called before a system was initialized, we have to modify the way that states were managed for each SystemParam. Previously FunctionSystem had an Option<Param::State>, SystemParam::initialize returns the initial state, and uninitialized systems do not have state.

In order to be able to configure a system param, we need to have some state to modify. This PR made it so that each SystemParam has a default_state, and in many cases it is an Option. The state is then passed into SystemParam::initialize as a mutable reference. Conceptually, instead of having the FunctionSystem to manage SystemParam state initialization centrally, each SystemParam now manages the initialization of its own states.

One might be concerned that this is going to bloat up the size of the system state due to the additional Option enum tags. However, this is completely fine. Today we only have the following types as the system state:

• ComponentId
• QueryState for Query
• SyncCell for Local and Deferred
• ()
• Vec
• Access

Out of these types, only QueryState and SyncCell don't have a Default implementation and needs to be wrapped in an Option. QueryState contains a Vec so the compiler can do null pointer optimization. ComponentId has a invalid value that we've been using. So at the end of the day, only SyncCell needs an actual enum tag.

Usage

 #[test]
 fn local_config() {
     let mut schedule = crate::schedule::Schedule::default();
     schedule.add_systems(
         test_system
             .with_config(&mut LocalConfig(Some(123_usize)))
             .with_config(&mut LocalConfig(Some(456_usize))),
     );
     let mut world = World::new();
     schedule.run(&mut world);

     fn test_system(local: Local<usize>, local2: Local<usize>) {
         assert_eq!(*local, 123);
         assert_eq!(*local2, 456);
     }
 }

Testing

All bevy_ecs tests passing.

It is recommended to review this PR commit-by-commit.

[PixelDust22]

This double-init hackery was added by @cart #2765 (comment) but it shouldn't be needed.

Instead of double-init, we should be able to init the ParamSet on top of the original SystemMeta, then merge all of them together.

[chescock]

Instead of double-init, we should be able to init the ParamSet on top of the original SystemMeta, then merge all of them together.

Note that doing it like this means the component_access_set.extend below will add a full copy of SystemMeta each time. There's no way to easily de-duplicate FilteredAccess, so this will wind up duplicating any access from earlier parameters in the FilteredAccessSet. If you have multiple ParamSetss, it will even grow exponentially!

We do make only a single call like this when using ParamSetBuilder, though, because that consumes the builder and can't be called multiple times.

bevy/crates/bevy_ecs/src/system/builder.rs

Lines 445 to 446 in 01d2b85

	// That means that any `filtered_accesses` in the `component_access_set` will get copied to every `$meta`
	// and will appear multiple times in the final `SystemMeta`.

[PixelDust22]

This is a rather unfortunate situation. Should have been addressed a long time ago. Sounds like we will have to eventually redesign the interface of SystemParam to fully fix this.

#2765 (comment)

[chescock]

This is a rather unfortunate situation. Should have been addressed a long time ago. Sounds like we will have to eventually redesign the interface of SystemParam to fully fix this.

Yeah, I think the cleanest solution would be for init to return an access. Tuples and ParamSet would merge the child accesses, but tuples would need to check for conflicts first so that they don't hand out conflicting accesses. That has the nice property that the conflict checks are only written in one place!

The problem is that passing a &mut and doing in-place updates is a lot more efficient, so I think it would be a loss overall.

We might be able to make it work if we first split out state initialization from access calculations, and then split out access calculations into a conflict check followed by an update. So there'd be init_state that does nothing with access, check_conflicts that panics if there is a conflict but doesn't update anything, append_access that adds access to a list without checking for conflicts, and check_conflicts_and_append_access that has a default impl calling the other two in order.

ParamSet would delegate check_conflicts and append_access to the child parameters, but because check_conflicts is called before append_access, they wouldn't check conflicts with each other. Tuples would have append_access create a temporary access list and call check_conflicts_and_append_access on each child, then merge the temporary list in at the end. For performance, it would then override check_conflicts_and_append_access to call check_conflicts_and_append_access on the child parameters directly. That should compile down to the same thing as today for tuples, would remove the double-init for ParamSet, and wouldn't require duplicating any code.

But that would be a pretty big refactoring, and I don't think anyone has actually had performance issues with ParamSet::init_access, so it might not be worth it. (Although I am planning a PR to split out state initialization and access calculation so that we can share the access calculations with system param builders.)

[chescock]

This seems to have some conceptual overlap with SystemParamBuilder. For example, your "usage" example can be done today with

let mut world = World::new();
let mut schedule = Schedule::default();
schedule.add_systems(
    (LocalBuilder(123_usize), LocalBuilder(456_usize))
        .build_state(&mut world)
        .build_system(test_system),
);
schedule.run(&mut world);

fn test_system(local: Local<usize>, local2: Local<usize>) {
    assert_eq!(*local, 123);
    assert_eq!(*local2, 456);
}

What can you do with this that you can't do already with builders? Is there some way we can combine the two concepts so that there is only one way to do this?

[chescock]

Instead of double-init, we should be able to init the ParamSet on top of the original SystemMeta, then merge all of them together.

Note that doing it like this means the component_access_set.extend below will add a full copy of SystemMeta each time. There's no way to easily de-duplicate FilteredAccess, so this will wind up duplicating any access from earlier parameters in the FilteredAccessSet. If you have multiple ParamSetss, it will even grow exponentially!

We do make only a single call like this when using ParamSetBuilder, though, because that consumes the builder and can't be called multiple times.

bevy/crates/bevy_ecs/src/system/builder.rs

Lines 445 to 446 in 01d2b85

	// That means that any `filtered_accesses` in the `component_access_set` will get copied to every `$meta`
	// and will appear multiple times in the final `SystemMeta`.

[PixelDust22]

@chescock This PR addresses an entirely different issue. It gives you the ability to configure a system dynamically, after the system was already built. I'm using Local as an example usage here because it's well understood by the community. Instead of dictating the state of a system upfront, this API allows you to modify it by passing "config tokens" afterwards.

For example, in a ScheduleBuildPass #11094 , you're given some Box<dyn System>. These systems were already created by someone else and you don't know who created them or what SystemParam they have. You just know that, if they have a Local<MyValue>, you want that MyValue to be MyValue(123).

The system builder will not help in this case because

1. The system was already built, and
2. You don't know what SystemParam it has

One additional benefit of this PR is that, because this PR separates the "default" system state and "initialize" system state, after this PR is merged, we can get rid of the awkward build_state(&mut World) part of the system builder API. And so the system will be initialized by the scheduler like everyone else. This was impossible previously because you couldn't have a SystemState without initializing it with the world.

So your example is going to look like

let mut schedule = Schedule::default();
schedule.add_systems(
    (LocalBuilder(123_usize), LocalBuilder(456_usize))
        .build_state() // << No world needed yet!
        .build_system(test_system),
);


let mut world = World::new(); // << World can be created later!
schedule.run(&mut world);

fn test_system(local: Local<usize>, local2: Local<usize>) {
    assert_eq!(*local, 123);
    assert_eq!(*local2, 456);
}

[chescock]

For example, in a ScheduleBuildPass #11094 , you're given some Box<dyn System>. These systems were already created by someone else and you don't know who created them or what SystemParam they have. You just know that, if they have a Local<MyValue>, you want that MyValue to be MyValue(123).

Can you describe the ScheduleBuildPass that you're actually trying to build? I see that this lets you update systems after they are created, but I don't understand why you would need to do that. Why not create the systems later so that the configuration is already available? Or configure them using a resource so that it can be reconfigured as needed?

Updating the local variables of a system that you didn't create sounds like it would break encapsulation! What if the system was relying on the values for soundness? (For contrast, builders can only configure systems if they are exposed as pub fns, and in that case they already have to handle being called as ordinary functions.)

[PixelDust22]

@chescock In my particular case, I have a ScheduleBuildPass that identifies all systems with a SystemParam SubmissionInfo that I defined, look at the system sets and some other information, group certain systems together, and make it so that systems in the same group share the same SubmissionInfo. It does this by configuring the SubmissionInfo SystemParam so that systems in the same group shares the same ComponentId. This allows systems that don't share the same SubmissionInfo to be executed in parallel.

This is very important for the Vulkan-based render backend that I'm working on. Each frame can be broken down into multiple submissions and each submission shares the same set of resources. In order to achieve optimal parallelism for command buffer recording, the scheduler needs to be able to look at the entire render graph and insert submission at the optimal places. This is what the ScheduleBuildPass does: it looks at your entire system graph, group systems together optimally based on some heuristics, insert a submission system for each group, then configure systems in the group to share resources with the submission system.

Why not create the systems later so that the configuration is already available?

Because the configuration came from the system schedule itself. Which doesn't exist until all the systems were added.

Or configure them using a resource so that it can be reconfigured as needed?

Because systems in each group needs to be executed in parallel. You can obviously create a resources that contains an array of SubmissionInfo, alongside a map from system to array index, but then each system will have to contend on this resource and nothing can be executed in parallel. You'll also have trouble identifying the systems. By allowing an external actor (in this case the ScheduleBuildPass) to modify system states, we can precisely specify their component access and maximize parallelism.

Updating the local variables of a system that you didn't create sounds like it would break encapsulation!

It won't break encapsulation, because these configurations must be done through config tokens that the systems define.

For example, in the case of Local, you configure the system state of Local using LocalConfig which is a config token defined by the same module as Local. If an unknown config token was passed to configuarate(&mut self, &mut dyn Any), systems and SystemParams will just do nothing. This is the same idea as builders - you can only configure systems if they're exposed as a config token.

And if you're saying that, if I have a system that behaves well if Local<u64> == 12 but does out-of-bound reads if Local<u64> == 12000, well then it's up to that system to verify. Local<u64> is an argument passed to the system, which is a function. In general a function shouldn't make any assumptions about its arguments. And when it does, it's up to the function to assert those assumptions.

In this particular case, the system can prevent such modifications by making it Local<MyPrivateStruct> where MyPrivateStruct is private. That way the system can be super sure that nobody can construct a LocalConfig<MyPrivateStruct> to modify the system state of that Local<MyPrivateStruct>.

[janhohenheim]

Triage: failing tests

[chescock]

In my particular case, I have a ScheduleBuildPass that identifies all systems with a SystemParam SubmissionInfo that I defined, look at the system sets and some other information, group certain systems together, and make it so that systems in the same group share the same SubmissionInfo. It does this by configuring the SubmissionInfo SystemParam so that systems in the same group shares the same ComponentId. This allows systems that don't share the same SubmissionInfo to be executed in parallel.

Thanks, that's helpful context!

And if you're saying that, if I have a system that behaves well if Local<u64> == 12 but does out-of-bound reads if Local<u64> == 12000, well then it's up to that system to verify. Local<u64> is an argument passed to the system, which is a function. In general a function shouldn't make any assumptions about its arguments. And when it does, it's up to the function to assert those assumptions.

Right, a pub fn needs to be able to accept any arguments. My worry here is plugins adding systems created using non-pub functions or closures. Today, Locals in those systems are implementation details, but the existence of LocalConfig will make them always part of the public API, which would be surprising.

[PixelDust22]

Right, a pub fn needs to be able to accept any arguments. My worry here is plugins adding systems created using non-pub functions or closures. Today, Locals in those systems are implementation details, but the existence of LocalConfig will make them always part of the public API, which would be surprising.

Any function that interacts with the outside world needs to be able to accept any arguments. A pub fn interacts with the outside world because someone else could call it and pass in arbitrary function. A closure or private function interacts with the outside world once you add it using the add_systems API, even if it's private.

Adding a closure as a system is basically equivalent to registering a callback. Even though the callback function itself is private, the moment you register it, it is already interacting with the outside world, and the input arguments are no longer in your control - because you're not the one calling it.

The pub keyword defines "visibility": you expose your function to the outside world. consent to other people calling your function, therefore making it a part of the public API. When you call add_systems, you also expose and explicitly consent to other people (bevy developers) calling your function, even though the function itself wasn't explicitly defined as pub. As such, we (bevy developers) should be able to call those functions in a well-defined way. In this case, we specify that someone can modify the way we call those functions using the LocalConfig token.

[chescock]

When you call add_systems, you also expose and explicitly consent to other people (bevy developers) calling your function, even though the function itself wasn't explicitly defined as pub. As such, we (bevy developers) should be able to call those functions in a well-defined way. In this case, we specify that someone can modify the way we call those functions using the LocalConfig token.

Right, my point is that this is changing the contract of how Bevy will call the function. Today, once a system is built, Bevy promises that a Local will never be modified outside of the system, and users may rely on that. After this change, it may be modified by a LocalConfig, so users would need to ensure that they can handle any value. That may be a worthwhile change! But it is a change, and it invalidates some patterns that are valid today.

[ItsDoot]

Today, once a system is built, Bevy promises that a Local will never be modified outside of the system, and users may rely on that.

To reduce controversial-ness, we could introduce a separate Local-like system parameter that accepts outside mutation. Name it VarLocal<T> or something similar.

[cart]

Rather than adding the concept of "configuration tokens", could we instead just let users operate directly on the system state? One of my reactivity experiments involved using this pattern:

let mut system = IntoSystem::into_system(|local: Local<usize>| {});
system.initialize(world);
let state = system.get_state_mut().unwrap();
*state.0.get() = 10; // get() is required because Local's state is a SyncCell<usize>

The only missing API is system.get_state_mut() (we currently only have the get_state() variant).

From there, the big question is "how do expose writing this state to the user". It seems like we could have my_system.set_state(|state| { state.0.get() = 10; }), which returns a wrapper system that calls that function during wrapper_system.initialize(). Likewise, if we move to systems as entities, devs could just query for the system state and modify it.

I think I prefer that over adding new concepts.

[PixelDust22]

@cart The problem is that set_state needs to be object safe so that we can call this on a Box<dyn System>. The get_state method you mentioned was defined on WorldQuery which doesn't need to be object safe afaik.

Obviously we can define get_state_mut as a function on trait System that returns a &mut dyn Any.

fn get_state_mut(&mut self) -> &mut dyn Any

But how does the caller know what to cast it into? And even if the caller does know, when the user adds a new param to this system, the downcast could fail.

There's also the problem of encapsulation. Systems may not want to expose all of its internals to the outside world. When the user wants to modify system states, systems and system params may want to ensure that it's done in a well-defined way.

My solution is to reverse this and have the caller pass a &mut dyn Any to the callee. we call this &mut dyn Any a "configuration token".

fn configurate(&mut self, token: &mut dyn Any);

The system distributes the token to each of its params. Params who know what it is will successfully downcast and react to it. Params who don't know what it is will do nothing. If multiple params know what it is, (for example if you have multiple Local<usize>), what happens then will be defined by the token itself. For example in this PR, LocalConfig<usize> is the configuration token and it specifies that it'll only modify the state of the first Local<usize>. If you have multiple Local<usize>, only the first Local<usize> will see its value changed.

I realize that using a big word (like "configuration token") may be a little scary, but really it's nothing other than a &mut dyn Any that represents the change you wanna make to a certain type of System or SystemParam.

[PixelDust22]

@ItsDoot This PR is really about the System::configurate and SystemParam::configurate APIs, and the necessity to have a "default state" that is present before a system was initialized.

I'm happy to remove the LocalConfig from this PR if it makes the PR easier to merge. This is just an example to illustrate how SystemParam::configure can be useful. We can discuss whether and how to modify the Local systemparam contract and the necessity of LocalConfig later on.