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

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

Conversation

ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented Aug 20, 2023

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.

…` to retrieve entities orphaned since the last update.

Without this filter, `sync_simple_transforms` will overwrite the `GlobalTransform` of entities that had their parent removed and then replaced in the same frame.
@ickshonpe ickshonpe changed the title Add Without<Parent> filter to sync_simple_transforms query for orphaned entities Add Without<Parent> filter to sync_simple_transforms' orphaned entities query Aug 20, 2023
@JoJoJet
Copy link
Member

JoJoJet commented Aug 20, 2023

Can you add some regression tests to make sure that this fixes the issues?

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Transform Translations, rotations and scales labels Aug 21, 2023
@alice-i-cecile
Copy link
Member

Definitely a reasonable fix, but I'd also like to see a test or three.

@ickshonpe
Copy link
Contributor Author

Can you add some regression tests to make sure that this fixes the issues?

Will do.

I'd like to get @nicopap to review as well. I think my understanding of the problem is correct, but want to make sure that there wasn't a good reason for not filtering by Without<Parent>.

Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

I didn't think of entities which Parent component was removed and later added back. I was thinking "if the Parent component is removed, then why check for Without<Parent>?", glossing over situations where it is removed and later added back.

@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 Aug 21, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 28, 2023
Merged via the queue into bevyengine:main with commit aa20565 Aug 28, 2023
21 checks passed
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Transform Translations, rotations and scales 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.

sync_simple_transforms only checks for removed parents and doesn't filter for Without<Parent>
5 participants