[Merged by Bors] - fix parenting of scenes by mockersf · Pull Request #2410 · bevyengine/bevy

mockersf · 2021-06-28T22:58:11Z

Objective

Scene parenting was not done completely, leaving the hierarchy maintenance to the standard system. As scene spawning happens in stage PreUpdate and hierarchy maintenance in stage PostUpdate, this left the scene in an invalid state parent wise for part of a frame

Solution

Also add/update the Children component when spawning the scene.

I kept the Children component as a SmallVec, it could be moved to an HashSet to guarantee uniqueness

NathanSWard · 2021-06-28T23:06:30Z

Could we add a test for this?
(You can probably just copy the code from the related issue and use that. Or make something similar)

mockersf · 2021-06-29T01:22:32Z

Not easily possible to add a test, at least as a unit test: it would need to access assets, which can't be added without the asset server, which needs a thread pool... this is more an integration test, and I would rather not add those at the moment

NathanSWard · 2021-06-29T03:56:43Z

this is more an integration test, and I would rather not add those at the moment

I would argue and say then, we should start to add integration testing haha 😄
If this fix was a response to a bug, we should prevent this bug from happening again if/when this logic gets refactored/changed in the future.

NathanSWard · 2021-06-29T04:05:05Z

I'm happy not to block this PR on testing, however, we should seriously consider integration testing for scenarios like this.

NathanSWard · 2021-06-29T17:08:25Z

crates/bevy_scene/src/scene_spawner.rs

+                            let children = &**children;
+                            let mut children = children.to_vec();
+                            children.push(entity);
+                            parent_entity.insert(Children::with(&children));


This seems like quite a bit of vector coppying.
Is it possible to just push entity to the parent's children component?

the SmallVec inside Children is not mutably accessible because it caused a lot of sync issues:

bevy/crates/bevy_transform/src/components/children.rs

Line 11 in 46b822e

pub struct Children(pub(crate) SmallVec<[Entity; 8]>);

I think it would be nice to add a comment about this?

inodentry · 2021-09-24T23:36:57Z

crates/bevy_scene/src/scene_spawner.rs

+                            let children = &**children;
+                            let mut children = children.to_vec();
+                            children.push(entity);
+                            parent_entity.insert(Children::with(&children));


I think it would be nice to add a comment about this?

cart · 2021-12-22T21:18:43Z

crates/bevy_scene/src/scene_spawner.rs

                        }
                    }
+                    // Add the scene root to the `Children` component on the parent
+                    if let Some(mut parent_entity) = world.get_entity_mut(parent) {


This feels like quite the hack, both because we're duplicating parenting logic in scenes and because of the children copy. I want to fix this bug, but maybe we need a slightly more holistic solution. Hard-coding logic here seems like a reasonable short term fix (with a TODO to improve this later), but reallocating Children for each child in a scene is hard to accept. One way to implement this without directly exposing internals is to add a new AddChild command to bevy_transform, which takes advantage of crate-only access to the Children internals. Then you could just construct the Command for each entity and immediately apply it (no need for command buffers / allocations).

(AddChild would transactionally set the proper values for the Parent and Children components)

added AddChild command. It's the only command with pub fields to be able to create and execute it directly

crates/bevy_transform/src/hierarchy/hierarchy_maintenance_system.rs

cart · 2021-12-23T22:27:23Z

bors r+

# Objective Fix #2406 Scene parenting was not done completely, leaving the hierarchy maintenance to the standard system. As scene spawning happens in stage `PreUpdate` and hierarchy maintenance in stage `PostUpdate`, this left the scene in an invalid state parent wise for part of a frame ## Solution Also add/update the `Children` component when spawning the scene. I kept the `Children` component as a `SmallVec`, it could be moved to an `HashSet` to guarantee uniqueness Co-authored-by: François <8672791+mockersf@users.noreply.github.com>

bors · 2021-12-23T22:28:43Z

Build failed:

build-wasm (nightly, ubuntu-latest)

cart · 2021-12-23T22:49:44Z

bors r+

# Objective Fix #2406 Scene parenting was not done completely, leaving the hierarchy maintenance to the standard system. As scene spawning happens in stage `PreUpdate` and hierarchy maintenance in stage `PostUpdate`, this left the scene in an invalid state parent wise for part of a frame ## Solution Also add/update the `Children` component when spawning the scene. I kept the `Children` component as a `SmallVec`, it could be moved to an `HashSet` to guarantee uniqueness Co-authored-by: François <8672791+mockersf@users.noreply.github.com>

bors · 2021-12-23T23:13:59Z

Build failed (retrying...):

build-wasm (nightly, ubuntu-latest)

# Objective Fix #2406 Scene parenting was not done completely, leaving the hierarchy maintenance to the standard system. As scene spawning happens in stage `PreUpdate` and hierarchy maintenance in stage `PostUpdate`, this left the scene in an invalid state parent wise for part of a frame ## Solution Also add/update the `Children` component when spawning the scene. I kept the `Children` component as a `SmallVec`, it could be moved to an `HashSet` to guarantee uniqueness Co-authored-by: François <8672791+mockersf@users.noreply.github.com>

bors · 2021-12-23T23:30:39Z

Build failed:

build-wasm (nightly, ubuntu-latest)

cart · 2021-12-23T23:33:11Z

bors r+

# Objective Fix #2406 Scene parenting was not done completely, leaving the hierarchy maintenance to the standard system. As scene spawning happens in stage `PreUpdate` and hierarchy maintenance in stage `PostUpdate`, this left the scene in an invalid state parent wise for part of a frame ## Solution Also add/update the `Children` component when spawning the scene. I kept the `Children` component as a `SmallVec`, it could be moved to an `HashSet` to guarantee uniqueness Co-authored-by: François <8672791+mockersf@users.noreply.github.com>

bors · 2021-12-23T23:36:08Z

Build failed (retrying...):

build-wasm (nightly, ubuntu-latest)

# Objective Fix #2406 Scene parenting was not done completely, leaving the hierarchy maintenance to the standard system. As scene spawning happens in stage `PreUpdate` and hierarchy maintenance in stage `PostUpdate`, this left the scene in an invalid state parent wise for part of a frame ## Solution Also add/update the `Children` component when spawning the scene. I kept the `Children` component as a `SmallVec`, it could be moved to an `HashSet` to guarantee uniqueness Co-authored-by: François <8672791+mockersf@users.noreply.github.com>

bors · 2021-12-23T23:41:21Z

Build failed:

build-wasm (nightly, ubuntu-latest)

cart · 2021-12-24T06:57:16Z

bors r+

# Objective Fix #2406 Scene parenting was not done completely, leaving the hierarchy maintenance to the standard system. As scene spawning happens in stage `PreUpdate` and hierarchy maintenance in stage `PostUpdate`, this left the scene in an invalid state parent wise for part of a frame ## Solution Also add/update the `Children` component when spawning the scene. I kept the `Children` component as a `SmallVec`, it could be moved to an `HashSet` to guarantee uniqueness Co-authored-by: François <8672791+mockersf@users.noreply.github.com>

bors · 2021-12-24T07:10:06Z

Pull request successfully merged into main.

Build succeeded:

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jun 28, 2021

NathanSWard added C-Bug An unexpected or incorrect behavior core and removed S-Needs-Triage This issue needs to be labelled labels Jun 28, 2021

NathanSWard reviewed Jun 29, 2021

View reviewed changes

cart added this to the Bevy 0.6 milestone Jul 1, 2021

mockersf mentioned this pull request Jul 3, 2021

[Merged by Bors] - add a SceneBundle to spawn a scene #2424

Closed

cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021

mockersf removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 24, 2021

alice-i-cecile added the S-Needs-Review label Sep 22, 2021

inodentry approved these changes Sep 24, 2021

View reviewed changes

cart removed the S-Needs-Review label Dec 16, 2021

cart reviewed Dec 22, 2021

View reviewed changes

mockersf and others added 4 commits December 23, 2021 20:43

fix parenting of scenes

289e1dc

only add root entity as children to scene parent

eb66e78

add perf comment

e57781a

uses an AddChild command

6fc5c49

mockersf force-pushed the scene-parenting branch from b7be890 to 6fc5c49 Compare December 23, 2021 21:50

make AddChild command available through usual Commands

487428c

bors bot changed the title ~~fix parenting of scenes~~ [Merged by Bors] - fix parenting of scenes Dec 24, 2021

bors bot closed this Dec 24, 2021

DJMcNab mentioned this pull request Jan 6, 2022

Slightly nicer handling of parent/child failure modes #3199

Closed

Uh oh!

Conversation

mockersf commented Jun 28, 2021

Objective

Solution

Uh oh!

NathanSWard commented Jun 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mockersf commented Jun 29, 2021

Uh oh!

NathanSWard commented Jun 29, 2021

Uh oh!

NathanSWard commented Jun 29, 2021

Uh oh!

NathanSWard Jun 29, 2021

Choose a reason for hiding this comment

Uh oh!

mockersf Jun 29, 2021

Choose a reason for hiding this comment

Uh oh!

inodentry Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

inodentry Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

cart Dec 22, 2021

Choose a reason for hiding this comment

Uh oh!

cart Dec 22, 2021

Choose a reason for hiding this comment

Uh oh!

mockersf Dec 23, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cart commented Dec 23, 2021

Uh oh!

bors bot commented Dec 23, 2021

Uh oh!

cart commented Dec 23, 2021

Uh oh!

bors bot commented Dec 23, 2021

Uh oh!

bors bot commented Dec 23, 2021

Uh oh!

cart commented Dec 23, 2021

Uh oh!

bors bot commented Dec 23, 2021

Uh oh!

bors bot commented Dec 23, 2021

Uh oh!

cart commented Dec 24, 2021

Uh oh!

bors bot commented Dec 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

NathanSWard commented Jun 28, 2021 •

edited

Loading