Fix transform propagation of orphaned entities by nicopap · Pull Request #7264 · bevyengine/bevy

nicopap · 2023-01-18T10:00:31Z

Objective

Fix The GlobalTransform did not get updated when Parent was removed #7263

This has nothing to do with #7024. This is for the case where the
user opted to not keep the same global transform on update.

Solution

Add a RemovedComponent<Parent> to propagate_transforms
Add a RemovedComponent<Parent> and Local<Vec<Entity>> to sync_simple_transforms
Add test to make sure all of this works.

Performance note

This should only incur a cost in cases where a parent is removed.

A minimal overhead (one look up in the removed_components
sparse set) per root entities without children which transform didn't
change. A Vec the size of the largest number of entities removed
with a Parent component in a single frame, and a binary search on
a Vec per root entities.

It could slow up considerably in situations where a lot of entities are
orphaned consistently during every frame, since
sync_simple_transforms is not parallel. But in this situation,
it is likely that the overhead of archetype updates overwhelms everything.

Changelog

Fix the GlobalTransform not getting updated when Parent is removed

Migration Guide

If you called bevy_transform::systems::sync_simple_transforms and bevy_transform::systems::propagate_transforms (which is not re-exported by bevy) you need to account for the additional RemovedComponents<Parent> parameter.

james7132 · 2023-01-18T10:51:11Z

crates/bevy_transform/src/systems.rs

        1,
        |(entity, children, transform, mut global_transform)| {
-            let changed = transform.is_changed();
+            let changed = transform.is_changed() || orphaned.binary_search(&entity).is_ok();


It may actually be faster to do a sparse set contains check here than do the binary search. It'd also avoid the allocation and sort.

How would this look? RemovedComponents only exposes the iter() method. all other fields private. I guess the best way would be to add a method on RemovedComponents to allow sparse set contains checks.

Looks like the entities in World::removed_components are stored in a Vec. It's unlikely that it's possible to outperform sorting + binary search. I expect the Vec to be always relatively small and not spill out of cache.

The alternative is to create a method that exposes a Option<&[Entity]> in RemovedComponents, and iterate through it for each root entities.

Another option is to store orphaned in a Local to avoid allocation at each frame. Of course, since the iterator returned by RemovedComponents::iter is Cloned<slice::Iter<Entity>>, I don't expect it to allocate when it's empty. And storing the Vec in a Local smells memory leak.

This is non-actionable. If performance is a concern, could you point to ways of benchmarking testing for regression?

Another option is to store orphaned in a Local to avoid allocation at each frame. Of course, since the iterator returned by RemovedComponents::iter is Cloned<slice::Iter<Entity>>, I don't expect it to allocate when it's empty. And storing the Vec in a Local smells memory leak.

This would be a nice (but not necessary) optimization. It would just allow the system to reuse a single allocation, it shouldn't cause a memory leak or anything bad.

My worry is that orphaning happens in bursts (think of unloading/loading scenes), so we end up with reserved memory for the worst case while the Vec is only fully used one frame every minutes/hours.

I think a cool thing bevy could provide is some sort of smart Local<Vec<T>> that clears intelligently. I think it would benefit the engine itself already.

I've added a Local<Vec<Entity>> to avoid the allocation, but see the two previous paragraphs.

crates/bevy_transform/src/systems.rs

james7132

LGTM other than this one nit.

crates/bevy_transform/src/systems.rs

github-actions · 2023-03-07T08:58:08Z

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

joseph-gio

Good fix, there's something I'm not sure about yet though.

joseph-gio · 2023-03-31T14:15:43Z

crates/bevy_transform/src/systems.rs

        1,
        |(entity, children, transform, mut global_transform)| {
-            let changed = transform.is_changed();
+            let changed = transform.is_changed() || orphaned.binary_search(&entity).is_ok();


Another option is to store orphaned in a Local to avoid allocation at each frame. Of course, since the iterator returned by RemovedComponents::iter is Cloned<slice::Iter<Entity>>, I don't expect it to allocate when it's empty. And storing the Vec in a Local smells memory leak.

This would be a nice (but not necessary) optimization. It would just allow the system to reuse a single allocation, it shouldn't cause a memory leak or anything bad.

crates/bevy_transform/src/systems.rs

Co-authored-by: vyb <vyb@users.noreply.github.com>

crates/bevy_transform/src/systems.rs

Co-authored-by: JoJoJet <21144246+JoJoJet@users.noreply.github.com>

…tities query (#9518) # Objective `sync_simple_transforms` only checks for removed parents and doesn't filter for `Without<Parent>`, so it overwrites the `GlobalTransform` of non-orphan entities that were orphaned and then reparented since the last update. Introduced by #7264 ## Solution Filter for `Without<Parent>`. Fixes #9517, #9492 ## Changelog `sync_simple_transforms`: * Added a `Without<Parent>` filter to the orphaned entities query.

…tities query (bevyengine#9518) # Objective `sync_simple_transforms` only checks for removed parents and doesn't filter for `Without<Parent>`, so it overwrites the `GlobalTransform` of non-orphan entities that were orphaned and then reparented since the last update. Introduced by bevyengine#7264 ## Solution Filter for `Without<Parent>`. Fixes bevyengine#9517, bevyengine#9492 ## Changelog `sync_simple_transforms`: * Added a `Without<Parent>` filter to the orphaned entities query.

nicopap added C-Bug An unexpected or incorrect behavior A-Transform Translations, rotations and scales A-Hierarchy labels Jan 18, 2023

nicopap force-pushed the fix-orphaned-transform branch from 480ab13 to 0436c5f Compare January 18, 2023 10:21

james7132 requested changes Jan 18, 2023

View reviewed changes

nicopap mentioned this pull request Jan 18, 2023

Add an inverse to Changed and Added filters #7265

Open

nicopap force-pushed the fix-orphaned-transform branch from 0436c5f to e59d44e Compare January 30, 2023 09:23

nicopap requested review from james7132 and rparrett January 30, 2023 09:31

rparrett mentioned this pull request Jan 31, 2023

transform_hierarchy example doesn't seem to do anything #7433

Closed

james7132 approved these changes Feb 3, 2023

View reviewed changes

crates/bevy_transform/src/systems.rs Show resolved Hide resolved

james7132 requested a review from tim-blackbird February 6, 2023 19:03

nicopap force-pushed the fix-orphaned-transform branch 2 times, most recently from efa3c16 to b63cbc8 Compare February 7, 2023 09:55

james7132 added this to the 0.11 milestone Mar 4, 2023

nicopap modified the milestones: 0.11, 0.10.1 Mar 7, 2023

nicopap added the M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Mar 7, 2023

joseph-gio reviewed Mar 31, 2023

View reviewed changes

nicopap and others added 3 commits April 3, 2023 16:20

Fix transform propagation for orphaned hierarchies

41f22df

Co-authored-by: vyb <vyb@users.noreply.github.com>

Fix rebase mishaps

3985aaf

Include jojojet feedback

d1af08a

nicopap force-pushed the fix-orphaned-transform branch from b63cbc8 to d1af08a Compare April 3, 2023 14:23

nicopap requested a review from joseph-gio April 3, 2023 14:25

joseph-gio approved these changes Apr 4, 2023

View reviewed changes

crates/bevy_transform/src/systems.rs Show resolved Hide resolved

crates/bevy_transform/src/systems.rs Outdated Show resolved Hide resolved

Apply suggestions from code review

1964a63

Co-authored-by: JoJoJet <21144246+JoJoJet@users.noreply.github.com>

james7132 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 Apr 4, 2023

tim-blackbird approved these changes Apr 9, 2023

View reviewed changes

james7132 added this pull request to the merge queue Apr 9, 2023

Merged via the queue into bevyengine:main with commit f32ee63 Apr 9, 2023

ickshonpe mentioned this pull request Aug 21, 2023

Add Without<Parent> filter to sync_simple_transforms' orphaned entities query #9518

Merged

nicopap deleted the fix-orphaned-transform branch August 30, 2023 13:48

alice-i-cecile added A-ECS Entities, components, systems, and events and removed A-Hierarchy labels Jan 28, 2025

Uh oh!

Conversation

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

Uh oh!

Objective

Solution

Performance note

Changelog

Migration Guide

Uh oh!

james7132 Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

nicopap Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

nicopap Jan 18, 2023 • 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!

nicopap Jan 30, 2023 • 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!

joseph-gio Mar 31, 2023

Choose a reason for hiding this comment

Uh oh!

nicopap Apr 3, 2023 • 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!

Uh oh!

james7132 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Mar 7, 2023

Uh oh!

joseph-gio left a comment

Choose a reason for hiding this comment

Uh oh!

joseph-gio Mar 31, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nicopap commented Jan 18, 2023 •

edited

Loading

nicopap Jan 18, 2023 •

edited

Loading

nicopap Jan 30, 2023 •

edited

Loading

nicopap Apr 3, 2023 •

edited

Loading