One Shot Systems

[Trashtalk217]

I'm adopting this child PR.

Objective

• Working with exclusive world access is not always easy: in many cases, a standard system or three is more ergonomic to write, and more modularly maintainable.
• For small, one-off tasks (commonly handled with scripting), running an event-reader system incurs a small but flat overhead cost and muddies the schedule.
• Certain forms of logic (e.g. turn-based games) want very fine-grained linear and/or branching control over logic.
• SystemState is not automatically cached, and so performance can suffer and change detection breaks.
• Fixes One-shot systems via Commands for scripting-like logic #2192.
• Partial workaround for API for dynamic management of systems (adding and removing at runtime) #279.

Solution

• Adds a SystemRegistry resource to the World, which stores initialized systems keyed by their SystemSet.
• Allows users to call world.run_system(my_system) and commands.run_system(my_system), without re-initializing or losing state (essential for change detection).
• Add a Callback type to enable convenient use of dynamic one shot systems and reduce the mental overhead of working with Box.
• Allow users to run systems based on their SystemSet, enabling more complex user-made abstractions.

Future work

• Parameterized one-shot systems would improve reusability and bring them closer to events and commands. The API could be something like run_system_with_input(my_system, my_input) and use the In SystemParam.
• We should evaluate the unification of commands and one-shot systems since they are two different ways to run logic on demand over a World.

Prior attempts

• run a system from a command #2234
• Run system from &mut World #2417
• Fast and correct one-shot systems with a system registry resource #4090
• Fast and correct one-shot systems with a system registry resource #7999

This PR continues the work done in #7999.

[alice-i-cecile]

Attempt 3 let's go :D Feel free to ping me with questions or complaints.

[Trashtalk217]

I've fixed most of the mistakes made during rebasing. I do have a question about apply_buffers? I commented it out, because I couldn't find it anywhere in the codebase. Was it removed / renamed?

Other things I still need to do:

• Remove SystemLabel from the docs (as per previous PR)
• Fix the command_processing test (may have something todo with apply_buffers)
• Go through clippy errors, warnings and formatting

[alice-i-cecile]

I do have a question about apply_buffers? I commented it out, because I couldn't find it anywhere in the codebase. Was it removed / renamed?

Renamed to apply_deferred for clarity.

[Trashtalk217]

Is SystemTypeIdLabel still used anywhere? It seems like only SystemId is used. Also, I've read the comments on the previous PR and I'm wondering about this:

I still don't like being forced to preregister systems: can't we make sure neither case panics?

I think we should stick to SystemTypeSet as our key type, but add a friendlier type alias for it.

Is this still your opinion @alice-i-cecile? What cases could panic? I take the last part to mean that the system registry should look like this:

#[derive(Resource, Default)]
pub struct SystemRegistry {
    systems: Vec<(bool, SystemTypeSet<dyn Any>)>,
    indices: TypeIdMap<usize>,
}

[alice-i-cecile]

Is SystemTypeIdLabel still used anywhere?

Yeah, I think this got cut in the stageless refactor.

Is this still your opinion @alice-i-cecile? What cases could panic?

The concern is "if a user calls commands.run_system(my_system) without preregistering it, should it panic?". My answer is firmly no. This should register the system, and then call it.

I take the last part to mean that the system registry should look like this

I really dislike that design 😂 If at all possible I'd like to just do a HashMap<TypeId, BoxedSystem>, rather than this indirection. There's no reason to store the topological sort like in Schedule.

[Trashtalk217]

I've added the following test for running unregistered systems with commands:

    #[test]
    fn unregistered_system_command() {
        fn command_system(mut commands: Commands) {
            commands.run_system(spawn_entity);
        }

        let mut world = World::new();
        assert_eq!(world.entities.len(), 0);
        world.run_system(command_system);
        assert_eq!(world.entities.len(), 1)
    }

And as expected this panics, but it doesn't panic where I expected it to.

thread 'system::system_registry::tests::unregistered_system_command' panicked at 'resource does not exist: bevy_ecs::system::system_registry::SystemRegistry', crates/bevy_ecs/src/world/mod.rs:1317:32

It can't seem to find the system registry in the resources. I double-checked the error location with a debugger and found the following:

(I added temp if that wasn't obvious)

So while temp is not None, the panic still triggers.

I am really confused.

[alice-i-cecile]

Oh: that's an artifact of how you're testing it.

In order to call World::run_system we need to temporarily extract the SystemRegistry from the world using a resource_scope. Then, any further attempt to use functionality that relies on the system registry (like applying those commands) fails, because the SystemRegistry has been removed.

I ran into this previously, describing it as a difficulty in using one shot systems "recursively".

As for a fix:

1. We could accept it as a known limitation (at least for now) and provide a better error message
2. We could store SystemRegistry as a special field on the World, and then split the borrow using UnsafeWorldCell instead and simply prevent you from calling the currently active system.
3. We could insert the SystemRegistry resource if it doesn't exist, and accept subtly broken behavior (e.g. change detection won't work).
4. We could insert the SystemRegistry resource if it doesn't exist, and then merge the temporary one with the original one somehow as the resource scope ends 🤔

4 is my preferred solution if we can get it to work. 1 is a good "fix for now" if that's not easy. 2 would be okay but not great. 3 is simply bad.

[Trashtalk217]

My initial intent was to fix unregistered systems panicking when being called through commands.

The concern is "if a user calls commands.run_system(my_system) without preregistering it, should it panic?". My answer is firmly no. This should register the system, and then call it.

Instead, I stumbled into a separate issue, which I will add to the list:

• Auto register systems when called through commands (still need to reproduce)
• Fix nested one-shot systems
• Simplify SystemRegister to use a TypeId Hashmap

While I think there's a way to implement nested one-shot systems cleanly, for now, I'll insert a better error message, as this PR is large enough already.

[Trashtalk217]

I've completed one of the tasks, I have two questions:

• Is it possible for two systems to have the same TypeId, causing a name collision?
• Is it still necessary to have the run_by_id method / Command, it feels largely unnecessary right now. I can only imagine a couple of uses and they're mostly marginal.

[Trashtalk217]

I've attempted to reproduce the 'system not being registered through commands', but I have not been able to do so. The current test run_system_through_command runs perfectly well. Other than that I'm technically done.

I still am really unsure what to do with the run_by_id methods, as I don't see the point in them. They are also largely untested. Apart from the missing documentation, I think I could use a code review.

[Trashtalk217]

After some deliberation on Discord, it was decided that the use case for calling systems by their Id is very marginal and it was removed. It can be added later if a good use case is found.

[github-actions]

Example alien_cake_addict failed to run, please try running it locally and check the result.

[Trashtalk217]

I've thought about this problem some more and due to the comments, I decided that for most substantial use cases it would be necessary to have a finer control over the system registry.

So SystemIds are back and I'm slowly discovering what the purpose of this fence is.

In my current implementation registering / run_by_id / removing would work as you expect. run_system, however, becomes interesting.

When run_system is called, the given system is registered, ran, and immediately removed. My reasons for this are as follows:

• It's simple, if you just want to run a system, you shouldn't have to deal with SystemIds, so it doesn't return any
• If we're not returning an id, we might as well remove the system as well
  Because we're treating systems as black boxes (disregarding TypeId), this is the only way I can see it done.

This, however, introduced two foot guns when it comes to using run_system.

1. Local variables don't work. This is obvious since none of the internal state is maintained between runs as that environment is recreated from scratch.
2. Change detection doesn't work, because of similar reasons.

These issues can be circumvented by using register and run_by_id manually. See also the changed tests in the commit.

I'm not a huge fan of the current SystemId ergonomics as they basically are glorified pointers, and while pointers are not bad, they are very powerful and easy to abuse.

[wizzeh]

I'm aware this isn't the most helpful type of review but IF you're frustrated with the current ergonomics my suggestion would be to rename run_by_id to run_system and rename what's currently run_system to run_callback . Might make the possible footgun of using Local when running a closure that isn't manually registered more obvious.

[Trashtalk217]

I'm aware this isn't the most helpful type of review but IF you're frustrated with the current ergonomics my suggestion would be to rename run_by_id to run_system and rename what's currently run_system to run_callback . Might make the possible footgun of using Local when running a closure that isn't manually registered more obvious.

I find run_system_by_id and run_system fine names as-is, but I'd be equally fine with these names.

[Trashtalk217]

I have a question regarding the CI. It says that there's an ambiguity when importing RunSystem, where it's imported both from system.rs and system_registry.rs, but I went through the system.rs and I couldn't find any mention of RunSystem. I'm also not able to recreate this problem locally.

[flisky]

@Trashtalk217 It's in the main branch.

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

Line 241 in b88ff15

pub trait RunSystem: Sized {

The CI will automatically rebase the PR based on main, just FYI.

[alice-i-cecile]

I'm very happy with the state of this, and think it's important to move this forward to unblock UI uses of this.

That said, we need to think about the API in light of #9366.

The caching introduced in this PR is very useful for more complex uses of one-shot systems, where you care about performance, change detection and system-local state. So both are helpful!

That said, we need to clearly disambiguate the two APIs in the docs and method names.

Steps:

1. Rebase this PR.
2. Rename the methods for run_system from this PR to run_cached_system or something to that effect.
3. Add notes to the docs of both methods to clarify the differences.

Further unification would be really nice, but I'm not immediately sure of the best way to do that.

[re0312]

it seems there is ambiguity with the name RunSystem. In #9366 , RunSystem is used as a trait implementation for &mut world, but in this pr, it is being used as a command."

[iiYese]

Additionally I think some sort of compatibility with #9366 would make this PR far more useful. Take for example a widget callback. Users will be interested in getting the components of the entity and to do that the callback system needs some way of knowing the entity it exists on. This could be done with #9366.

cmds.spawn((
    Rect,
    Button,
    Counter(0),
    OnClick(reg.register(|In(entity): In<Entity>, mut counter: Query<&mut Counter>| {
        counter.get_mut(entity).unwrap() += 1;
    }))
))

fn handle_mouse_clicks(clickable: Query<(&Rect, &OnClick, Entity)>, registry: SystemRegistry, ..) {
    if left_button_pressed {
        if let Some((_, OnClick(sys), ent)) = clickable
            .iter()
            .find(|(rect, ..)| rect.contains(mouse_pos)) {
                registry.run_by_id_with(ent, sys);
            }
            
    }
}

[Trashtalk217]

I'm back. A lot of discussion has been happening. Let me just summarize a couple of my opinions.

It is indeed quite silly to have a SystemRegistry only for it to be removed in the next PR, and since it wasn't very hard to do (see my last commit), I decided that this was better. It saves code, it saves structs, and nested system calls are now possible.

However, I am not quite ready for a full 'systems as entities', because that has a lot of knock-on effects and creates weird behavior (as @mxgrey pointed out). Granted it's not as bad as UB, but I'd still rather not have them. The trade-off between predictable behavior and unconstrained expressiveness is very real, but I want to stress that it only matters when users encounter problems with either side of this equation. The edge case that @mxgrey pointed out seems very unlikely, but also, as @iiYese pointed out, it's questionable whether or not some of the proposed features will ever see any use.

I'd much rather act upon an issue by someone developing a game/library than act on speculation. Foresight is good, especially for library development, but there must be some limit.

I've settled on a closed-off implementation, where the RegisteredSystem component stays private and entity access is locked behind SystemId so people don't go modifying stored systems willy-nilly.

Briefly about the API. I'm thinking about removing the following endpoints.

• RunSystemCommand because RunSystem is kinda footgunny so access should be restricted to just World, see Add RunSystem #9366.
• run_system(&mut App, IntoSystem), run_system_by_id(&mut App, SystemId), register_system(&mut App, IntoSystem), remove_system(&mut App, SystemId) because it clutters the App namespace unnecessarily and all of them can be called by simply doing app.world.some_method(...).

My current working theory is that if I simply keep decreasing the size of this PR it may merge one day.

Jokes aside, I'd like feedback on the API changes and the 'systems as almost entities' approach. It would be greatly appreciated.

[alice-i-cecile]

I like this API quite a bit better: it's still nicely encapsulated but meaningfully simpler.

[maniwani]

This works for me. Just need to update the documentation.

[JoJoJet]

Looks good!

As a final note, run_system_once probably doesn't need to be hidden behind a trait anymore (it can just be an inherent impl on world). We can save that for a follow-up though since it may be a more controversial change.

[Trashtalk217]

@james7132, @maniwani or @cart, can this PR be merged?

[james7132]

I can see the value in having this, especially the reactive use cases like UIs. The implementation seems good to me as well. I was worried that there would be safety concerns, but the current exclusive nature of these systems seems to avoid that. In turn, this does introduce a few performance concerns that users should be aware of.