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

Fix bugs/inconsistencies with adding child entities #4706

Closed
wants to merge 3 commits into from
Closed

Fix bugs/inconsistencies with adding child entities #4706

wants to merge 3 commits into from

Conversation

SUPERCILEX
Copy link
Contributor

@SUPERCILEX SUPERCILEX commented May 9, 2022

Objective

Currently, when new children are added via commands, the old parent's children component is not updated.

Solution

This PR fixes that by partially applying changes from #4197. The ability to update relationships by adding or removing a Parent component is not changed, but bugs with updating relations from commands are fixed.

Changelog

Fix inconsistencies created when adding children via commands.

Co-authored-by: James Liu contact@jamessliu.com

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
@SUPERCILEX
Copy link
Contributor Author

TODO(me): once this goes through add remove_child API.

@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Hierarchy Parent-child entity hierarchies labels May 10, 2022
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

Generally LGTM. This should ease merging #4197. One thing, since a sizable chunk of the code is based on that PR, mind crediting me here?

@tim-blackbird
Copy link
Contributor

Does Bors have a command to add someone as co-author to the rollup commit? That'd be quite useful here.

@SUPERCILEX
Copy link
Contributor Author

SUPERCILEX commented May 11, 2022

Since the rollup commit takes from the PR description, I made @james7132 a co-author there which should work.

@james7132
Copy link
Member

Since the rollup commit takes from the PR description, I made @james7132 a co-author there which should work.

Just a small note, that email is incorrect. Use contact@jamessliu.com instead.

# Conflicts:
#	crates/bevy_hierarchy/src/child_builder.rs
@SUPERCILEX
Copy link
Contributor Author

@mockersf Mind reviewing this? (You show up in a decent chunk of the blame.)

@james7132 Or if #4197 is for-sure going to get merged, can I send you a patch with a few improvements from this PR?

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:

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.

@james7132
Copy link
Member

james7132 commented Jun 22, 2022

@james7132 Or if #4197 is for-sure going to get merged, can I send you a patch with a few improvements from this PR?

The RFC has been merged, so it's just the implementation PR that needs review and approval.

@SUPERCILEX
Copy link
Contributor Author

@james7132 Or if #4197 is for-sure going to get merged, can I send you a patch with a few improvements from this PR?

The RFC has been merged, so it's just the implementation PR that needs review and approval.

K, I'll send a patch when I can and close this PR.

@SUPERCILEX
Copy link
Contributor Author

Ok, done: #4197 (comment)

@SUPERCILEX SUPERCILEX closed this Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Hierarchy Parent-child entity hierarchies C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants