bevy_reflect: Deprecate PartialReflect::clone_value

[MrGVSV]

Objective

#13432 added proper reflection-based cloning. This is a better method than cloning via clone_value for reasons detailed in the description of that PR. However, it may not be immediately apparent to users why one should be used over the other, and what the gotchas of clone_value are.

Solution

This PR marks PartialReflect::clone_value as deprecated, with the deprecation notice pointing users to PartialReflect::reflect_clone. However, it also suggests using a new method introduced in this PR: PartialReflect::to_dynamic.

PartialReflect::to_dynamic is essentially a renaming of PartialReflect::clone_value. By naming it to_dynamic, we make it very obvious that what's returned is a dynamic type. The one caveat to this is that opaque types still use reflect_clone as they have no corresponding dynamic type.

Along with changing the name, the method is now optional, and comes with a default implementation that calls out to the respective reflection subtrait method. This was done because there was really no reason to require manual implementors provide a method that almost always calls out to a known set of methods.

Lastly, to make this default implementation work, this PR also did a similar thing with the clone_dynamic methods on the reflection subtraits. For example, Struct::clone_dynamic has been marked deprecated and is superseded by Struct::to_dynamic_struct. This was necessary to avoid the "multiple names in scope" issue.

Open Questions

This PR maintains the original signature of clone_value on to_dynamic. That is, it takes &self and returns Box<dyn PartialReflect>.

However, in order for this to work, it introduces a panic if the value is opaque and doesn't override the default reflect_clone implementation.

One thing we could do to avoid the panic would be to make the conversion fallible, either returning Option<Box<dyn PartialReflect>> or Result<Box<dyn PartialReflect>, ReflectCloneError>.

This makes using the method a little more involved (i.e. users have to either unwrap or handle the rare possibility of an error), but it would set us up for a world where opaque types don't strictly need to be Clone. Right now this bound is sort of implied by the fact that clone_value is a required trait method, and the default behavior of the macro is to use Clone for opaque types.

Alternatively, we could keep the signature but make the method required. This maintains that implied bound where manual implementors must provide some way of cloning the value (or YOLO it and just panic), but also makes the API simpler to use.

Finally, we could just leave it with the panic. It's unlikely this would occur in practice since our macro still requires Clone for opaque types, and thus this would only ever be an issue if someone were to manually implement PartialReflect without a valid to_dynamic or reflect_clone method.

Testing

You can test locally using the following command:

cargo test --package bevy_reflect --all-features

---

Migration Guide

PartialReflect::clone_value is being deprecated. Instead, use PartialReflect::to_dynamic if wanting to create a new dynamic instance of the reflected value. Alternatively, use PartialReflect::reflect_clone to attempt to create a true clone of the underlying value.

Similarly, the following methods have been deprecated and should be replaced with these alternatives:

• Array::clone_dynamic → Array::to_dynamic_array
• Enum::clone_dynamic → Enum::to_dynamic_enum
• List::clone_dynamic → List::to_dynamic_list
• Map::clone_dynamic → Map::to_dynamic_map
• Set::clone_dynamic → Set::to_dynamic_set
• Struct::clone_dynamic → Struct::to_dynamic_struct
• Tuple::clone_dynamic → Tuple::to_dynamic_tuple
• TupleStruct::clone_dynamic → TupleStruct::to_dynamic_tuple_struct

[eugineerd]

Entity cloning changes look good to me and seem to increase performance for Reflect-based path in the benchmark by ~50% on my machine.

[alice-i-cecile]

@MrGVSV I'm comfortable merging this when CI is green; just say the word.

[MrGVSV]

@MrGVSV I'm comfortable merging this when CI is green; just say the word.

Sounds good, I'll try to do that soon!