The documentation for `ReflectComponent` is inconsistent with how `Reflect` is implemented for lists
- 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
Assessment
This issue has not been assessed yet.