bevyengine / bevyengine/bevy

The documentation for `ReflectComponent` is inconsistent with how `Reflect` is implemented for lists

Open
#7,129 10 comments 1 reaction 0 assignees View on GitHub
A-Reflection C-Docs
Dominant language
Rust
Stars
48.2k
Forks
4.8k
Avg merge
3d 22h
Merged PRs (30d)
161

Description

## Bevy version

v0.9.1

## What you did

The documentation for `ReflectComponent` currently reads:

```rust
/// Uses reflection to set the value of this [`Component`] type in the entity to the given value.
```

However, internally, the implementation of `apply` for lists:

```rust
/// Applies the elements of `b` to the corresponding elements of `a`.
///
/// If the length of `b` is greater than that of `a`, the excess elements of `b`
/// are cloned and appended to `a`.
///
/// # Panics
///
/// This function panics if `b` is not a list.
#[inline]
pub fn list_apply(a: &mut L, b: &dyn Reflect) {
if let ReflectRef::List(list_value) = b.reflect_ref() {
for (i, value) in list_value.iter().enumerate() {
if i < a.len() {
if let Some(v) = a.get_mut(i) {
v.apply(value);
}
} else {
List::push(a, value.clone_value());
}
}
} else {
panic!("Attempted to apply a non-list type to a list type.");
}
}
```

This means `List[1, 2, 3].apply(List[4, 5]) == List[4, 5, 3]`.

So the result of calling `ReflectComponent::apply()` on a component with a list in it would not actually result in the components being equal afterwards.

In particular, this was causing problems in `bevy_ggrs`, where the `Children` component is being rolled back. We used `ReflectComponent::apply` since that sounded like the right thing to do, but since `list_apply` didn't remove elements if `self.len()` > `value.len()`, we were getting leftover children that shouldn't be there.

To work around this, we used `ReflectComponent::remove` followed by `ReflectComponent::insert`. However, by doing this it is causing unnecessary copies of the entire children hierarchy on every snapshot restore, and also needlessly triggering `Added` queries, causing further performance regressions.

## What went wrong

I think the least confusing thing for users, would be if `Reflect::apply` for lists actually removed excess elements, so `List[1, 2, 3].apply(List[4, 5]) == List[4, 5]`

Alternatively, the documentation for `ReflectComponent::apply` should specifically warn about this gotcha, and we need some alternative way of performing in-place updates of components.

## Additional information

- [bug fixed in bevy_ggrs](https://github.com/gschup/bevy_ggrs/pull/34)
- [Needless `Added` triggering in bevy_ggrs](https://github.com/gschup/bevy_ggrs/issues/39)
- [discussion on discord](https://discord.com/channels/691052431525675048/1002362493634629796/1061572585516711936)
- related: #7061

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.