Skip to content

Preserve extra data in RelationshipTarget with replace_related(_with_difference) #19588

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

urben1680
Copy link
Contributor

@urben1680 urben1680 commented Jun 11, 2025

Objective

The methods and commands replace_related and replace_related_with_difference may cause data stored at the RelationshipTarget be lost when all original children are removed before new children are added.

Part of #19589

Solution

Fix the issue, either by removing the old children after adding the new ones and not before (replace_related_with_difference) or by taking the whole RelationshipTarget to modify it, not only the inner collection (replace_related).

Testing

I added a new test asserting the data is kept. I also added a general test of these methods as they had none previously.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events X-Uncontroversial This work is generally agreed upon S-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Jun 11, 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.

Good; thanks for the tests here. This is too tricky to be confident without them.

Copy link
Contributor

@gwafotapa gwafotapa 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 to me! I would just like to see the dead code disappear.

On a side note, I'm not sure why replace_related_with_difference needs the entities_to_relate argument.

.insert_with_relationship_hook_mode(R::from(this), RelationshipHookMode::Skip);
}
});

if !entities_to_relate.is_empty() {
if let Some(mut target) = self.get_mut::<R::RelationshipTarget>() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the if at line 244, this if let else always branches on if let.

.insert_with_relationship_hook_mode(R::from(this), RelationshipHookMode::Skip);
}
});

if !entities_to_relate.is_empty() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this test is necessary.

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 14, 2025
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 D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants