Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Insert children #17558

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

bjoernp116
Copy link

Objective

This PR implements the requested, previously deprecated feature from #17478. Which is concerning the insert_children of EntityWorldMut. This method inserts children into the tree, at a specific index.

Solution

I mostly copied the code from the deprecated crate bevy_hierarchy though this is no longer a trait method.
This code also used SmallVec which has later been switched by Vec. This was easily interchangeable with working code.

Testing

I wrote a unit test covering both EntityWorldMut::add_entities and EntityWorldMut::insert_entities, simply called hierarchy::tests::insert_children. I was unsure what error handling architecture i should be using, but i think the simplest to debug in this case would just be a panic in the case of an index out of bounds.

Showcase

I find this feature pretty self exoplanetary, but the unit test can also bee informing about what this feature implements:

Click to view showcase
let mut world = World::new();
let child1 = world.spawn_empty().id();
let child2 = world.spawn_empty().id();
let child3 = world.spawn_empty().id();
let child4 = world.spawn_empty().id();

let mut entity_world_mut = world.spawn_empty();

let first_children = entity_world_mut.add_children(&[child1, child2]);

let root = first_children.insert_children(1, &[child3, child4]).id();

let hierarchy = get_hierarchy(&world, root);
assert_eq!(
     hierarchy,
     Node::new_with(root, vec![Node::new(child1), Node::new(child3), Node::new(child4), Node::new(child2)])
);

Copy link
Contributor

Welcome, new contributor!

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

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events X-Uncontroversial This work is generally agreed upon D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 27, 2025
@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Jan 27, 2025
@BenjaminBrienen
Copy link
Contributor

Fixes #17478

@@ -185,6 +185,33 @@ impl<'w> EntityWorldMut<'w> {
self.add_related::<ChildOf>(children)
}

/// Insert children at specific index
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the entity is already a child? As a user, I'd like to know whether that is okay ok or not. For example, does doing so reorder it?

);
}
children_component.0.reserve(children.len());
let mut v = children_component.0.split_off(index);
Copy link
Member

Choose a reason for hiding this comment

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

This does not update the other side of the relationship (ChildOf), invalidating the assumptions of the system as a whole.

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 27, 2025
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 D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants