Handle unsuccessful add and remove of entities in `RelationshipTarget`

[Dampfwalze]

RelationshipSourceCollection::add and RelationshipSourceCollection::remove are designed to be fallible. They return a bool to indicate that. Currently the on_insert and on_replace hooks on a RelationshipTarget make no effort to handle the case where this actually fails. This can result in an invalid state if an entity did not get added.

bevy/crates/bevy_ecs/src/relationship/relationship_source_collection.rs

Lines 39 to 50 in f47b1c0

	/// Adds the given `entity` to the collection.
	///
	/// Returns whether the entity was added to the collection.
	/// Mainly useful when dealing with collections that don't allow
	/// multiple instances of the same entity ([`EntityHashSet`]).
	fn add(&mut self, entity: Entity) -> bool;
	/// Removes the given `entity` from the collection.
	///
	/// Returns whether the collection actually contained
	/// the entity.
	fn remove(&mut self, entity: Entity) -> bool;

On line 118, 121 and 159:

bevy/crates/bevy_ecs/src/relationship/mod.rs

Lines 84 to 180 in f47b1c0

	/// The `on_insert` component hook that maintains the [`Relationship`] / [`RelationshipTarget`] connection.
	fn on_insert(
	mut world: DeferredWorld,
	HookContext {
	entity,
	caller,
	relationship_hook_mode,
	..
	}: HookContext,
	) {
	match relationship_hook_mode {
	RelationshipHookMode::Run => {}
	RelationshipHookMode::Skip => return,
	RelationshipHookMode::RunIfNotLinked => {
	if <Self::RelationshipTarget as RelationshipTarget>::LINKED_SPAWN {
	return;
	}
	}
	}
	let target_entity = world.entity(entity).get::<Self>().unwrap().get();
	if target_entity == entity {
	warn!(
	"{}The {}({target_entity:?}) relationship on entity {entity:?} points to itself. The invalid {} relationship has been removed.",
	caller.map(|location|format!("{location}: ")).unwrap_or_default(),
	core::any::type_name::<Self>(),
	core::any::type_name::<Self>()
	);
	world.commands().entity(entity).remove::<Self>();
	return;
	}
	if let Ok(mut target_entity_mut) = world.get_entity_mut(target_entity) {
	if let Some(mut relationship_target) =
	target_entity_mut.get_mut::<Self::RelationshipTarget>()
	{
	relationship_target.collection_mut_risky().add(entity);
	} else {
	let mut target = <Self::RelationshipTarget as RelationshipTarget>::with_capacity(1);
	target.collection_mut_risky().add(entity);
	world.commands().entity(target_entity).insert(target);
	}
	} else {
	warn!(
	"{}The {}({target_entity:?}) relationship on entity {entity:?} relates to an entity that does not exist. The invalid {} relationship has been removed.",
	caller.map(|location|format!("{location}: ")).unwrap_or_default(),
	core::any::type_name::<Self>(),
	core::any::type_name::<Self>()
	);
	world.commands().entity(entity).remove::<Self>();
	}
	}
	/// The `on_replace` component hook that maintains the [`Relationship`] / [`RelationshipTarget`] connection.
	// note: think of this as "on_drop"
	fn on_replace(
	mut world: DeferredWorld,
	HookContext {
	entity,
	relationship_hook_mode,
	..
	}: HookContext,
	) {
	match relationship_hook_mode {
	RelationshipHookMode::Run => {}
	RelationshipHookMode::Skip => return,
	RelationshipHookMode::RunIfNotLinked => {
	if <Self::RelationshipTarget as RelationshipTarget>::LINKED_SPAWN {
	return;
	}
	}
	}
	let target_entity = world.entity(entity).get::<Self>().unwrap().get();
	if let Ok(mut target_entity_mut) = world.get_entity_mut(target_entity) {
	if let Some(mut relationship_target) =
	target_entity_mut.get_mut::<Self::RelationshipTarget>()
	{
	relationship_target.collection_mut_risky().remove(entity);
	if relationship_target.len() == 0 {
	let command = |mut entity: EntityWorldMut| {
	// this "remove" operation must check emptiness because in the event that an identical
	// relationship is inserted on top, this despawn would result in the removal of that identical
	// relationship ... not what we want!
	if entity
	.get::<Self::RelationshipTarget>()
	.is_some_and(RelationshipTarget::is_empty)
	{
	entity.remove::<Self::RelationshipTarget>();
	}
	};
	world
	.commands()
	.queue(command.with_entity(target_entity).handle_error_with(ignore));
	}
	}
	}
	}
	}

Problem

There are two cases (I can think of) for why an implementation of add would fail:

1. The entity already existed in the collection (supported)
2. The collection is full

There are also two cases (I can think of) for why an implementation of remove would fail:

1. The entity did not exist in the collection (supported, unreachable with valid state)
2. The entity exists in the collection, but cannot be removed (really niche, debatable if needed)

The problem is that respectively only one case is accepted by the current implementation. In the case of add, both cases exist currently in bevy: The RelationshipSourceCollection trait is implemented by the EntityHashSet and by Entity, the latter acting as a collection with a maximum size of one. When a second entity gets added to the Entity, currently this case is handled with a panic.

Solution

Especially the case where the collection has a maximum size is very important, so this should be handled correctly so that it does not result in an invalid state.

To handle the case where the collection is full, there are two possibilities: Panicing, since this should not be allowed, or cancelling the relationship by removing the Relationship component. But this also requires to correctly identify the two cases. For that I have two solutions:

Solution 1

Change the meaning of the result of RelationshipSourceCollection::add to indicate the collection is full, since the case where it already existed can simply be handled by not caring about that (like it currently is).

Solution 2

Introduce a result enum to indicate the outcome of the operation, either to indicate all outcomes:

enum CollectionAddResult {
    Success,
    AlreadyExisting,
    NoSpace,
}

Or wrapping it in a Result:

enum CollectionAddError {
    AlreadyExisting,
    NoSpace,
}
trait RelationshipSourceCollection {
    fn add(&mut self, entity: Entity) -> Result<(), CollectionAddError>
}