command based entry api with EntityCommands::entry

[atornity]

Objective

It's convenient to be able to modify a component if it exist, and insert a default value if it doesn't. You can already do most of this with EntityCommands::insert_if_new, and all of this using a custom command. However, that does not spark joy in my opinion.

Closes #10669

Solution

Introduce a new commands type EntityEntryCommands, along with a method to access it, EntityCommands::entry.

EntityEntryCommands exposes a subset of the entry API (and_modify, or_insert, etc), however it's not an enum so it doesn't allow pattern matching. Also, or_insert won't return the component because it's all based on commands.

Testing

Added a new test entity_commands_entry.

---

Showcase

commands
    .entity(player)
    .entry::<Level>()
    .and_modify(|mut lvl| lvl.0 += 1)
    .or_default();

[MiniaczQ]

@benfrankel

[alice-i-cecile]

Can you say more about why this isn't an enum? I quite liked that design from the standard library and would prefer it here too.

[atornity]

Can you say more about why this isn't an enum? I quite liked that design from the standard library and would prefer it here too.

I don't think we can know if the component is present is all (not before actually executing the commands), so we can't know if it should be the Vacant or Occupied variant.

[alice-i-cecile]

Right, the deferred nature of it complicates things. We'd want to change which commands get queued or executed based on a branch that's taken later, which is way harder.

[atornity]

Can you say more about why this isn't an enum? I quite liked that design from the standard library and would prefer it here too.

I don't think we can know if the component is present is all (not before actually executing the commands), so we can't know if it should be the Vacant or Occupied variant.

Altough, this would work

commands
    .entity(entity)
    .entry::<MyComponent>(|entry| {
        // use entry as you would `EntityWorldMut::entry`
    });

[alice-i-cecile]

I really like this. I think we should also add an or_from_world variant, and I have another tiny suggestion.

[atornity]

wait, it says the "expect" thingy is experimental, I'll just revert that I think

[alice-i-cecile]

We can bump the MSRV and use expect, but I won't block on that being done in this PR. See #15118 for more info.

I'll merge this by next Monday at the latest.

[atornity]

Can you say more about why this isn't an enum? I quite liked that design from the standard library and would prefer it here too.

I don't think we can know if the component is present is all (not before actually executing the commands), so we can't know if it should be the Vacant or Occupied variant.

If Commands had access to Archetypes and Components this would actually be possible. However, it would only work if the component is already registered.

[alice-i-cecile]

Even with access to that, it's still not correct. That would give you data about the state at the time the command is sent. But what we need is information at the time the command is evaluated, which may not match.

[atornity]

That would give you data about the state at the time the command is sent. But what we need is information at the time the command is evaluated, which may not match.

Aah yea, good point

[janhohenheim]

Lovely bit of API! Could we remove the _if_new family of methods in the same PR, @alice-i-cecile? I would like to not have two ways of doing the same thing. _if_new has not landed in a release yet, so no need to deprecate it.
Edit: nvm, this API does not work with bundles. In that case, could you add some breadcrumbs to _if_new about using this API for Component and some breadcrumbs here pointing to _if_new for Bundles?

[atornity]

could you add some breadcrumbs to _if_new about using this API for Component and some breadcrumbs here pointing to _if_new for Bundles?

Yep, I'll do that once I get home

[janhohenheim]

Thanks! Left some grammar corrections, then this is good to go :)

[janhohenheim]

Thanks for addressing my comments :)

[atornity]

Should I add or_try_insert?

[janhohenheim]

Ah heck, NameOrEntity is in Bevy Core, not Bevy ECS. You can revert that suggestion :/

[janhohenheim]

or_try_insert

That'd be nice, yes :)

[janhohenheim]

Maybe for completeness we should also have or_try_insert_with?