Skip to content

Fix EntityCloner replacing required components. #19326

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

Merged

Conversation

eugineerd
Copy link
Contributor

Objective

Fix #19324

Solution

EntityCloner replaces required components when filtering. This is unexpected when comparing with the way the rest of bevy handles required components. This PR separates required components from explicit components when filtering in EntityClonerBuilder.

Testing

Added a regression test for this case.

@alice-i-cecile alice-i-cecile added this to the 0.16.1 milestone May 21, 2025
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 21, 2025
Copy link
Contributor

@urben1680 urben1680 left a comment

Choose a reason for hiding this comment

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

Looks good. Nice that this does not come with an API change.

@urben1680
Copy link
Contributor

urben1680 commented May 22, 2025

I wonder if this can be extended a bit. If you could manually populate the new filter_required field, you could basically make the cloner fit for insert-if-new capability. I certainly would make use of that for my crate.

An API do allow/deny by BundleId would be interesting for me too. 🤔 When this is merged I could implement the features in my own PR and see if this fits into 0.16.1 or not.

@eugineerd
Copy link
Contributor Author

I wonder if this can be extended a bit. If you could manually populate the new filter_required field, you could basically make the cloner fit for insert-if-new capability. I certainly would make use of that for my crate.

Yeah, that could be useful. Although I don't think new API is supposed to be introduced in patch releases, so I think it's better to leave this PR just as a bugfix for 0.16.1 and add new stuff in 0.17.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 30, 2025
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 30, 2025
Merged via the queue into bevyengine:main with commit 0694743 May 30, 2025
41 checks passed
mockersf pushed a commit that referenced this pull request May 30, 2025
# Objective
Fix #19324

## Solution
`EntityCloner` replaces required components when filtering. This is
unexpected when comparing with the way the rest of bevy handles required
components. This PR separates required components from explicit
components when filtering in `EntityClonerBuilder`.

## Testing
Added a regression test for this case.
mockersf pushed a commit that referenced this pull request May 30, 2025
# Objective
Fix #19324

## Solution
`EntityCloner` replaces required components when filtering. This is
unexpected when comparing with the way the rest of bevy handles required
components. This PR separates required components from explicit
components when filtering in `EntityClonerBuilder`.

## Testing
Added a regression test for this case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events 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.

EntityCloner has no concept of required components after configuration which may have surprising effects depending on the target archetype
3 participants