Should EntityCommands consume self?

[Luminoth]

Bevy version

0.14.1

What you did

This is more of a question about a possible change than a real Bug bug - I'm wondering if EntityCommands should consume self rather than taking it in as a reference and returning it as a reference. The way it works today, you can't do something like this:

fn create_button<'a>(parent: &'a mut ChildBuilder) -> EntityCommands<'a> {
    parent
        .spawn(ButtonBundle::default())
        .with_children(|parent| {
            parent.spawn(TextBundle::default());
        })
}

because spawn creates a temporary that can't be returned from here. A key to this is that the method is taking in a ChildBuilder, not a Commands, implying that it's being used from within a call to with_children. Normally a method like this might return the Entity instead and then the caller would use Commands to further modify it, but that isn't possible here:

    commands
        .spawn(ButtonBundle::default())
        // this fails to build because commands is being held mutably in the outer call to spawn
        // and can't be accessed within the closure
        .with_children(|parent| {
            let ent = parent.spawn(TextBundle::default()).id();
            commands.entity(ent).insert(TextBundle::default());
        });

Not being able to do this makes composing more complex, reusable UI structures somewhat difficult I think and I believe making the change to have those EntityCommands methods consume and return self would fix that without breaking anything else?

I've also tried resolving this by returning from create_button with reborrow but it appears to still be referencing the temporary created by the call to parent.spawn() and so fails the same as the original version

[benfrankel]

Another example where this is awkward is in the implementation of Commands::spawn:

https://github.com/bevyengine/bevy/blob/35fb54fa499074112a9e0391cab7583a14698209/crates/bevy_ecs/src/system/commands/mod.rs#L366-L370

Taking self instead of &mut self, this would instead be:

pub fn spawn<T: Bundle>(&mut self, bundle: T) -> EntityCommands {
    self.spawn_empty().insert(bundle)
}

[alice-i-cecile]

I think this is a good idea :)

[crvarner]

@alice-i-cecile just ran into this breaking avian3d (easy to fix), but thought it was worth pointing out it definitely does break some things. In particular, you can no longer call EntityCommand::insertX multiple times with the same value.

let entity_commands = commands.entity(e);
------------------- move occurs because `entity_commands` has type `EntityCommands<'_>`, which does not implement the `Copy` trait

entity_commands.insert(...)
--------------- value moved here

entity_commands.insert(...)
^^^^^^^^^^^^^^^ value used here after move

So now chaining is mandatory when it used to be optional

[alice-i-cecile]

Yep, that makes sense. We saw similar things in our internal migration.

[crvarner]

Ahh I see now that is the whole reason insert_if and try_insert_if exist, because the following is no longer possible without reassigning the variable repeatedly

entity_commands.insert(...)
if blah {
    entity_commands.insert(...)
}

[eidloi]

This is a limitation of the ChildBuilder, sickle_ui faced the same issues and I resolved it by creating a dedicated builder for UI nodes. You can capture returned IDs outside of closures and do more the next line. Also, your example is trying to insert a TextBundle on the entity that you spawned with a TextBundle, but I get the point ;D