Skip to content

Conversation

janis-bhm
Copy link
Contributor

Objective

works on #18238

Solution

convert calls to .with_children to use the Children::spawn or Children::spawn_one types or children! macro.
This touches the window, 2d, animation folders, as well as ecs/one_shot_systems.rs.
observer_propagation.rs looks like exactly what with_children is useful for, so I deliberately haven't touched it.
I can break this up into more PRs or squash if desired.

Testing

I've run the examples before and after this patch and verified visually that nothing has changed

..Default::default()
},
Transform::from_translation(250. * Vec3::Y),
Children::spawn(SpawnIter(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a fan of this one, but won't block on it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I am either but I also didn't know about this functionality so maybe it's worth it!

Copy link
Contributor Author

@janis-bhm janis-bhm Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the ecs/hierarchy.rs example would lend itself quite well to demonstrating the different possible methods for inserting children/handling relations. Would you be more against having this in this example if that example included a demonstration of SpawnIter?

Copy link
Member

@janhohenheim janhohenheim Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API in this case just looks clunky to me, no matter if it's also used in other places. But that might just be me being more used to the "old" way :)

@janhohenheim janhohenheim added C-Examples An addition or correction to our examples D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward A-ECS Entities, components, systems, and events labels Sep 5, 2025
Copy link
Member

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quite a significant improvement!

@tychedelia tychedelia added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 5, 2025
Copy link
Contributor

@hukasu hukasu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i know that this new style can cause issues with lifetimes, as they are lazy in some parts, so i don't know if the uses of Children::spawn_one that i highlighted are intended

the ones using SpawnIter could be using SpawnableList

commands.spawn(Children::spawn(MySpawnList(vec![(...),(...),(...),(...)])));

struct MySpawnList(Vec<(...)>);

impl SpawnableList<ChildOf> for MySpawnList {
    fn size_hint(&self) -> usize {
        self.0.len()
    }

    fn spawn(self, world: &mut World, entity: Entity) {
        for (...) in self.0 {
            world.spawn((
                ChildOf(entity),
                (...),
                ...
            ));
        }
    }
}

Comment on lines 159 to 178
Children::spawn_one((
Transform::default(),
Visibility::default(),
orbit_controller,
AnimationTarget {
id: orbit_controller_animation_target_id,
player: planet_entity,
},
Children::spawn_one((
Mesh3d(meshes.add(Cuboid::new(0.5, 0.5, 0.5))),
MeshMaterial3d(materials.add(Color::srgb(0.3, 0.9, 0.3))),
Transform::from_xyz(1.5, 0.0, 0.0),
AnimationTarget {
id: orbit_controller_animation_target_id,
id: satellite_animation_target_id,
player: planet_entity,
},
))
.with_children(|p| {
// The satellite, placed at a distance of the planet
p.spawn((
Mesh3d(meshes.add(Cuboid::new(0.5, 0.5, 0.5))),
MeshMaterial3d(materials.add(Color::srgb(0.3, 0.9, 0.3))),
Transform::from_xyz(1.5, 0.0, 0.0),
AnimationTarget {
id: satellite_animation_target_id,
player: planet_entity,
},
satellite,
));
});
});
satellite,
)),
)),
));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What stops these from being children!?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nothing, really, I just didn't see the point of using the macro here when it's just a single child. I'm not sure I see why using children! everywhere, even when it results in more indirection (which might get compiled out), is better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have a vague memory of someone saying that children! should always take precedence over Children::spawn or Children::spawn_one

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally think using children! everywhere is preferable in learning material to lower the complexity for beginners.

Comment on lines 143 to 157
entity.insert(Children::spawn_one((
Text::new("Bevy"),
TextFont {
font: asset_server.load("fonts/FiraSans-Bold.ttf"),
font_size: 24.0,
..default()
},
TextColor(Color::Srgba(Srgba::RED)),
TextLayout::new_with_justify(Justify::Center),
AnimationTarget {
id: animation_target_id,
player,
},
animation_target_name,
)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this one be children!?

Comment on lines 322 to 335
Children::spawn_one((
Text(format!("{label:?}")),
if index > 0 {
button_text_style.clone()
} else {
selected_button_text_style.clone()
},
TextLayout::new_with_justify(Justify::Center),
Node {
flex_grow: 1.0,
margin: UiRect::vertical(px(3)),
..default()
},
)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

children!?

@james7132 james7132 added this pull request to the merge queue Sep 6, 2025
Merged via the queue into bevyengine:main with commit 6227064 Sep 6, 2025
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Examples An addition or correction to our examples D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants