Better API for Commands methods that modify components

[alice-i-cecile]

Currently, we can modify components via Commands using .insert, .insert_one, .remove and .remove_one.

This is confusing to beginners, and makes the code less legible.

I suggest we rename these to .insert_component etc, to be more clear and sync up with .insert_resource and friends.

Are we also able to condense the bundle / single component versions of these in some way? I suspect the issue is similar to query.get and query.get_component, where there were small performance differences, but it would be a nice simplification.

[bjorn3]

Are we also able to condense the bundle / single component versions of these in some way

That would make it ambiguous if you wanted to insert a bundle of components or a single component that just so happens to have a type that could be a bundle too.

[mockersf]

Are we also able to condense the bundle / single component versions of these in some way?

Sadly a few of us tried that one way or another, without a nice success yet. There are hacky way abusing rust trait bounds, but nothing clean until we have at least negative trait bound stable I think

[Lythenas]

(This is maybe not directly related or maybe more interesting for #1251)

I would also suggest pulling the APIs of Commands and ChildBuilder into a trait. That would make it a little easier to write helper functions that work both to spawn something at the root level and as a child. Currently I would have to make two identical functions one taking Commands and one taking ChildBuilder.

Alternatively (and I think the better option) would be to make commands more composable in general. And make it easier to create the "raw" commands (Insert<T>, InsertChildren, etc). E.g. then I would write a function that simply returns a command (composed of some base commands) instead of taking Commands as a parameter. I.e.

fn spawn_my_button(button_text: String) -> impl Command {}
commands.add_command(spawn_my_button("Button text"));

instead of

fn spawn_my_button(commands: &mut Commands, button_text: String) {}
spawn_my_button(commands, "Button text");

But this requires a lot more design and I haven't really thought any of this through.

[alice-i-cecile]

As part of this, we need to remember to also change the API of World to ensure that it stays synced :)

[cart]

Relevant: My bevy_ecs core rewrite (currently) looks like this for entity creation:

world.spawn()
  .insert_bundle(SomeBundle::default())
  .insert(SomeComponent::default());

world.spawn() creates a new Entity and returns an EntityMut (which has insert_bundle, insert, remove_bundle, remove, etc)

This has a number of benefits:

1. Cuts down on redundant Entity location lookups for multiple operations (because EntityMut stores the location)
2. Improved clarity around Bundles
3. Reusable. Calling world.entity_mut(entity) also returns an EntityMut (and world.entity(entity) returns an EntityRef with read-only operations).

Imo entities are "collections of components", so insert/remove/get operations should be "component operations". Bundles are "higher level" constructs, so they need explicit terminology.

[tigregalis]

My suggestions are as follows. Terminology:

• Insert Resource / Spawn Entity == Create
• Add Component(s) / Remove Component(s) == Update
• Despawn Entity == Delete

// impl Commands
add_command               -> push
add_command_boxed         -> push_boxed
apply                     -> 
clear_current_entity      -> 
current_entity            -> 
despawn                   -> 
for_current_entity        -> 
insert                    -> add_bundle
insert_local_resource     -> 
insert_one                -> add_component
insert_resource           -> 
remove                    -> remove_bundle
remove_one                -> remove_component
set_current_entity        -> with_entity
set_entity_reserver       -> 
spawn                     -> spawn_bundle
spawn_batch               -> 
with                      -> with_component
with_bundle               ->
// impl BuildChildren for Commands
insert_children           ->
push_children             ->
with_children             ->
// impl DespawnRecursiveExt for Commands
despawn_recursive         ->
// impl SpawnSceneCommands for Commands
spawn_scene               ->

[cart]

Imo "add" is the wrong verb for the operation happening, especially in a rust context. "adding" is for adding something that was previously not a part of the object. "insert" in rust collections is an "upsert" (create or update), which is the operation that we perform for components / bundles.

[alice-i-cecile]

2. Improved clarity around Bundles

I like this, although I'm curious if it plays nice with spawn_batch. This could resolve #792 if it works well.

3. Reusable. Calling world.entity_mut(entity) also returns an EntityMut (and world.entity(entity) returns an EntityRef with read-only operations).

This is very much in line with my proposal of how we could get #1251 working, which is really nice :)

Imo entities are "collections of components", so insert/remove/get operations should be "component operations". Bundles are "higher level" constructs, so they need explicit terminology.

I agree with this pattern of thinking. The main challenge for me here is that while .insert is clear in a World context, it's substantially less so in Commands, where a) the internals of how the ECS works are a bit more removed and b) you can also modify resources.

Imo "add" is the wrong verb for the operation happening, especially in a rust context. "adding" is for adding something that was previously not a part of the object. "insert" in rust collections is an "upsert" (create or update), which is the operation that we perform for components / bundles.

Definitely think "insert" is the correct verb here. With the component-centric view, I also like keeping spawn and despawn around, because it indicates that something a little stranger is happening when you're operating on entities as a whole.

add_command -> push

I like the change from "add_command" to "push", although see the discussion in #1251 and the event API in #1244 for more context :)

[alice-i-cecile]

This should be improved with #1562.