Have EntityCommands methods consume self for easier chaining

[Luminoth]

Objective

Fixes #14883

Solution

Pretty simple update to EntityCommands methods to consume self and return it rather than taking &mut self. The things probably worth noting:

• I added #[allow(clippy::should_implement_trait)] to the add method because it causes a linting conflict with std::ops::Add.
• despawn and log_components now return Self. I'm not sure if that's exactly the desired behavior so I'm happy to adjust if that seems wrong.

Testing

Tested with cargo run -p ci. I think that should be sufficient to call things good.

Migration Guide

The most likely migration needed is changing code from this:

        let mut entity = commands.get_or_spawn(entity);

        if depth_prepass {
            entity.insert(DepthPrepass);
        }
        if normal_prepass {
            entity.insert(NormalPrepass);
        }
        if motion_vector_prepass {
            entity.insert(MotionVectorPrepass);
        }
        if deferred_prepass {
            entity.insert(DeferredPrepass);
        }

to this:

        let mut entity = commands.get_or_spawn(entity);

        if depth_prepass {
            entity = entity.insert(DepthPrepass);
        }
        if normal_prepass {
            entity = entity.insert(NormalPrepass);
        }
        if motion_vector_prepass {
            entity = entity.insert(MotionVectorPrepass);
        }
        if deferred_prepass {
            entity.insert(DeferredPrepass);
        }

as can be seen in several of the example code updates here. There will probably also be instances where mutable EntityCommands vars no longer need to be mutable.

[github-actions[bot]]

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

[Luminoth]

Some follow up on the despawn and log_components changes - they can't continue to take &mut self because add will consume it, so it's really a question of whether or not those should be terminal. I think you could probably argue that despawn might want to be but I don't know for certain there.

[janhohenheim]

Added X-Contentious because this is a rather big API change imo and I'd like some more people to look at it before we merge it. It's very possible I did not consider something vital when giving my approval.

[Luminoth]

While there's a sec waiting for this to get some more eyes on it, I realized I messed up the commit with a merge in there that didn't need to happen. Is there a good way for me to go about cleaning that up so I can squash the two actual commits?

[janhohenheim]

@Luminoth the whole PR gets squashed anyways on merge, don't bother :)

[hymm]

This makes EntityCommands inconsistent with the Commands api with still takes &mut self. We shouldn't be changing one without changing the other. I haven't investigated if it makes sense to change Commands to consume self or not.

[Luminoth]

@hymm from a cursory look at Commands they seem to already be inconsistent. Nothing in Commands can be chained right now (eg. nothing returns either Self or &mut Self). Is there a reason to try and force them into consistency here?

[JMS55]

it would nice to have an insert_if method:

fn insert_if<C: Component>(self, condition: bool, component: C) -> EntityCommands {
    if condition { self.insert(component) } else { self }
}

[janhohenheim]

@JMS55 I like that idea! @Luminoth if you do this, make sure to also add a try_insert_if :)

[ThomasAlban]

What if, instead of consuming self, the methods took in a &mut self and returned &mut self, so that they could be chained and also used in the normal way? This is how it's done with App in bevy plugins and the pattern works really well I think

[Luminoth]

@ThomasAlban that's how it works currently. If you look at the Issue this PR references, the problem I've run into with that approach is when the EntityCommands are created inside a helper method (for reusable creation of UI components in this instance) - I can't return the result due to referencing a temporary. Consuming self resolves those sorts of issues while maintaining the ability to chain methods.

That said, it definitely seems to be an opinionated choice between the two styles of Builder pattern.

Another way to consider the options here might be that the difference between App and EntityCommands comes down to the general need to nest the result of building with EntityCommands?

[Luminoth]

@JMS55 @janhohenheim I've added the insert_if and try_insert_if. I made the condition passed in a FnOnce in case that's useful. It's a little awkward with the existing insert_if_new methods but hopefully not overly so?

I did just sort of wing the documentation, so if there's any changes there that seem needed, definitely let me know and I can get that taken care of.

[janhohenheim]

@Luminoth the documentation should imo link to insert or try_insert and say "it's like this but with a predicate. This is useful for chaining your method calls" and then show an example of a realistic chained call with a predicate. No need to repeat the information found in insert; focus on why this variant of the function is useful. Also, hmm, there probably should also be an insert_if_new_if (and try_insert_if_new_if, but try_insert_if_new was not added yet because it was forgotten in a PR IIRC), but that naming is obviously horrendous, so I'm completely fine with leaving it out of scope for the moment. Although one thing we could do is replace the if suffix with predicate. Bit ugly though. Maybe if_new should just be called new? Eh, as I said, it's fine leaving that out for the moment.

[Luminoth]

@janhohenheim I updated those docs and also updated some of the affected code to use the new insert_if where I could. The one downside this has is whatever thing you're inserting is going to get created even if it's not inserted. I don't think that's too bad in the general case?

[janhohenheim]

I think that's alright. Components are meant to be lightweight. If this is an issue in the future, a follow-up can add a variant that accepts a closure as a component constructor.

[hymm]

Nothing in Commands can be chained right now (eg. nothing returns either Self or &mut Self)

That's a fair point. I was thinking there were some things that did do chaining.

[alice-i-cecile]

Thanks for the investigation and back-and-forth. I think this is ultimately a bit nicer, and the migration doesn't seem bad at all. Merging.

[eidloi]

I think the pros outweigh the cons here. Chaining is a way more common operation than doing if foo { cmd.stuff() }, so we should optimize for that case imo. @Luminoth I think these two returning Self is perfectly fine, as you might want to call .id() in the end.

There is very little you can do by chaining commands or entity commands directly. You are more often issuing single commands then switch scope to another entity to issue another command (with potential processing steps in-between). Wrappers and existing extensions on the other hand relied on its reference nature, so now I have to rewrite an entire ~20kloc library for this slight change in ergonomics. And I am not even writing a game yet....

[tim-blackbird]

I'm iffy on this change too. It certainly doesn't seem idiomatic for a struct that isn't a builder.

This PR also introduces inconsistency between the API on EntityCommands and EntityWorldMut that should be addressed before 0.15

Maybe we should cherry pick this change onto 0.14 and check what it's like on some larger codebases?

[eidloi]

I'm iffy on this change too. It certainly doesn't seem idiomatic for a struct that isn't a builder.

This PR also introduces inconsistency between the API on EntityCommands and EntityWorldMut that should be addressed before 0.15

Maybe we should cherry pick this change onto 0.14 and check what it's like on some larger codebases?

I updated sickle_ui to main, it took about two hours which was a lot less than the removal of a lifetime param (it was slow, but there were no layered errors like with the lifetime). Note that sickle_ui is already using a builder pattern around &muts so most of the touch points were in the private API.

Still, aside from saving some characters here and there, having to recapture entity commands all the time is quite bothersome. I also had to .reborrow() it more than before. Feels like something oily that constantly tries to slip through my fingers.

But this is just my short term experience. I think being able to .reborrow it was enough to act as an ownership gate to work around lifetime issues. Chaining was never an issue. Working with the owned version was already possible, but this changes the ergonomics completely for no practical reason I can see.

[tim-blackbird]

I also had to .reborrow() it more than before.

I don't like the sound of that. Ideally lifetime tricks like .reborrow() are needed as little as possible.

[eidloi]

I also had to .reborrow() it more than before.

I don't like the sound of that. Ideally lifetime tricks like .reborrow() are needed as little as possible.

Yeah but if you have a wrapper builder which owns an EntityCommands it needs to borrow it out since calls on it would consume it.

[janhohenheim]

The practical use can be seen in the diff of this PR. Multiple pieces of code are now chained. No comment about whether that is good or bad for your specific use-case in sickle_ui, just pointing out the practical reason you have missed. This kind of usage has a precedent in App, so I don't think we need to restrict it to traditional builders. In fact, I'd argue that App and Commands can both be seen as a kind of builder for a queue, but that's not really important. I'd like to keep the API in place for the RC, see how the ecosystem at large reacts to it (especially people porting apps, not only libraries) and then decide whether to cut it or not from the actual release. I myself have not played around with the change too much, so I'm fairly neutral, but I do like the diff in the PR and would like to give it at least a chance.

[eidloi]

The practical use can be seen in the diff of this PR. Multiple pieces of code are now chained. No comment about whether that is good or bad for your specific use-case in sickle_ui, just pointing out the practical reason you have missed. This kind of usage has a precedent in App, so I don't think we need to restrict it to traditional builders. In fact, I'd argue that App and Commands can both be seen as a kind of builder for a queue, but that's not really important. I'd like to keep the API in place for the RC, see how the ecosystem at large reacts to it (especially people porting apps, not only libraries) and then decide whether to cut it or not from the actual release. I myself have not played around with the change too much, so I'm fairly neutral, but I do like the diff in the PR and would like to give it at least a chance.

Just to point out the chaining changes in the PR are due to a new method that removes the need for the external IFs. The new method would work with a &mut self as well.

[tim-blackbird]

The addition of insert_if is indeed nice. I was commenting purely on methods now consuming self. I realize now that I wasn't clear about that in my earlier comments, sorry about that

Within the changes in this PR I counted 6 cases where the change to consuming self enabled chaining. On the other hand I counted 15 instances where doing re-assignment (entity_commands = entity_commands.XXX()) was now necessary.

IMO, that's not looking great.

I'd like to keep the API in place for the RC, see how the ecosystem at large reacts to it

Right, we do have release candidates now. That does ease my worries a bit.

[janhohenheim]

@eidloi @tim-blackbird those are very valid arguments 👀