[Merged by Bors] - Hierarchy commandization by james7132 · Pull Request #4197 · bevyengine/bevy

james7132 · 2022-03-13T03:38:07Z

Objective

Implement absolute minimum viable product for the changes proposed in bevyengine/rfcs#53.

Solution

Remove public mutative access to Parent (Children is already publicly read-only). This includes public construction methods like Copy, Clone, and Default.
Remove PreviousParent
Remove parent_update_system
Update all hierarchy related commands to immediately update both Parent and Children references.

Remaining TODOs

Update documentation for both Parent and Children. Discourage using EntityCommands::remove
Add HierarchyEvent to notify listeners of hierarchy updates. This is meant to replace listening on PreviousParent

Followup

These changes should be best moved to the hooks mentioned in Entity-entity relations 🌈 #3742.
Backing storage for both might be best moved to indexes mentioned in the same relations.

alice-i-cecile · 2022-03-13T04:13:34Z

We should probably merge #4168 before merging this, but I can do more rebasing if needed...

james7132 · 2022-03-13T05:45:39Z

We should probably merge #4168 before merging this, but I can do more rebasing if needed...

The intention is that this won't be merged until we have bevyengine/rfcs#53 is merged. Still quite some hefty few TODOs here too.

james7132 · 2022-05-30T22:16:36Z

The associated RFC has been merged. I'll update the PR to match the design there.

SkiFire13 · 2022-06-23T08:16:55Z

I'm suprised this compiles. This looks unsound. Doesn't the inner remove invalidate the get_mut on the outside?

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>();
            }
        }
    }
}

This is normal behaviour with NLL: children's last use is before the remove so the borrow is released.

cart

Generally looks good to me!

cart · 2022-07-08T00:46:07Z

crates/bevy_hierarchy/src/child_builder.rs

+        }
+        let mut parent = world.entity_mut(self.parent);
+        if let Some(mut children) = parent.get_mut::<Children>() {
+            if !children.contains(&self.child) {


Is this check necessary? Given that this is now transactional, won't children only contain child if child already has the Parent component pointing to self.parent? In that case, the update_parent call would have returned Some(self.parent) and AddChild would early return on line 101.

Not doing this check could result in duplicate children if the hierarchy is altered directly via World. It's highly unlikely given that there's no easy way to make those edits, but it is doable.

Reasonable. O(n) behavior (especially in the context of something that will likely happen regularly at runtime) is nasty enough that I think its worth considering omitting this check. But thats a controversial enough conversation that its probably worth having separately and biasing toward correctness here.

cart · 2022-07-10T20:28:46Z

bors r+

## Objective Implement absolute minimum viable product for the changes proposed in bevyengine/rfcs#53. ## Solution - Remove public mutative access to `Parent` (Children is already publicly read-only). This includes public construction methods like `Copy`, `Clone`, and `Default`. - Remove `PreviousParent` - Remove `parent_update_system` - Update all hierarchy related commands to immediately update both `Parent` and `Children` references. ## Remaining TODOs - [ ] Update documentation for both `Parent` and `Children`. Discourage using `EntityCommands::remove` - [x] Add `HierarchyEvent` to notify listeners of hierarchy updates. This is meant to replace listening on `PreviousParent` ## Followup - These changes should be best moved to the hooks mentioned in #3742. - Backing storage for both might be best moved to indexes mentioned in the same relations.

bors · 2022-07-10T20:49:09Z

Pull request successfully merged into main.

Build succeeded:

#4197 intended to remove all `pub` constructors of `Children` and `Parent` and it seems like this one was missed. Co-authored-by: devil-ira <justthecooldude@gmail.com>

## Objective Implement absolute minimum viable product for the changes proposed in bevyengine/rfcs#53. ## Solution - Remove public mutative access to `Parent` (Children is already publicly read-only). This includes public construction methods like `Copy`, `Clone`, and `Default`. - Remove `PreviousParent` - Remove `parent_update_system` - Update all hierarchy related commands to immediately update both `Parent` and `Children` references. ## Remaining TODOs - [ ] Update documentation for both `Parent` and `Children`. Discourage using `EntityCommands::remove` - [x] Add `HierarchyEvent` to notify listeners of hierarchy updates. This is meant to replace listening on `PreviousParent` ## Followup - These changes should be best moved to the hooks mentioned in bevyengine#3742. - Backing storage for both might be best moved to indexes mentioned in the same relations.

bevyengine#4197 intended to remove all `pub` constructors of `Children` and `Parent` and it seems like this one was missed. Co-authored-by: devil-ira <justthecooldude@gmail.com>

## Objective Implement absolute minimum viable product for the changes proposed in bevyengine/rfcs#53. ## Solution - Remove public mutative access to `Parent` (Children is already publicly read-only). This includes public construction methods like `Copy`, `Clone`, and `Default`. - Remove `PreviousParent` - Remove `parent_update_system` - Update all hierarchy related commands to immediately update both `Parent` and `Children` references. ## Remaining TODOs - [ ] Update documentation for both `Parent` and `Children`. Discourage using `EntityCommands::remove` - [x] Add `HierarchyEvent` to notify listeners of hierarchy updates. This is meant to replace listening on `PreviousParent` ## Followup - These changes should be best moved to the hooks mentioned in bevyengine#3742. - Backing storage for both might be best moved to indexes mentioned in the same relations.

bevyengine#4197 intended to remove all `pub` constructors of `Children` and `Parent` and it seems like this one was missed. Co-authored-by: devil-ira <justthecooldude@gmail.com>

## Objective Implement absolute minimum viable product for the changes proposed in bevyengine/rfcs#53. ## Solution - Remove public mutative access to `Parent` (Children is already publicly read-only). This includes public construction methods like `Copy`, `Clone`, and `Default`. - Remove `PreviousParent` - Remove `parent_update_system` - Update all hierarchy related commands to immediately update both `Parent` and `Children` references. ## Remaining TODOs - [ ] Update documentation for both `Parent` and `Children`. Discourage using `EntityCommands::remove` - [x] Add `HierarchyEvent` to notify listeners of hierarchy updates. This is meant to replace listening on `PreviousParent` ## Followup - These changes should be best moved to the hooks mentioned in bevyengine#3742. - Backing storage for both might be best moved to indexes mentioned in the same relations.

bevyengine#4197 intended to remove all `pub` constructors of `Children` and `Parent` and it seems like this one was missed. Co-authored-by: devil-ira <justthecooldude@gmail.com>

# Objective - For many UI use cases (e.g. tree views, lists), it is important to be able to imperatively sort child nodes. - This also enables us to eventually support something like the [`order`](https://developer.mozilla.org/en-US/docs/Web/CSS/order) CSS property, that declaratively re-orders flex box items by a numeric value, similar to z-index, but in space. ## Solution We removed the ability to directly construct `Children` from `&[Entity]` some time ago (#4197 #5532) to enforce consistent hierarchies ([RFC 53](https://github.com/bevyengine/rfcs/blob/main/rfcs/53-consistent-hierarchy.md)). If I understand it correctly, it's currently possible to re-order children by using `Children::swap()` or `commands.entity(id).replace_children(...)`, however these are either too cumbersome, needlessly inefficient, and/or don't take effect immediately. This PR exposes the in-place sorting methods from the `slice` primitive in `Children`, enabling imperatively sorting children in place via `&mut Children`, while still preserving consistent hierarchies. --- ## Changelog ### Added - The sorting methods from the `slice` primitive are now exposed by the `Children` component, allowing imperatively sorting children in place (Useful for UI scenarios such as lists)

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 13, 2022

james7132 added S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged A-Transform Translations, rotations and scales and removed S-Needs-Triage This issue needs to be labelled labels Mar 13, 2022

alice-i-cecile added the C-Usability A targeted quality-of-life change that makes Bevy easier to use label Mar 13, 2022

james7132 added 15 commits March 15, 2022 01:45

Make Parent immutable, remove PreviousParent, parent_update_system

613756a

Make hierarchy commands "atomic"

838de43

Fix tests

9b3a8f0

Formatting and clippy

d471ba3

More complete and correct updates

9a35a57

Hierarchy events

2e9a9a4

Document new events

42d0ad3

Remove return type

70a96fc

Panic if invariant broken

4d7dea3

fix up CI

88958a1

fix formatting

51368c1

Fix UI tests

01b0d48

Remove copying/constructing traits from Parent/Children

c07b2ee

Scenes are Worlds too

e58b087

fix rebase errors

82febea

james7132 force-pushed the hierarchy-commandization branch from e7ef4a5 to 82febea Compare March 15, 2022 09:22

This was referenced May 9, 2022

Hierarchy Child Global Transforms are Not Updated When Spawned as a Child #1354

Closed

Fix bugs/inconsistencies with adding child entities #4706

Closed

james7132 added 2 commits May 24, 2022 02:26

Merge branch 'main' into hierarchy-commandization

f05a2ca

Update to reflect changes to the RFC

b914610

james7132 removed the S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged label May 30, 2022

Merge branch 'main' into hierarchy-commandization

d8b3d14

Some code cleanup

cc5244a

james7132 requested a review from BoxyUwU July 1, 2022 23:25

cart reviewed Jul 8, 2022

View reviewed changes

Add back malformed hierarchy test

456c0ea

bors bot changed the title ~~Hierarchy commandization~~ [Merged by Bors] - Hierarchy commandization Jul 10, 2022

bors bot closed this Jul 10, 2022

tim-blackbird mentioned this pull request Jul 11, 2022

The test correct_transforms_when_no_children spuriously fails. #5285

Closed

alice-i-cecile mentioned this pull request Jul 14, 2022

Enforce uniqueness in Children #1079

Closed

hymm mentioned this pull request Jul 15, 2022

[Merged by Bors] - 0.8 Migration guide bevyengine/bevy-website#389

Closed

alice-i-cecile mentioned this pull request Jul 18, 2022

Improve incrementality of transform propagation updates for siblings #4647

Closed

nicopap mentioned this pull request Jul 28, 2022

Add Transform-preserving hierarchy update method #5475

Closed

tim-blackbird mentioned this pull request Aug 1, 2022

[Merged by Bors] - Make Children constructor pub(crate). #5532

Closed

bors bot pushed a commit that referenced this pull request Aug 5, 2022

Make Children constructor pub(crate). (#5532)

c37939d

#4197 intended to remove all `pub` constructors of `Children` and `Parent` and it seems like this one was missed. Co-authored-by: devil-ira <justthecooldude@gmail.com>

nicopap mentioned this pull request Sep 1, 2022

Add a set_parent method to ChildBuilder #5845

Closed

Shatur mentioned this pull request Sep 18, 2022

Make hierarchy semi-mutable #6012

Closed

james7132 mentioned this pull request Nov 12, 2022

Transform not propagated for child due to delay of inserting Children #1807

Closed

james7132 deleted the hierarchy-commandization branch December 12, 2022 06:45

coreh mentioned this pull request May 1, 2023

Expose sorting methods in Children #8522

Merged

alice-i-cecile mentioned this pull request Oct 1, 2024

Removing Parent / Children from the world and then reinserting them can break hierarchies #15575

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Merged by Bors] - Hierarchy commandization#4197

[Merged by Bors] - Hierarchy commandization#4197
james7132 wants to merge 24 commits intobevyengine:mainfrom
james7132:hierarchy-commandization

james7132 commented Mar 13, 2022 •

edited

Loading

Uh oh!

alice-i-cecile commented Mar 13, 2022

Uh oh!

james7132 commented Mar 13, 2022

Uh oh!

james7132 commented May 30, 2022

Uh oh!

SkiFire13 commented Jun 23, 2022

Uh oh!

cart left a comment

Uh oh!

cart Jul 8, 2022

Uh oh!

james7132 Jul 10, 2022

Uh oh!

cart Jul 10, 2022

Uh oh!

cart commented Jul 10, 2022

Uh oh!

bors bot commented Jul 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

james7132 commented Mar 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Remaining TODOs

Followup

Uh oh!

alice-i-cecile commented Mar 13, 2022

Uh oh!

james7132 commented Mar 13, 2022

Uh oh!

james7132 commented May 30, 2022

Uh oh!

SkiFire13 commented Jun 23, 2022

Uh oh!

cart left a comment

Choose a reason for hiding this comment

Uh oh!

cart Jul 8, 2022

Choose a reason for hiding this comment

Uh oh!

james7132 Jul 10, 2022

Choose a reason for hiding this comment

Uh oh!

cart Jul 10, 2022

Choose a reason for hiding this comment

Uh oh!

cart commented Jul 10, 2022

Uh oh!

bors bot commented Jul 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

james7132 commented Mar 13, 2022 •

edited

Loading