Open johanhelsing opened 1 year ago
this is not actually blocked on #7061 since we can use List::pop
to remove elements already.
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.
@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.
@MrGVSV @soqb I think the issue doesn't relate to the behavior of
Reflect::apply
(which also is well documented), but the documentation ofReflectComponent::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)?
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
.
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
Reflect
has aset
method, which does what you describe as the expected behavior ofReflectComponent::apply
.ReflectComponent::apply
should probably renamed toset
and updated to useReflect::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)
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?
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.
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
Bevy version
v0.9.1
What you did
The documentation for
ReflectComponent
currently reads:However, internally, the implementation of
apply
for lists: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 theChildren
component is being rolled back. We usedReflectComponent::apply
since that sounded like the right thing to do, but sincelist_apply
didn't remove elements ifself.len()
>value.len()
, we were getting leftover children that shouldn't be there.To work around this, we used
ReflectComponent::remove
followed byReflectComponent::insert
. However, by doing this it is causing unnecessary copies of the entire children hierarchy on every snapshot restore, and also needlessly triggeringAdded
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, soList[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
Added
triggering in bevy_ggrs