Skip to content

EntityWorldMut methods do not automatically overwrite Relationship components #19601

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

Conversation

urben1680
Copy link
Contributor

@urben1680 urben1680 commented Jun 12, 2025

Objective

Some methods and commands carelessly overwrite Relationship components. This may overwrite additional data stored at them which is undesired.

Part of #19589

Solution

A new private method will be used instead of insert: modify_or_insert_relation_with_relationship_hook_mode.

This method behaves different to insert if Relationship is a larger type than Entity and already contains this component. It will then use the modify_component API and a new Relationship::set_risky method to set the related entity, keeping all other data untouched.

For the replace_related(_with_difference) methods this also required a InsertHookMode parameter for efficient modifications of multiple children. The changes here are limited to the non-public methods.

I would appreciate feedback if this is all good.

Testing

Added tests of all methods that previously could reset Relationship data.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events S-Blocked This cannot move forward until something else changes labels Jun 13, 2025
@urben1680 urben1680 marked this pull request as ready for review June 15, 2025 18:26
@alice-i-cecile alice-i-cecile added M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Blocked This cannot move forward until something else changes labels Jun 15, 2025
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Okay, this fix makes sense to me, the code is good, the migration guide is good and there are tests. I have a couple small suggestions, but nothing blocking.

@urben1680 urben1680 requested a review from copygirl June 21, 2025 10:52
@urben1680 urben1680 requested a review from copygirl June 21, 2025 21:37
Copy link
Contributor

@copygirl copygirl left a comment

Choose a reason for hiding this comment

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

Only got a final nitpick, otherwise I couldn't see anything that stood out to me.

(Got pleasantly surprised I was asked to review, hope I'm doing this properly.)

@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 Jun 21, 2025
@alice-i-cecile
Copy link
Member

(Got pleasantly surprised I was asked to review, hope I'm doing this properly.)

It's much appreciated!

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 22, 2025
Merged via the queue into bevyengine:main with commit c6ae964 Jun 22, 2025
32 checks passed
@urben1680 urben1680 deleted the related-methods-try-to-modify-relationship branch June 26, 2025 21:44
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 M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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.

3 participants