Skip to content

Fix bugs/inconsistencies with adding child entities #4706

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Fix bugs/inconsistencies with adding child entities #4706

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2641cd4
Fix bugs/inconsistencies with adding child entities
SUPERCILEX May 9, 2022
113965c
Remove PreviousParent in place
SUPERCILEX May 9, 2022
d8b2ab8
Merge remote-tracking branch 'upstream/main' into children
SUPERCILEX Jun 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
257 changes: 115 additions & 142 deletions crates/bevy_hierarchy/src/child_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,69 @@ use bevy_ecs::{

use crate::prelude::{Children, Parent, PreviousParent};

fn push_child_unchecked(world: &mut World, parent: Entity, child: Entity) {
let mut parent = world.entity_mut(parent);
if let Some(mut children) = parent.get_mut::<Children>() {
children.0.push(child);
} else {
parent.insert(Children(smallvec::smallvec![child]));
}
}

fn update_parent(world: &mut World, child: Entity, new_parent: Entity) -> Option<Entity> {
let mut child = world.entity_mut(child);
if let Some(mut parent) = child.get_mut::<Parent>() {
let previous = parent.0;
*parent = Parent(new_parent);
child.insert(PreviousParent(new_parent));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
child.insert(PreviousParent(new_parent));
child.insert(PreviousParent(previous));

shouldn't that be the previous parent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. The goal is to hit this line:

bevy/crates/bevy_hierarchy/src/systems.rs

Line 34 in 86dd6f0

if previous_parent.0 == parent.0 {

Copy link
Member

Choose a reason for hiding this comment

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

I think it's wrong to set a wrong value to a component because you target a special case for a system that happens after. If you insert the PreviousParent component, it should have the correct value at the moment of insertion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does match, that is the correct value. PreviousParent is supposed to match Parent unless someone manually inserts a new Parent.

Some(previous)
} else {
child.insert_bundle((Parent(new_parent), PreviousParent(new_parent)));
None
}
}

fn remove_from_children(world: &mut World, parent: Entity, child: Entity) {
let mut parent = world.entity_mut(parent);
if let Some(mut children) = parent.get_mut::<Children>() {
if let Some(idx) = children.iter().position(|x| *x == child) {
children.0.remove(idx);
if children.is_empty() {
parent.remove::<Children>();
}
}
}
}

fn update_old_parents(world: &mut World, parent: Entity, children: &[Entity]) {
for child in children {
if let Some(previous) = update_parent(world, *child, parent) {
remove_from_children(world, previous, *child);
}
}
}

fn remove_children(parent: Entity, children: &[Entity], world: &mut World) {
for child in children.iter() {
let mut child = world.entity_mut(*child);
if let Some(child_parent) = child.get::<Parent>() {
if child_parent.0 == parent {
child.remove_bundle::<(Parent, PreviousParent)>();
}
}
}

let mut parent = world.entity_mut(parent);
if let Some(mut parent_children) = parent.get_mut::<Children>() {
parent_children
.0
.retain(|parent_child| !children.contains(parent_child));
if children.is_empty() {
parent.remove::<Children>();
}
}
}

/// Command that adds a child to an entity
#[derive(Debug)]
pub struct AddChild {
Expand All @@ -20,16 +83,22 @@ pub struct AddChild {

impl Command for AddChild {
fn write(self, world: &mut World) {
world
.entity_mut(self.child)
// FIXME: don't erase the previous parent (see #1545)
.insert_bundle((Parent(self.parent), PreviousParent(self.parent)));
if let Some(mut children) = world.get_mut::<Children>(self.parent) {
children.0.push(self.child);
let previous = update_parent(world, self.child, self.parent);
if let Some(previous) = previous {
if previous == self.parent {
return;
}

remove_from_children(world, previous, self.child);
}

let mut parent = world.entity_mut(self.parent);
if let Some(mut children) = parent.get_mut::<Children>() {
if !children.contains(&self.child) {
children.0.push(self.child);
}
} else {
world
.entity_mut(self.parent)
.insert(Children(smallvec::smallvec![self.child]));
parent.insert(Children(smallvec::smallvec![self.child]));
}
}
}
Expand All @@ -44,20 +113,13 @@ pub struct InsertChildren {

impl Command for InsertChildren {
fn write(self, world: &mut World) {
for child in self.children.iter() {
world
.entity_mut(*child)
// FIXME: don't erase the previous parent (see #1545)
.insert_bundle((Parent(self.parent), PreviousParent(self.parent)));
}
{
if let Some(mut children) = world.get_mut::<Children>(self.parent) {
children.0.insert_from_slice(self.index, &self.children);
} else {
world
.entity_mut(self.parent)
.insert(Children(self.children));
}
update_old_parents(world, self.parent, &self.children);
let mut parent = world.entity_mut(self.parent);
if let Some(mut children) = parent.get_mut::<Children>() {
children.0.retain(|value| !self.children.contains(value));
children.0.insert_from_slice(self.index, &self.children);
} else {
parent.insert(Children(self.children));
}
}
}
Expand All @@ -71,26 +133,13 @@ pub struct PushChildren {

impl Command for PushChildren {
fn write(self, world: &mut World) {
for child in self.children.iter() {
world
.entity_mut(*child)
// FIXME: don't erase the previous parent (see #1545)
.insert_bundle((Parent(self.parent), PreviousParent(self.parent)));
}
{
let mut added = false;
if let Some(mut children) = world.get_mut::<Children>(self.parent) {
children.0.extend(self.children.iter().cloned());
added = true;
}

// NOTE: ideally this is just an else statement, but currently that _incorrectly_ fails
// borrow-checking
if !added {
world
.entity_mut(self.parent)
.insert(Children(self.children));
}
update_old_parents(world, self.parent, &self.children);
let mut parent = world.entity_mut(self.parent);
if let Some(mut children) = parent.get_mut::<Children>() {
children.0.retain(|child| !self.children.contains(child));
children.0.extend(self.children);
} else {
parent.insert(Children(self.children));
}
}
}
Expand All @@ -101,32 +150,8 @@ pub struct RemoveChildren {
children: SmallVec<[Entity; 8]>,
}

fn remove_children(parent: Entity, children: &[Entity], world: &mut World) {
for child in children.iter() {
let mut child = world.entity_mut(*child);
let mut remove_parent = false;
if let Some(child_parent) = child.get_mut::<Parent>() {
if child_parent.0 == parent {
remove_parent = true;
}
}
if remove_parent {
if let Some(parent) = child.remove::<Parent>() {
child.insert(PreviousParent(parent.0));
}
}
}
// Remove the children from the parents.
if let Some(mut parent_children) = world.get_mut::<Children>(parent) {
parent_children
.0
.retain(|parent_child| !children.contains(parent_child));
}
}

impl Command for RemoveChildren {
fn write(self, world: &mut World) {
// Remove any matching Parent components from the children
remove_children(self.parent, &self.children, world);
}
}
Expand Down Expand Up @@ -288,13 +313,7 @@ impl<'w> WorldChildBuilder<'w> {
.insert_bundle((Parent(parent_entity), PreviousParent(parent_entity)))
.id();
self.current_entity = Some(entity);
if let Some(mut parent) = self.world.get_entity_mut(parent_entity) {
if let Some(mut children) = parent.get_mut::<Children>() {
children.0.push(entity);
} else {
parent.insert(Children(smallvec::smallvec![entity]));
}
}
push_child_unchecked(self.world, parent_entity, entity);
self.world.entity_mut(entity)
}

Expand All @@ -307,13 +326,7 @@ impl<'w> WorldChildBuilder<'w> {
.insert_bundle((Parent(parent_entity), PreviousParent(parent_entity)))
.id();
self.current_entity = Some(entity);
if let Some(mut parent) = self.world.get_entity_mut(parent_entity) {
if let Some(mut children) = parent.get_mut::<Children>() {
children.0.push(entity);
} else {
parent.insert(Children(smallvec::smallvec![entity]));
}
}
push_child_unchecked(self.world, parent_entity, entity);
self.world.entity_mut(entity)
}

Expand Down Expand Up @@ -361,16 +374,14 @@ impl<'w> BuildWorldChildren for EntityMut<'w> {
{
// SAFE: parent entity is not modified and its location is updated manually
let world = unsafe { self.world_mut() };
for child in children.iter() {
world
.entity_mut(*child)
// FIXME: don't erase the previous parent (see #1545)
.insert_bundle((Parent(parent), PreviousParent(parent)));
}
update_old_parents(world, parent, children);
// Inserting a bundle in the children entities may change the parent entity's location if they were of the same archetype
self.update_location();
}
if let Some(mut children_component) = self.get_mut::<Children>() {
children_component
.0
.retain(|value| !children.contains(value));
children_component.0.extend(children.iter().cloned());
} else {
self.insert(Children::with(children));
Expand All @@ -383,17 +394,15 @@ impl<'w> BuildWorldChildren for EntityMut<'w> {
{
// SAFE: parent entity is not modified and its location is updated manually
let world = unsafe { self.world_mut() };
for child in children.iter() {
world
.entity_mut(*child)
// FIXME: don't erase the previous parent (see #1545)
.insert_bundle((Parent(parent), PreviousParent(parent)));
}
update_old_parents(world, parent, children);
// Inserting a bundle in the children entities may change the parent entity's location if they were of the same archetype
self.update_location();
}

if let Some(mut children_component) = self.get_mut::<Children>() {
children_component
.0
.retain(|value| !children.contains(value));
children_component.0.insert_from_slice(index, children);
} else {
self.insert(Children::with(children));
Expand All @@ -405,26 +414,7 @@ impl<'w> BuildWorldChildren for EntityMut<'w> {
let parent = self.id();
// SAFE: This doesn't change the parent's location
let world = unsafe { self.world_mut() };
for child in children.iter() {
let mut child = world.entity_mut(*child);
let mut remove_parent = false;
if let Some(child_parent) = child.get_mut::<Parent>() {
if child_parent.0 == parent {
remove_parent = true;
}
}
if remove_parent {
if let Some(parent) = child.remove::<Parent>() {
child.insert(PreviousParent(parent.0));
}
}
}
// Remove the children from the parents.
if let Some(mut parent_children) = world.get_mut::<Children>(parent) {
parent_children
.0
.retain(|parent_child| !children.contains(parent_child));
}
remove_children(parent, children, world);
self
}
}
Expand All @@ -450,13 +440,11 @@ impl<'w> BuildWorldChildren for WorldChildBuilder<'w> {
let parent = self
.current_entity
.expect("Cannot add children without a parent. Try creating an entity first.");
for child in children.iter() {
self.world
.entity_mut(*child)
// FIXME: don't erase the previous parent (see #1545)
.insert_bundle((Parent(parent), PreviousParent(parent)));
}
update_old_parents(self.world, parent, children);
if let Some(mut children_component) = self.world.get_mut::<Children>(parent) {
children_component
.0
.retain(|value| !children.contains(value));
children_component.0.extend(children.iter().cloned());
} else {
self.world
Expand All @@ -470,14 +458,11 @@ impl<'w> BuildWorldChildren for WorldChildBuilder<'w> {
let parent = self
.current_entity
.expect("Cannot add children without a parent. Try creating an entity first.");

for child in children.iter() {
self.world
.entity_mut(*child)
// FIXME: don't erase the previous parent (see #1545)
.insert_bundle((Parent(parent), PreviousParent(parent)));
}
update_old_parents(self.world, parent, children);
if let Some(mut children_component) = self.world.get_mut::<Children>(parent) {
children_component
.0
.retain(|value| !children.contains(value));
children_component.0.insert_from_slice(index, children);
} else {
self.world
Expand Down Expand Up @@ -622,14 +607,8 @@ mod tests {
);
assert!(world.get::<Parent>(child1).is_none());
assert!(world.get::<Parent>(child4).is_none());
assert_eq!(
*world.get::<PreviousParent>(child1).unwrap(),
PreviousParent(parent)
);
assert_eq!(
*world.get::<PreviousParent>(child4).unwrap(),
PreviousParent(parent)
);
assert!(world.get::<PreviousParent>(child1).is_none());
assert!(world.get::<PreviousParent>(child4).is_none());
}

#[test]
Expand Down Expand Up @@ -691,14 +670,8 @@ mod tests {
);
assert!(world.get::<Parent>(child1).is_none());
assert!(world.get::<Parent>(child4).is_none());
assert_eq!(
*world.get::<PreviousParent>(child1).unwrap(),
PreviousParent(parent)
);
assert_eq!(
*world.get::<PreviousParent>(child4).unwrap(),
PreviousParent(parent)
);
assert!(world.get::<PreviousParent>(child1).is_none());
assert!(world.get::<PreviousParent>(child4).is_none());
}

#[test]
Expand Down