Add ApplyCommands (#6184)

[Zeenobit]

Objective

Fixed #6184

Solution

This PR adds an ApplyCommands trait which is implemented for &mut World and &mut App. It allows the user to do this:

let mut world = World::default();
let _ = world.apply_commands(|_world, mut commands| {
    /* ... */
});

Initially this PR was posted as #6186. I accidentally deleted the repo, so this is the replacement.
I also took this chance to rename Execute to ApplyCommands because I think it matches better with existing terminology, and simplified how_to_test_commands.rs.

[alice-i-cecile]

Great docs: this is much better motivated now.

I think this should live in bevy_ecs (still using a trait extension and not in the prelude), and be linked to from the Commands docs.

[alice-i-cecile]

As a heads up, I just added Schedule::apply_system_buffers in #7267 (because pipelined rendering needs it). We should wait and see how that shakes out, and then decide whether this is still needed post-Stageless.

[alice-i-cecile]

This is no longer blocked!

[Zeenobit]

Not sure what's going on with the validation here. This is already moved to bevy_ecs.

[alice-i-cecile]

Yeah pushing a new commit will probably reset CI: we changed it quite a bit (to remove bors) since your last push.

[hymm]

should add something here to say what you should do for normal situations. i.e. call methods that act directly on World

[Zeenobit]

I feel like that's a bit overly verbose. It should be axiomatically known that if you have a &mut World, you can modify that world without commands. I'd also argue that methods which act directly on World aren't "normal situations". In normal situations Commands and systems are used to modify the world, which is why we added the link to Commands.

[Zeenobit]

@alice-i-cecile Is there anything blocking the integration of this from my end? As far as I can tell all the review concerns have been addressed.

[alice-i-cecile]

This has my approval now: it's genuinely useful for tests, and I think the cleanest way to do this.

[cart]

Why is this preferable to the existing:

InsertOrAddRequest(entity, Request).write(&mut world);
InsertOrAddRequest(entity, Request).write(&mut world);

It doesn't add a new abstraction, uses fewer lines of code, and is more efficient (because it doesn't use an intermediate Commands buffer).

[Zeenobit]

It's often desirable to write utility functions that take a &mut Commands in game logic. For example:

fn spawn(commands: &mut Commands) -> Entity {
  let bundle = todo!("Make a bundle");
  commands.spawn(bundle).id()
}

Without this changelist, I can't test such a function without manually creating a CommandQueue, or a duplicate version of the function which works on &mut World.

[cart]

Fair point! I agree that Commands access might be necessary in some cases. But this should only really be used for tests as allocating and immediately dropping CommandBuffer isn't a pattern we should be encouraging.

Is the risk of making this easy really worth it? Especially when this is almost as ergonomic?

let mut schedule = Schedule::new();
schedule
    .add_systems(move |mut commands: Commands| {
        commands.add(InsertOrAddRequest(entity, Request));
        commands.add(InsertOrAddRequest(entity, Request));
    })
    .run(&mut world);

And if we reeeeeealllly want to make this ergonomic, maybe we should just do this:

world.run_system(move |mut commands: Commands| {
    commands.add(InsertOrAddRequest(entity, Request));
    commands.add(InsertOrAddRequest(entity, Request));
});

Also an anti-pattern in game code (because it will create the system state and immediately drop it). But it removes the special casing of commands and makes this more generally useful.

[Zeenobit]

I would agree that run_system is a superior alternative to apply_commands.

Let me know if you'd like me to modify this CL into run_system.

[Zeenobit]

On second thought, there is a downside to run_system compared to apply_commands.

apply_commands returns the return value of its function. This allows the function to return the list of entities spawned, for example. As far as I know, we wouldn't be able to get the output of a system with run_system. Right...?

[alice-i-cecile]

We could hook up run_system to system piping TBH.

[Zeenobit]

If run_system could return the output of a piped system, that would be most ideal. I'm not sure how I'd approach implementing that though. I could give it a try with some guidance/examples though.

[cart]

apply_commands returns the return value of its function. This allows the function to return the list of entities spawned, for example. As far as I know, we wouldn't be able to get the output of a system with run_system. Right...?

System impls can have outputs (return values) and run_system can return them. No need for any new abstractions. It would "just work".

[cart]

I do think we should port this to run_system.

[Zeenobit]

Sounds good to me. I'll close this PR and open a new one for run_system once I have a working prototype.

[Zeenobit]

Closing in favor of run_system based on feedback.