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

[Merged by Bors] - Fix RemoveChildren command #6192

Closed

Conversation

tim-blackbird
Copy link
Contributor

Objective

RemoveChildren could remove the Parent component from children belonging to a different parent, which breaks the hierarchy.

This change looks a little funny because I'm reusing the events to avoid needing to clone the parent's Children.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Hierarchy Parent-child entity hierarchies labels Oct 7, 2022
@alice-i-cecile alice-i-cecile added this to the Bevy 0.9 milestone Oct 7, 2022
Copy link
Contributor

@inodentry inodentry left a comment

Choose a reason for hiding this comment

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

LGTM. The logic makes sense.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Yay, more correctness fixes! Relations plz though.

bors r+

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Oct 10, 2022
bors bot pushed a commit that referenced this pull request Oct 10, 2022
# Objective

`RemoveChildren` could remove the `Parent` component from children belonging to a different parent, which breaks the hierarchy.

This change looks a little funny because I'm reusing the events to avoid needing to clone the parent's `Children`.

Co-authored-by: devil-ira <justthecooldude@gmail.com>
@bors bors bot changed the title Fix RemoveChildren command [Merged by Bors] - Fix RemoveChildren command Oct 10, 2022
@bors bors bot closed this Oct 10, 2022
@tim-blackbird tim-blackbird deleted the remove-children-brokey branch October 13, 2022 09:22
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 19, 2022
# Objective

`RemoveChildren` could remove the `Parent` component from children belonging to a different parent, which breaks the hierarchy.

This change looks a little funny because I'm reusing the events to avoid needing to clone the parent's `Children`.

Co-authored-by: devil-ira <justthecooldude@gmail.com>
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

`RemoveChildren` could remove the `Parent` component from children belonging to a different parent, which breaks the hierarchy.

This change looks a little funny because I'm reusing the events to avoid needing to clone the parent's `Children`.

Co-authored-by: devil-ira <justthecooldude@gmail.com>
Pietrek14 pushed a commit to Pietrek14/bevy that referenced this pull request Dec 17, 2022
# Objective

`RemoveChildren` could remove the `Parent` component from children belonging to a different parent, which breaks the hierarchy.

This change looks a little funny because I'm reusing the events to avoid needing to clone the parent's `Children`.

Co-authored-by: devil-ira <justthecooldude@gmail.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

`RemoveChildren` could remove the `Parent` component from children belonging to a different parent, which breaks the hierarchy.

This change looks a little funny because I'm reusing the events to avoid needing to clone the parent's `Children`.

Co-authored-by: devil-ira <justthecooldude@gmail.com>
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-Bug An unexpected or incorrect behavior 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.

4 participants