Skip to content

Rework one-to-one relationships to remove existing instead of panicking #20232

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
merged 4 commits into from
Jul 23, 2025

Conversation

muddxyii
Copy link
Contributor

Objective

Solution

  • Modified the on_insert hook in the Relationship trait to detect one-to-one relationships (where the target collection type is Entity) and automatically remove any existing relationship before establishing the new one.
  • Uses runtime type checking with TypeId to identify one-to-one vs one-to-many relationships.
  • Safely removes the existing source relationship using try_remove() to handle edge cases gracefully.

Testing

  • Removed panic assertions from Entity::add() and Entity::extend_from_iter() methods that would previously crash when attempting to establish overlapping one-to-one relationships
  • Modified existing test one_to_one_relationship_shared_target by removing #[should_panic] and adding assertions to verify:
      - Original relationship is properly removed
      - New relationship is correctly established
      - Target entity points to the new source

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events P-Crash A sudden unexpected crash S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 21, 2025
@alice-i-cecile alice-i-cecile added the X-Uncontroversial This work is generally agreed upon label Jul 21, 2025
@alice-i-cecile alice-i-cecile added this to the 0.17 milestone Jul 21, 2025
@alice-i-cecile alice-i-cecile requested a review from chescock July 21, 2025 19:30
}

if let Some(current_source) = current_source_to_remove {
world.commands().entity(current_source).try_remove::<Self>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this have a race condition if we do a batch insert, similar to the one #19519 fixed? If we insert multiple one-to-one relationship sources without flushing, this will remove any previous source from the target, but I think later sources in the batch will overwrite earlier ones without removing.

This might be easier to fix if we merge #19932 first and do this whole check inside the command.

@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 Jul 21, 2025
Copy link
Contributor

@chescock chescock 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!

I'm still worried about reintroducing a race condition in batch inserts, especially since we just fixed one there, but we can probably fix that in a follow up after #19932.

@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-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jul 23, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jul 23, 2025
Merged via the queue into bevyengine:main with commit 069f110 Jul 23, 2025
34 checks passed
@muddxyii muddxyii deleted the rework-one-to-one-relations branch July 23, 2025 19:01
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 P-Crash A sudden unexpected crash S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants