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

[johanhelsing]

Bevy version

v0.9.1

What you did

The documentation for ReflectComponent currently reads:

    /// 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:

/// 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<L: List>(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
• Needless Added triggering in bevy_ggrs
• discussion on discord
• related: reflect: insert and remove methods for List #7061

[soqb]

this is not actually blocked on #7061 since we can use List::pop to remove elements already.

[MrGVSV]

I’m working on adding diff support to bevy_reflect (currently partially blocked by #6971). Diffing would allow insert/remove operations to be performed on List in a much more explicit way. Because of this, it might make sense to change Reflect::apply to fully replace List and Array values as suggested in this issue. Otherwise, the only alternative is Reflect::set, which won't work for Dynamic types and borrowed data.

[nicopap]

@MrGVSV @soqb I think the issue doesn't relate to the behavior of Reflect::apply (which also is well documented), but the documentation of ReflectComponent::apply. Updating the documentation to reflect the behavior also works. No need for changing code or new methods.

[johanhelsing]

@MrGVSV @soqb I think the issue doesn't relate to the behavior of Reflect::apply (which also is well documented), but the documentation of ReflectComponent::apply. Updating the documentation to reflect the behavior also works. No need for changing code or new methods.

This still means we would have no way to fully set a component except remove followed by insert.

Is there a compelling reason for Reflect::apply to work that way (except backwards compatibility)?

[nicopap]

Reflect has a set method, which does what you describe as the expected behavior of ReflectComponent::apply. ReflectComponent::apply should probably renamed to set and updated to use Reflect::set.

[johanhelsing]

We would need a new ReflectComponent::set to access it, though?

And as mentioned earlier it wouldn't work with borrowed data

Edit: Sorry, I misread your comment

[MrGVSV]

Reflect has a set method, which does what you describe as the expected behavior of ReflectComponent::apply. ReflectComponent::apply should probably renamed to set and updated to use Reflect::set.

Reflect::set requires the actual type, though. This prevents us from passing it Dynamic data like DynamicStruct (which, for something like scenes, is almost always the case)

[detrimentalist]

I have been tripped up by this when using hot reloading of scenes. If a component in the file contains a Vec, only changes that grows the Vec or keeps it at the same length has the expected outcome. My suggestion for a fix would simply be to remove the excess elems from the list like so:

pub fn list_apply<L: List>(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 {
                a.push(value.clone_value());
            }
        }

        while a.len() > list_value.len() {
            a.remove(a.len() - 1);
        }
    } else {
        panic!("Attempted to apply a non-list type to a list type.");
    }
}

Is there any reason why this wasn't already implemented?

[PROMETHIA-27]

I've also run into this now. While the current list behavior is consistent with structs/tuples/etc, there is definitely a need for a set that works with dynamic values. I think MrGVSV's diff solution will probably be the way forward, though. It sounds the most simple and flexible.

[MrGVSV]

I think MrGVSV's diff solution will probably be the way forward, though. It sounds the most simple and flexible.

Yeah I hope so. I'm going to try to find time for it this month. Ideally, I could at least get it up in draft PR form for initial thoughts