bevyengine / bevyengine/bevy

Handle unsuccessful add and remove of entities in `RelationshipTarget`

Open
#19,636 0 comments 0 reactions 0 assignees View on GitHub
A-ECS C-Bug D-Modest S-Ready-For-Implementation
Dominant language
Rust
Stars
48.2k
Forks
4.8k
Avg merge
3d 22h
Merged PRs (30d)
161

Description

`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.

https://github.com/bevyengine/bevy/blob/f47b1c00ee6c55f98f1858db6d6bc1fc1a4bed0e/crates/bevy_ecs/src/relationship/relationship_source_collection.rs#L39-L50

On line 118, 121 and 159:

https://github.com/bevyengine/bevy/blob/f47b1c00ee6c55f98f1858db6d6bc1fc1a4bed0e/crates/bevy_ecs/src/relationship/mod.rs#L84-L180

### 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`](https://docs.rs/bevy_ecs/0.16.1/bevy_ecs/relationship/trait.RelationshipSourceCollection.html) trait is implemented by the [`EntityHashSet`](https://docs.rs/bevy_ecs/0.16.1/bevy_ecs/entity/hash_set/struct.EntityHashSet.html) 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:

```rust
enum CollectionAddResult {
Success,
AlreadyExisting,
NoSpace,
}
```

Or wrapping it in a `Result`:

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

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.