Ensure `GlobalTransform` consistency when `Parent` is spawned without `Children` by yilinwei · Pull Request #3340 · bevyengine/bevy

yilinwei · 2021-12-15T16:23:00Z

Objective

Solution

parent_update and transform_propagate run in the same stage but parent_update can spawn Children. This means that the system can enter an inconsistent state where the GlobalTransform has not been updated on Children when spawning an Entity where the Parent does not have an existing Children component.

Introduce a marker trait, DirtyParent, so that the system will revert to the correct state the next time the systems run.

crates/bevy_transform/src/hierarchy/hierarchy_maintenance_system.rs

`parent_update` and `transform_propagate` run in the same stage but `parent_update` can spawn `Children`. This means that the `system` can enter an inconsistent state where the `GlobalTransform` has not been updated on `Children` when spawning an `Entity` where the `Parent` does not have an existing `Children` component. Introduce a marker trait, `DirtyParent`, so that the system will revert to the correct state the next time the systems run.

alice-i-cecile · 2021-12-15T19:02:55Z

Thanks for the fix :) I'll dig into the details now. Could you swap the title of this PR over to something more descriptive, like "Ensure GlobalTransform consistency when spawning children"?

crates/bevy_transform/src/hierarchy/hierarchy_maintenance_system.rs

alice-i-cecile · 2021-12-15T23:17:32Z

crates/bevy_transform/src/transform_propagate_system.rs

    for (entity, children, transform, mut global_transform) in root_query.iter_mut() {
        let mut changed = false;
-        if changed_transform_query.get(entity).is_ok() {
+        if changed_transform_query.get(entity).is_ok() || dirty_parent_query.get(entity).is_ok() {


Like you suggested on Discord, I think this is going to be cleaner with an Option<&DirtyParent> query parameter, rather than creating a new query entirely.

I've switched this to use an Or and the same changed_transform_query, wdyt?

alice-i-cecile · 2021-12-15T23:18:29Z

Looks good! Thanks for the fix; the code quality, tests and debugging are excellent for a new contributor. I left a couple of nits; once those are cleaned up this will have my approval.

alice-i-cecile

Looks good to me :) The Or filter is nicer, I'm glad you suggested that idea.

BorisBoutillier · 2022-01-01T19:22:54Z

Not a usual reviewer, but I have reviewed the changes and approves them.

IceSentry · 2022-01-01T20:18:26Z

crates/bevy_transform/src/hierarchy/hierarchy_maintenance_system.rs

+        // Mark the entity with a `DirtyParent` component so that systems which run
+        // in the same stage have a way to query for a missed update.


I feel like this comment is more appropriate on the struct declaration. Right now the only explanation as to what DirtyParent means is here but the struct itself has no documentation.

I'm not certain the struct declaration is a great place for this particular bit of documentation - DirtyParent is a quirk of both the parent_update_system and how the stages are currently run; the comment is there because it's not obvious why you'd add it in that particular code branch.

As to the wider point, I don't know what the documentation on the declaration should achieve. The problem is quite a niche one - you need to spawn your components in a particular way and even then, you'll only touch DirtyParent in the specific case that you're running a system in the same stage. If you're looking at DirtyParent directly, it's likely that you'll want to read through the code itself.

Arguably it should be part of the documentation on the whole hierarchy system, since that would be where the "entrypoint" to this behaviour would be.

Davier · 2022-01-01T20:35:38Z

Could it be done by using set_changed() on the dirty parent's transform instead of inserting a new component?

yilinwei · 2022-01-02T11:02:27Z

@Davier I don't think so, but I'm not too familiar with the semantics of set_changed.

The parent_update_system runs before the transform_propagate_system (which is where we add the new Children and mark it with DirtyParent).
The Transform is already changed for that particular frame.
The Children component is not visible on that frame.

Davier · 2022-01-02T16:09:37Z

@Davier I don't think so, but I'm not too familiar with the semantics of set_changed.

You're right, I had misunderstood the issue

mirkoRainer

LGTM. Test passes as expected and change is fairly straightforward. 👍

DJMcNab

I like the test, but I agree that the actual mechanism of using a temporary component for this is unneccessary.

DJMcNab · 2022-01-15T23:46:28Z

crates/bevy_transform/src/transform_propagate_system.rs

    >,
    mut transform_query: Query<(&Transform, &mut GlobalTransform), With<Parent>>,
-    changed_transform_query: Query<Entity, Changed<Transform>>,
+    changed_transform_query: Query<Entity, Or<(Changed<Transform>, With<DirtyParent>)>>,


I think Changed<Children> would be a better filter here; in that case we could avoid the entire DirtyParent bookkeeping.

I'm not actually sure that (without doing that) any changes to Children which didn't also change the child's parent would result in transforms being updated; unless there is a system similar to update_parents but the other way around.

I did have a discussion about this when implementing it. The options were:

Add a marker component

Remove the automatic insertion of the Children resource, so that it fails in a way which is obvious to the user (how Transform and GlobalTransform behave)

Recalculate when the Children change.

My preference was 1. or 2. The reasoning was that Children feels like a general Component since it's first-class in the API and using it when there are Parent/Child relationships is encouraged.

IMO, it shouldn't really trigger a Transform update; not merely for optimization purposes, but I don't like the idea of the user receiving the GlobalTransform update events each time a child is added or removed. I think that would be confusing.

I suppose what actually needs to happen is that Changed<Children> needs to be reacted to by re-calculating the children, but 'self' doesn't need to be recalculated (unless it already needed to be). The tree has changed, so the transformations of the subtree need updating.

I don't quite follow; could you please elaborate?

I think in general Changed<Children> is too coarse a filter; under normal operation if a child is changed, there shouldn't be a reason to recalculate the GlobalTransform of it's siblings. It's only in this specific case, where we "lose" the insertion since we're in the same stage that we need to react to it.

Well my point is: if you add a chilld, how does the child's transform get updated? Neither the parent or child's transforms have changed, so that filter won't activate; so changed will be false, so it won't activate.

If you don't want to update 'self''s transform then, the way to implement this would be to add a changed_children query.
If this solution is implemented, it cleanly sidesteps the need for the messy DirtyParent component.

OK I don't understand the solution at all; at least I can't think of a way of doing it without recalculating the siblings which haven't changed.

This is how I think that the algorithm currently works.

Go to the root nodes

Walk down each node checking if the Transform has changed

If the Transform has changed, then update the GlobalTransform and the GlobalTransform of the Children

In the problematic case, the Children has been created in this stage so it doesn't see the child when walking down the tree. In the frame afterwards, neither the parent nor child have an updated Transform but has a Changed<Children> which would evaluate to true.

The Changed<Children> query:

Can't distinguish which child has changed, which means we'd have to update all the siblings since we need to mark the parent. Note, this is also true of DirtyParent, but since it only lasts for a single frame, that doesn't matter since any changes during that single frame needs to be calculated anyway.

Will also evaluate to true when mutating Children later as well.

I cannot see how a single Changed<Children> would be able to sidestep both the issues?

yilinwei · 2022-01-18T16:51:08Z

I think there's another method without the marker component. We could do a dummy update of the Transform using commands.insert which would apply on the same frame as the children insertion; but that would overwrite any user mutations for that frame.

alice-i-cecile · 2022-04-26T19:14:43Z

I'm going to block this on seeing if we can get @DJMcNab's suggestion to do this without a temporary component to work.

Reviewers: if you can submit a PR to this branch to fix that, it would be greatly appreciated.

alice-i-cecile · 2022-04-26T20:47:27Z

Closing in favor of #4608. This was really valuable foundation, but I think that the approach without the temporary component is substantially more useful.

…ren (#4608) Supercedes #3340, and absorbs the test from there. # Objective - Fixes #3329 ## Solution - If the `Children` component has changed, we currently do not have a way to know how it has changed. - Therefore, we must update the hierarchy downwards from that point to be correct. Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>

…ren (bevyengine#4608) Supercedes bevyengine#3340, and absorbs the test from there. # Objective - Fixes bevyengine#3329 ## Solution - If the `Children` component has changed, we currently do not have a way to know how it has changed. - Therefore, we must update the hierarchy downwards from that point to be correct. Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>

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

yilinwei commented Dec 15, 2021

View reviewed changes

crates/bevy_transform/src/hierarchy/hierarchy_maintenance_system.rs Outdated Show resolved Hide resolved

yilinwei force-pushed the propogate-log branch from 1a4fcdc to 6732003 Compare December 15, 2021 16:25

fix clippy warnings

5adb285

alice-i-cecile self-requested a review December 15, 2021 19:01

alice-i-cecile added A-Transform Translations, rotations and scales C-Bug An unexpected or incorrect behavior S-Needs-Review and removed S-Needs-Triage This issue needs to be labelled labels Dec 15, 2021

yilinwei changed the title ~~Closes #3329~~ Ensure GlobalTransform consistency when Parent is spawned without Children Dec 15, 2021

alice-i-cecile reviewed Dec 15, 2021

View reviewed changes

crates/bevy_transform/src/hierarchy/hierarchy_maintenance_system.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Dec 15, 2021

View reviewed changes

crates/bevy_transform/src/hierarchy/hierarchy_maintenance_system.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Dec 15, 2021

View reviewed changes

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

address comments.

46cead4

alice-i-cecile approved these changes Dec 17, 2021

View reviewed changes

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 Jan 1, 2022

IceSentry reviewed Jan 1, 2022

View reviewed changes

mirkoRainer approved these changes Jan 4, 2022

View reviewed changes

alice-i-cecile added this to the Bevy 0.6.1 milestone Jan 15, 2022

DJMcNab reviewed Jan 15, 2022

View reviewed changes

alice-i-cecile removed this from the Bevy 0.6.1 milestone Feb 10, 2022

james7132 mentioned this pull request Mar 8, 2022

Hierarchy optimized systems and storage #4141

Open

nicopap mentioned this pull request Apr 19, 2022

Scenes do not hot reload #3759

Closed

alice-i-cecile removed 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 Apr 26, 2022

DJMcNab mentioned this pull request Apr 26, 2022

[Merged by Bors] - Make Transform propagation correct in the presence of updated children #4608

Closed

alice-i-cecile closed this Apr 26, 2022

yilinwei mentioned this pull request May 3, 2022

Improve incrementality of transform propagation updates for siblings #4647

Closed

		// Mark the entity with a `DirtyParent` component so that systems which run
		// in the same stage have a way to query for a missed update.

Uh oh!

Conversation

yilinwei commented Dec 15, 2021

Objective

Solution

Uh oh!

Uh oh!

alice-i-cecile commented Dec 15, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile commented Dec 15, 2021

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

BorisBoutillier commented Jan 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yilinwei Jan 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Davier commented Jan 1, 2022

Uh oh!

yilinwei commented Jan 2, 2022

Uh oh!

Davier commented Jan 2, 2022

Uh oh!

mirkoRainer left a comment

Choose a reason for hiding this comment

Uh oh!

DJMcNab left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yilinwei Jan 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yilinwei commented Jan 18, 2022

Uh oh!

alice-i-cecile commented Apr 26, 2022

Uh oh!

alice-i-cecile commented Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

yilinwei Jan 2, 2022 •

edited

Loading

yilinwei Jan 18, 2022 •

edited

Loading

alice-i-cecile commented Apr 26, 2022 •

edited

Loading