Add .push_child, .insert_child and .remove_child to child-building API

[alice-i-cecile]

What problem does this solve or what need does it fill?

Adding a single child is cumbersome: using .push_children requires wrapping the entity in a &[entity] code block.

.with_children is even worse, largely because of the boilerplate and rightward drift it creates.

What solution would you like?

Add .push_child, .insert_child and .remove_child to child-building API. These should each accept a single Entity.

What alternative(s) have you considered?

Implement relations (#3742).

Additional context

Relevant to discussions in #3852.

[cart]

Is this:

fn setup(mut commands: Commands) {
    let child_1 = commands
        .spawn_bundle(PbrBundle {
            transform: Transform::from_xyz(1.0, 1.0, 1.0),
            ..Default::default()
        })
        .id();
    let child_2 = commands
        .spawn_bundle(PbrBundle {
            transform: Transform::from_xyz(2.0, 2.0, 2.0),
            ..Default::default()
        })
        .id();
    commands
        .spawn_bundle(PbrBundle {
            transform: Transform::from_xyz(1.0, 1.0, 1.0),
            ..Default::default()
        })
        .push_child(child_1)
        .push_child(child_1);
}

Really preferable to this?

fn setup(mut commands: Commands) {
    commands
        .spawn_bundle(PbrBundle {
            transform: Transform::from_xyz(1.0, 1.0, 1.0),
            ..Default::default()
        })
        .with_children(|parent| {
            parent
                .spawn_bundle(PbrBundle {
                    transform: Transform::from_xyz(1.0, 1.0, 1.0),
                    ..Default::default()
                })
                .spawn_bundle(PbrBundle {
                    transform: Transform::from_xyz(2.0, 2.0, 2.0),
                    ..Default::default()
                })
        });
}

The latter is terser. And imo nesting makes it much easier to reason about the relationship between children and parents. Theres a reason we indent child divs in html.

[alice-i-cecile]

Personally, yes. The closure adds a lot of abstraction overhead.

The trick is to split apart the entity definition and hierarchy definition. This lets readers see exactly how the hierarchy is constructed in one place, without getting distracted by the details of definition.

The problem with nesting is that in real use cases the entity definitions get too long, and so you can't actually see the structure.

[cart]

I do agree that having the option to break up / un-nest definitions when they get unwieldy makes sense (large numbers of children or very deep nesting). However for anything with <= ~3 levels of nesting or <= ~5 children, I think a single closure + nesting is significantly clearer. In addition to the points made above, using push_child serves to "invert" the hierarchy, as it forces you to define children above their parents. This actively abstracts the "top down" truth of the situation. I would oppose refactors for anything but the most complex hierarchies (like the ui example).

How do you propose splitting out the hierarchy definition from entity definition with the new methods? What would that look like in practice? I do think it makes sense to explore that design space.

[ghost]

Personal opinion

Nesting

My preferred way of handling this would be to be able to have clean nesting capabilities and also a way to break up hierarchies. Usually someone wouldn't want one huge hierarchy with every single UI element of the game in there (at least I wouldn't want that). It would be nice to be able to break up large hierarchies into smaller ones (maybe 3-5 levels like cart already mentioned) and add them back up afterwards. The 3-5 hierarchy levels work really well with nesting and make it easy to understand the hierarchy.

Indentation

One thing I don't like about the current implementation is the fact that you have one additional indentation for every level in the hierarchy. This makes it less readable in my opinion. If we go back to the comparison with HTML then we don't have this additional indentation for every hierarchy level there which makes it way more readable. Especially large hierarchies are affected by this.

Boilerplate

If we use the with_children function, it is obvious that we want to add everything to the parent. Therefore I think someone shouldn't be forced to write it out every single time. I haven't thought about how to implement this yet so take this with a grain of salt, but I just wanted to throw this in here, because I think that this would make the UI way more user friendly and easier to work with.

Splitting entity definition and hierarchies

I really like this approach, because entity definitions can get pretty huge and can hurt the readability of the hierarchy. This is probably more of a problem for larger hierarchies, but still something to think about. The following would be an example of how this might look like:

// Entity definitions
let parent = commands
    .spawn_bundle(PbrBundle {
        transform: Transform::from_xyz(1.0, 1.0, 1.0),
        // Imagine another definition here
        // Imagine another definition here
        // Imagine another definition here
        ..Default::default()
    })
    .id();
let child_one = commands
    .spawn_bundle(PbrBundle {
        transform: Transform::from_xyz(1.0, 1.0, 1.0),
        // Imagine another definition here
        // Imagine another definition here
        // Imagine another definition here
        ..Default::default()
    })
    .id();
let child_two = commands
    .spawn_bundle(PbrBundle {
        transform: Transform::from_xyz(1.0, 1.0, 1.0),
        // Imagine another definition here
        // Imagine another definition here
        // Imagine another definition here
        ..Default::default()
    })
    .id();
let child_three = commands
    .spawn_bundle(PbrBundle {
        transform: Transform::from_xyz(1.0, 1.0, 1.0),
        // Imagine another definition here
        // Imagine another definition here
        // Imagine another definition here
        ..Default::default()
    })
    .id();

// Hierarchy definition
commands
    .entity(parent)
    .insert_child(child_one)
    .insert_child(child_two)
    .insert_child(child_three);

Notice that we still have a "top-down" approach even though we split the entity definitions and the hierarchy definition. I'm currently not sure how this would look like with multiple hierarchy levels. Would it look something like the following or is there a better approach? The following would break the "top-down" approach sadly.

commands
    .entity(parent)
    .insert_child(child_one)
    .insert_child(child_two)
    .insert_child(child_three);

commands
    .entity(child_one)
    .insert_child(child_one_one)
    .insert_child(child_one_two)
    .insert_child(child_one_three);

// ...

Comparison

I made a small comparison how a hierarchy with three levels would look like if we wouldn't have the additional indentation or the additional parent closure or the additional ..Default::default calls. Like I said above I haven't thought about how to implement this so take this with a grain of salt.

Warning

These examples aren't working!
They are just here to show the way hierarchies would look like!

Current state

commands
    .spawn_bundle(PbrBundle {
        transform: Transform::from_xyz(1.0, 1.0, 1.0),
        ..Default::default()
    })
    .with_children(|parent| {
        parent
            .spawn_bundle(PbrBundle {
                transform: Transform::from_xyz(1.0, 1.0, 1.0),
                ..Default::default()
            })
            .insert(ChildComponent::One)
            .with_children(|parent| {
                parent
                    .spawn_bundle(PbrBundle {
                        transform: Transform::from_xyz(1.0, 1.0, 1.0),
                        ..Default::default()
                    })
                    .insert(ChildComponent::Two)
                    .with_children(|parent| {
                        parent
                            .spawn_bundle(PbrBundle {
                                transform: Transform::from_xyz(1.0, 1.0, 1.0),
                                ..Default::default()
                            })
                            .insert(ChildComponent::Three);
                    });
            });
    });

No parent closure; Single indentation

commands
    .spawn_bundle(PbrBundle {
        transform: Transform::from_xyz(1.0, 1.0, 1.0),
        ..Default::default()
    })
    .with_children(
        .spawn_bundle(PbrBundle {
            transform: Transform::from_xyz(1.0, 1.0, 1.0),
            ..Default::default()
        })
        .insert(ChildComponent::One)
        .with_children(
            .spawn_bundle(PbrBundle {
                transform: Transform::from_xyz(1.0, 1.0, 1.0),
                ..Default::default()
            })
            .insert(ChildComponent::Two)
            .with_children(
                .spawn_bundle(PbrBundle {
                    transform: Transform::from_xyz(1.0, 1.0, 1.0),
                    ..Default::default()
                })
                .insert(ChildComponent::Three);
            );
        );
    );

No parent closure; Single indentation; No ..Default::default()

commands
    .spawn_bundle(PbrBundle {
        transform: Transform::from_xyz(1.0, 1.0, 1.0),
    })
    .with_children(
        .spawn_bundle(PbrBundle {
            transform: Transform::from_xyz(1.0, 1.0, 1.0),
        })
        .insert(ChildComponent::One)
        .with_children(
            .spawn_bundle(PbrBundle {
                transform: Transform::from_xyz(1.0, 1.0, 1.0),
            })
            .insert(ChildComponent::Two)
            .with_children(
                .spawn_bundle(PbrBundle {
                    transform: Transform::from_xyz(1.0, 1.0, 1.0),
                })
                .insert(ChildComponent::Three);
            );
        );
    );

[cart]

If Bundles were promoted to have "prefab-like" properties (ex: the ability to spawn children and insert more floating components or bundles), we could have this api:

commands.spawn_bundle(
    PbrBundle {
        transform: Transform::from_xyz(1.0, 1.0, 1.0),
        ..Default::default()
    }
    .insert_component(ChildComponent::One)
    .spawn_child(
        PbrBundle {
            transform: Transform::from_xyz(1.0, 1.0, 1.0),
            ..Default::default()
        }
        .spawn_child(
            PbrBundle {
                transform: Transform::from_xyz(1.0, 1.0, 1.0),
                ..Default::default()
            }
            .insert_component(ChildComponent::Two)
            .spawn_child(
                PbrBundle {
                    transform: Transform::from_xyz(1.0, 1.0, 1.0),
                    ..Default::default()
                }
                .insert_component(ChildComponent::Three),
            ),
        ),
    ),
);

Note that this actually compiles today if you add these traits:

trait SpawnChild: Bundle + Sized {
    fn spawn_child<C: Bundle>(self, child: C) -> AddChildBundle<Self, C> {
        todo!()
    }
}

impl<T: Bundle> SpawnChild for T {}

pub struct AddChildBundle<T: Bundle, C: Bundle> {
    bundle: T,
    child: C,
}

unsafe impl<T: Bundle, C: Bundle> Bundle for AddChildBundle<T, C> {
    fn component_ids(
        components: &mut bevy::ecs::component::Components,
        storages: &mut bevy::ecs::storage::Storages,
    ) -> Vec<bevy::ecs::component::ComponentId> {
        todo!()
    }

    unsafe fn from_components(func: impl FnMut() -> *mut u8) -> Self
    where
        Self: Sized,
    {
        todo!()
    }

    fn get_components(self, func: impl FnMut(*mut u8)) {
        todo!()
    }
}

trait InsertComponent: Bundle + Sized {
    fn insert_component<C: Component>(self, component: C) -> InsertBundleComponent<Self, C> {
        todo!()
    }
}

impl<T: Bundle> InsertComponent for T {}

pub struct InsertBundleComponent<T: Bundle, C: Component> {
    bundle: T,
    component: C,
}

unsafe impl<T: Bundle, C: Component> Bundle for InsertBundleComponent<T, C> {
    fn component_ids(
        components: &mut bevy::ecs::component::Components,
        storages: &mut bevy::ecs::storage::Storages,
    ) -> Vec<bevy::ecs::component::ComponentId> {
        todo!()
    }

    unsafe fn from_components(func: impl FnMut() -> *mut u8) -> Self
    where
        Self: Sized,
    {
        todo!()
    }

    fn get_components(self, func: impl FnMut(*mut u8)) {
        todo!()
    }
}

[ghost]

Thoughts

I really love this API. This is way better and more user-friendly than the current implementation. It removes the double indentation for every hierarchy level and also the parent closure reducing boilerplate and increasing readability significantly.

If implementing what you suggested or something similar is feasible I'd be more than happy to see this make it in. This would make everything bundle related easier to use which is lovely. Spawning a player with a child sprite would be as simple as the following which pretty much has zero boilerplate.

#[derive(Component)]
struct Health(f32);

#[derive(Component)]
struct Speed(f32);

#[derive(Bundle)]
struct PlayerBundle {
    health: Health,
    speed: Speed,
}

// New way of doing it
fn setup_new(mut commands: Commands, asset_server: Res<AssetServer>) {
    commands.spawn_bundle(
        PlayerBundle {
            health: Health(100.0),
            speed: Speed(1.0),
        }
        .spawn_child(SpriteBundle {
            texture: asset_server.load("sprite.png"),
            ..Default::default()
        }),
    );
}

// Old way of doing it
fn setup_old(mut commands: Commands, asset_server: Res<AssetServer>) {
    commands
        .spawn_bundle(PlayerBundle {
            health: Health(100.0),
            speed: Speed(1.0),
        })
        .with_children(|parent| {
            parent.spawn_bundle(SpriteBundle {
                texture: asset_server.load("sprite.png"),
                ..Default::default()
            });
        });
}

Comparison

I made another comparison of a decently large hierarchy using our current implementation and the one cart suggested which is way less noisy.

Current state

commands
    .spawn_bundle(PbrBundle {
        transform: Transform::from_xyz(1.0, 1.0, 1.0),
        ..Default::default()
    })
    .with_children(|parent| {
        parent
            .spawn_bundle(PbrBundle {
                transform: Transform::from_xyz(1.0, 1.0, 1.0),
                ..Default::default()
            })
            .insert(ChildComponent::One)
            .with_children(|parent| {
                parent
                    .spawn_bundle(PbrBundle {
                        transform: Transform::from_xyz(1.0, 1.0, 1.0),
                        ..Default::default()
                    })
                    .insert(ChildComponent::Two)
                    .with_children(|parent| {
                        parent
                            .spawn_bundle(PbrBundle {
                                transform: Transform::from_xyz(1.0, 1.0, 1.0),
                                ..Default::default()
                            })
                            .insert(ChildComponent::Three);
                    });
            })
            .with_children(|parent| {
                parent
                    .spawn_bundle(PbrBundle {
                        transform: Transform::from_xyz(1.0, 1.0, 1.0),
                        ..Default::default()
                    })
                    .insert(ChildComponent::Two);
            });
    })
    .with_children(|parent| {
        parent
            .spawn_bundle(PbrBundle {
                transform: Transform::from_xyz(1.0, 1.0, 1.0),
                ..Default::default()
            })
            .insert(ChildComponent::One)
            .with_children(|parent| {
                parent
                    .spawn_bundle(PbrBundle {
                        transform: Transform::from_xyz(1.0, 1.0, 1.0),
                        ..Default::default()
                    })
                    .insert(ChildComponent::Two)
                    .with_children(|parent| {
                        parent
                            .spawn_bundle(PbrBundle {
                                transform: Transform::from_xyz(1.0, 1.0, 1.0),
                                ..Default::default()
                            })
                            .insert(ChildComponent::Three);
                    });
            })
    });

Using the suggestion from cart

commands.spawn_bundle(
    PbrBundle {
        transform: Transform::from_xyz(1.0, 1.0, 1.0),
        ..Default::default()
    }
    .spawn_child(
        PbrBundle {
            transform: Transform::from_xyz(1.0, 1.0, 1.0),
            ..Default::default()
        }
        .insert_component(ChildComponent::One)
        .spawn_child(
            PbrBundle {
                transform: Transform::from_xyz(1.0, 1.0, 1.0),
                ..Default::default()
            }
            .insert_component(ChildComponent::Two)
            .spawn_child(
                PbrBundle {
                    transform: Transform::from_xyz(1.0, 1.0, 1.0),
                    ..Default::default()
                }
                .insert_component(ChildComponent::Three),
            ),
        )
        .spawn_child(
            PbrBundle {
                transform: Transform::from_xyz(1.0, 1.0, 1.0),
                ..Default::default()
            }
            .insert_component(ChildComponent::Two),
        ),
    )
    .spawn_child(
        PbrBundle {
            transform: Transform::from_xyz(1.0, 1.0, 1.0),
            ..Default::default()
        }
        .insert_component(ChildComponent::One)
        .spawn_child(
            PbrBundle {
                transform: Transform::from_xyz(1.0, 1.0, 1.0),
                ..Default::default()
            }
            .insert_component(ChildComponent::Two)
            .spawn_child(
                PbrBundle {
                    transform: Transform::from_xyz(1.0, 1.0, 1.0),
                    ..Default::default()
                }
                .insert_component(ChildComponent::Three),
            ),
        ),
    ),
);

[alice-i-cecile]

Closing this out; this whole API needs a full RFC.