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.

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

Fix double parenting. #1545

Closed
wants to merge 15 commits into from
Closed

Fix double parenting. #1545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
0f8fc38
set PreviousParent when it exists
siler Feb 27, 2021
0937610
Merge remote-tracking branch 'upstream/main' into main
siler Mar 4, 2021
f725ff2
Merge remote-tracking branch 'upstream/main' into main
siler Mar 5, 2021
1a6f9b8
remove some dead code
siler Mar 5, 2021
ffedfd7
dropped a brace
siler Mar 5, 2021
94bb58e
remove vec
siler Mar 5, 2021
86fe77d
Merge remote-tracking branch 'upstream/main' into main
siler Mar 5, 2021
f7cadc2
Merge remote-tracking branch 'upstream/main' into main
siler Mar 6, 2021
80e39b0
Merge remote-tracking branch 'upstream/main' into main
siler Mar 9, 2021
bfebd92
Merge remote-tracking branch 'upstream/main' into main
siler Mar 9, 2021
9e37f9a
Merge remote-tracking branch 'upstream/main' into main
siler Mar 13, 2021
815ae0f
Merge remote-tracking branch 'upstream/main' into main
siler Mar 15, 2021
ea02255
Merge remote-tracking branch 'upstream/main' into main
siler Mar 18, 2021
c0d4423
remove duplicate Parent update, properly set PreviousParent
siler Mar 3, 2021
6fced49
fix remaining ECSv2 changes
siler Mar 18, 2021
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
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,6 @@ Cargo.lock
.cargo/config
.cargo/config.toml
/.idea
.iml
/.vscode
/benches/target
6 changes: 5 additions & 1 deletion crates/bevy_transform/src/hierarchy/child_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,13 @@ pub struct ChildBuilder<'a, 'b> {
impl Command for PushChildren {
fn write(self: Box<Self>, world: &mut World) {
for child in self.children.iter() {
let previous = match world.get::<Parent>(*child) {
Some(Parent(previous)) => *previous,
None => self.parent,
};
world
.entity_mut(*child)
.insert_bundle((Parent(self.parent), PreviousParent(self.parent)));
.insert_bundle((Parent(self.parent), PreviousParent(previous)));
}
{
let mut added = false;
Expand Down
33 changes: 3 additions & 30 deletions crates/bevy_transform/src/hierarchy/hierarchy_maintenance_system.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@ use bevy_ecs::{
query::Without,
system::{Commands, Query},
};
use bevy_utils::HashMap;
use smallvec::SmallVec;

pub fn parent_update_system(
mut commands: Commands,
Expand All @@ -24,9 +22,6 @@ pub fn parent_update_system(
}
}

// Tracks all newly created `Children` Components this frame.
let mut children_additions = HashMap::<Entity, SmallVec<[Entity; 8]>>::default();

// Entities with a changed Parent (that also have a PreviousParent, even if None)
for (entity, parent, possible_previous_parent) in parent_query.iter_mut() {
if let Some(mut previous_parent) = possible_previous_parent {
Expand All @@ -45,31 +40,7 @@ pub fn parent_update_system(
} else {
commands.insert(entity, PreviousParent(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.

Wont removing this break manual insertion of the Parent(Entity) component (or changing the parent value directly)? If so, that would be a pretty major breaking change.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I think it would. But I would make the argument that mutably interacting with those is broken anyway. It only works for a narrow - if, perhaps, "typical" - set of use cases. Is this something we want to support if we cannot do it correctly?

For example, it is impossible to maintain a PastParent and correctly remove entities from Children collections if the user of this API is simply adding/altering the Parent component instead of calling commands.push_children(..). The old Parent would be gone and, if the entity was previously childed to another Parent, we would have no idea what list to remove it from. If we had #1655 it might be possible to support.

I do recognize that it would be a breaking change in that the use case that works correctly is taking two entities that haven't been part of parent/child relationships and just doing commands.insert(child, Parent(parent)).

Copy link
Member

Choose a reason for hiding this comment

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

Yup theres no question that the whole system needs an overhaul. Currently it is possible (and easy) to mutably change Parent (either via commands or &mut Parent queries). Using that to change parents has probably been done (and regardless its possible and easy to do). Breaking that behavior silently feels about as problematic as the bug being fixed in this pr. If we can do it in a way that completely prevents manual Parent mutations (both via queries and direct world access), thats worth considering.

I'm also starting to consider moving forward on merging my hierarchy branch, or if that ends up not being ready / too disruptive for 0.5, maybe just leaving this behavior as-is for 0.5.

Let me know if you have any other suggestions.

Copy link
Member

Choose a reason for hiding this comment

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

My preference would be to leave this behavior as-is and then do a full RFC on how parent-child stuff should work immediately after 0.5 lands. It's very central to UI, and @BoxyUwU's relations are also threatening to disrupt the space and need consideration.

Copy link
Author

Choose a reason for hiding this comment

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

Cool, yeah there's several sets of changes in motion that will contribute to a better solution for this problem space. I don't have any good ideas about how to fix it in a way that works for everyone with what we have now, I think we need either Boxy's changes or something like #1655. I'm good closing this then, I'll keep running a fork until we get a proper solution.

// Add to the parent's `Children` (either the real component, or
// `children_additions`).
if let Ok(mut new_parent_children) = children_query.get_mut(parent.0) {
// This is the parent
debug_assert!(
!(*new_parent_children).0.contains(&entity),
"children already added"
);
(*new_parent_children).0.push(entity);
} else {
// The parent doesn't have a children entity, lets add it
children_additions
.entry(parent.0)
.or_insert_with(Default::default)
.push(entity);
}
}

// Flush the `children_additions` to the command buffer. It is stored separate to
// collect multiple new children that point to the same parent into the same
// SmallVec, and to prevent redundant add+remove operations.
children_additions.iter().for_each(|(k, v)| {
commands.insert(*k, Children::with(v));
});
}
#[cfg(test)]
mod test {
Expand Down Expand Up @@ -124,8 +95,10 @@ mod test {
);

// Parent `e1` to `e2`.
(*world.get_mut::<Parent>(children[0]).unwrap()).0 = children[1];
let mut commands = Commands::new(&mut command_queue, &world);
commands.push_children(children[1], &[children[0]]);

command_queue.apply(&mut world);
schedule.run(&mut world);

assert_eq!(
Expand Down