Skip to content

Conversation

@cart
Copy link
Member

@cart cart commented Apr 12, 2025

Objective

One to one relationships (added in #18087) can currently easily be invalidated by having two entities relate to the same target:

    fn one_to_one_relationships() {
        #[derive(Component)]
        #[relationship(relationship_target = Below)]
        struct Above(Entity);

        #[derive(Component)]
        #[relationship_target(relationship = Above)]
        struct Below(Entity);

        let mut world = World::new();
        let a = world.spawn_empty().id();
        let b = world.spawn_empty().id();
        let c = world.spawn_empty().id();

        world.entity_mut(a).insert(Above(c));
        assert_eq!(a, world.get::<Below>(c).unwrap().0);
        world.entity_mut(b).insert(Above(c));
        assert_eq!(b, world.get::<Below>(c).unwrap().0);
        // this still erroneously points to c
        assert_eq!(c, world.get::<Above>(a).unwrap().0);
    }

Solution

Revert one-to-one relationships. We can re-add them in the future if the impl properly takes this case into account.

@cart cart added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events labels Apr 12, 2025
@cart cart added this to the 0.16 milestone Apr 12, 2025
@urben1680
Copy link
Contributor

urben1680 commented Apr 12, 2025

This can still be implemented easily by the user by newtyping Entity, right? The user would likely want to add a panic when the current entity is not PLACEHOLDER.

A panicking version:
(also replaced the iteration type as I am not sure if iter may be called if it is a PLACEHOLDER.

impl RelationshipSourceCollection for MyNewtyped(Entity) {
    type SourceIter<'a> = core::option::IntoIter<Entity>;

    fn new() -> Self {
        MyNewtyped(Entity::PLACEHOLDER)
    }

    fn reserve(&mut self, _: usize) {}

    fn with_capacity(_capacity: usize) -> Self {
        MyNewtyped(Self::new())
    }

    fn add(&mut self, entity: Entity) -> bool {
        assert_eq!(self.0, Entity::PLACEHOLDER, "remove the original relationship first");
        self.0 = entity;

        true
    }

    fn remove(&mut self, entity: Entity) -> bool {
        if self.0 == entity {
            self.0 = Entity::PLACEHOLDER;

            return true;
        }

        false
    }

    fn iter(&self) -> Self::SourceIter<'_> {
        if self.0 == Entity::PLACEHOLDER {
            None.into_iter()
        } else {
            Some(self.0).into_iter()
        }
    }

    fn len(&self) -> usize {
        if self.0 == Entity::PLACEHOLDER {
            return 0;
        }
        1
    }

    fn clear(&mut self) {
        self.0 = Entity::PLACEHOLDER;
    }

    fn shrink_to_fit(&mut self) {}

    fn extend_from_iter(&mut self, entities: impl IntoIterator<Item = Entity>) {
        if let Some(entity) = entities.into_iter().last() {
            assert_eq!(self.0, Entity::PLACEHOLDER, "remove the original relationship first");
            self.0 = entity;
        }
    }
}

A proper fix here would be if RelationshipSourceCollection::add returned an Option in case the collection was full, and RelationshipSourceCollection::extend_from_iter a draining iterator of Entity. Then the caller could react on it.

@cart
Copy link
Member Author

cart commented Apr 13, 2025

Good call! I do prefer panicking over fully removing the API. The newtype isn't necessary though! Just put out a PR.

@mockersf
Copy link
Member

closed in favour of #18833

@mockersf mockersf closed this Apr 15, 2025
github-merge-queue bot pushed a commit that referenced this pull request Apr 15, 2025
# Objective

One to one relationships (added in
#18087) can currently easily be
invalidated by having two entities relate to the same target.

Alternative to #18817 (removing one-to-one relationships)

## Solution

Panic if a RelationshipTarget is already targeted. Thanks @urben1680 for
the idea!

---------

Co-authored-by: François Mockers <[email protected]>
mockersf added a commit that referenced this pull request Apr 15, 2025
# Objective

One to one relationships (added in
#18087) can currently easily be
invalidated by having two entities relate to the same target.

Alternative to #18817 (removing one-to-one relationships)

## Solution

Panic if a RelationshipTarget is already targeted. Thanks @urben1680 for
the idea!

---------

Co-authored-by: François Mockers <[email protected]>
jf908 pushed a commit to jf908/bevy that referenced this pull request May 12, 2025
# Objective

One to one relationships (added in
bevyengine#18087) can currently easily be
invalidated by having two entities relate to the same target.

Alternative to bevyengine#18817 (removing one-to-one relationships)

## Solution

Panic if a RelationshipTarget is already targeted. Thanks @urben1680 for
the idea!

---------

Co-authored-by: François Mockers <[email protected]>
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants