bevy_reflect: Reflection-based cloning #13432

MrGVSV · 2024-05-20T01:30:13Z

Objective

Using Reflect::clone_value can be somewhat confusing to those unfamiliar with how Bevy's reflection crate works. For example take the following code:

let value: usize = 123;
let clone: Box<dyn Reflect> = value.clone_value();

What can we expect to be the underlying type of clone? If you guessed usize, then you're correct! Let's try another:

#[derive(Reflect, Clone)]
struct Foo(usize);

let value: Foo = Foo(123);
let clone: Box<dyn Reflect> = value.clone_value();

What about this code? What is the underlying type of clone? If you guessed Foo, unfortunately you'd be wrong. It's actually DynamicStruct.

It's not obvious that the generated Reflect impl actually calls Struct::clone_dynamic under the hood, which always returns DynamicStruct.

There are already some efforts to make this a bit more apparent to the end-user: #7207 changes the signature of Reflect::clone_value to instead return Box<dyn PartialReflect>, signaling that we're potentially returning a dynamic type.

But why can't we return Foo?

Foo can obviously be cloned— in fact, we already derived Clone on it. But even without the derive, this seems like something Reflect should be able to handle. Almost all types that implement Reflect either contain no data (trivially clonable), they contain a #[reflect_value] type (which, by definition, must implement Clone), or they contain another Reflect type (which recursively fall into one of these three categories).

This PR aims to enable true reflection-based cloning where you get back exactly the type that you think you do.

Solution

Add a Reflect::reflect_clone method which returns Result<Box<dyn Reflect>, ReflectCloneError>, where the Box<dyn Reflect> is guaranteed to be the same type as Self.

#[derive(Reflect)]
struct Foo(usize);

let value: Foo = Foo(123);
let clone: Box<dyn Reflect> = value.reflect_clone().unwrap();
assert!(clone.is::<Foo>());

Notice that we didn't even need to derive Clone for this to work: it's entirely powered via reflection!

Under the hood, the macro generates something like this:

fn reflect_clone(&self) -> Result<Box<dyn Reflect>, ReflectCloneError> {
    Ok(Box::new(Self {
        // The `reflect_clone` impl for `usize` just makes use of its `Clone` impl
        0: Reflect::reflect_clone(&self.0)?.take().map_err(/* ... */)?,
    }))
}

If we did derive Clone, we can tell Reflect to rely on that instead:

#[derive(Reflect, Clone)]
#[reflect(Clone)]
struct Foo(usize);

Generated Code

fn reflect_clone(&self) -> Result<Box<dyn Reflect>, ReflectCloneError> {
    Ok(Box::new(Clone::clone(self)))
}

Or, we can specify our own cloning function:

#[derive(Reflect)]
#[reflect(Clone(incremental_clone))]
struct Foo(usize);

fn incremental_clone(value: &usize) -> usize {
  *value + 1
}

Generated Code

fn reflect_clone(&self) -> Result<Box<dyn Reflect>, ReflectCloneError> {
    Ok(Box::new(incremental_clone(self)))
}

Similarly, we can specify how fields should be cloned. This is important for fields that are #[reflect(ignore)]'d as we otherwise have no way to know how they should be cloned.

#[derive(Reflect)]
struct Foo {
 #[reflect(ignore, clone)]
  bar: usize,
  #[reflect(ignore, clone = "incremental_clone")]
  baz: usize,
}

fn incremental_clone(value: &usize) -> usize {
  *value + 1
}

Generated Code

fn reflect_clone(&self) -> Result<Box<dyn Reflect>, ReflectCloneError> {
    Ok(Box::new(Self {
        bar: Clone::clone(&self.bar),
        baz: incremental_clone(&self.baz),
    }))
}

If we don't supply a clone attribute for an ignored field, then the method will automatically return Err(ReflectCloneError::FieldNotClonable {/* ... */}).

Err values "bubble up" to the caller. So if Foo contains Bar and the reflect_clone method for Bar returns Err, then the reflect_clone method for Foo also returns Err.

Attribute Syntax

You might have noticed the differing syntax between the container attribute and the field attribute.

This was purely done for consistency with the current attributes. There are PRs aimed at improving this. #7317 aims at making the "special-cased" attributes more in line with the field attributes syntactically. And #9323 aims at moving away from the stringified paths in favor of just raw function paths.

Compatibility with Unique Reflect

This PR was designed with Unique Reflect (#7207) in mind. This method actually wouldn't change that much (if at all) under Unique Reflect. It would still exist on Reflect and it would still Option<Box<dyn Reflect>>. In fact, Unique Reflect would only improve the user's understanding of what this method returns.

We may consider moving what's currently Reflect::clone_value to PartialReflect and possibly renaming it to partial_reflect_clone or clone_dynamic to better indicate how it differs from reflect_clone.

Testing

You can test locally by running the following command:

cargo test --package bevy_reflect

Changelog

Added Reflect::reflect_clone method
Added ReflectCloneError error enum
Added #[reflect(Clone)] container attribute
Added #[reflect(clone)] field attribute

cBournhonesque · 2024-05-20T14:03:53Z

crates/bevy_reflect/derive/src/container_attributes.rs

+    fn parse_clone(&mut self, input: ParseStream) -> syn::Result<()> {
+        let ident = input.parse::<kw::Clone>()?;
+
+        if input.peek(token::Paren) {


I wonder if we should use darling as a helper crate to parse attributes?
It seems easier than what you're doing, especially if the attributes become more complex later on

We could utilize something like darling in the future (not this PR though). I'm not sure our parsing logic is so complex we really need it, but it's certainly worth considering.

crates/bevy_reflect/derive/src/derive_data.rs

crates/bevy_reflect/derive/src/enum_utility.rs

crates/bevy_reflect/src/lib.rs

cBournhonesque · 2024-05-20T14:49:13Z

crates/bevy_reflect/derive/src/container_attributes.rs

@@ -509,6 +533,24 @@ impl ContainerAttributes {
        }
    }

+    pub fn get_clone_impl(&self, bevy_reflect_path: &Path) -> Option<proc_macro2::TokenStream> {


Where is this used? I couldn't understand this part.
I get the clone_impl on struct,tuple,enum,value,etc. but not this.

Or is this the top-level impl?

It's used by derive_data.rs. I could probably move this logic into that file directly, since it's the only place where it's used.

cBournhonesque

Looks good to me, I like the change!
I think the naming will be clearer when the PartialReflect/UniqueReflect PR gets merged as well.
Had some small comments, and i'll probably wait for the blocking PR to be merged before approving

# Objective Fix #10284. ## Solution When `DynamicSceneBuilder` extracts entities, they are cloned via `PartialReflect::clone_value`, making them into dynamic versions of the original components. This loses any custom `ReflectSerialize` type data. Dynamic scenes are deserialized with the original types, not the dynamic versions, and so any component with a custom serialize may fail. In this case `Rect` and `Vec2`. The dynamic version includes the field names 'x' and 'y' but the `Serialize` impl doesn't, hence the "expect float" error. The solution here: Instead of using `clone_value` to clone the components, `FromReflect` clones and retains the original information needed to serialize with any custom `Serialize` impls. I think using something like `reflect_clone` from (#13432) might make this more efficient. I also did the same when deserializing dynamic scenes to appease some of the round-trip tests which use `ReflectPartialEq`, which requires the types be the same and not a unique/proxy pair. I'm not sure it's otherwise necessary. Maybe this would also be more efficient when spawning dynamic scenes with `reflect_clone` instead of `FromReflect` again? An alternative solution would be to fall back to the dynamic version when deserializing `DynamicScene`s if the custom version fails. I think that's possible. Or maybe simply always deserializing via the dynamic route for dynamic scenes? ## Testing This example is similar to the original test case in #10284: ``` rust #![allow(missing_docs)] use bevy::{prelude::*, scene::SceneInstanceReady}; fn main() { App::new() .add_plugins(DefaultPlugins) .add_systems(Startup, (save, load).chain()) .observe(check) .run(); } static SAVEGAME_SAVE_PATH: &str = "savegame.scn.ron"; fn save(world: &mut World) { let entity = world.spawn(OrthographicProjection::default()).id(); let scene = DynamicSceneBuilder::from_world(world) .extract_entity(entity) .build(); if let Some(registry) = world.get_resource::<AppTypeRegistry>() { let registry = registry.read(); let serialized_scene = scene.serialize(&registry).unwrap(); // println!("{}", serialized_scene); std::fs::write(format!("assets/{SAVEGAME_SAVE_PATH}"), serialized_scene).unwrap(); } world.entity_mut(entity).despawn_recursive(); } fn load(mut commands: Commands, asset_server: Res<AssetServer>) { commands.spawn(DynamicSceneBundle { scene: asset_server.load(SAVEGAME_SAVE_PATH), ..default() }); } fn check(_trigger: Trigger<SceneInstanceReady>, query: Query<&OrthographicProjection>) { dbg!(query.single()); } ``` ## Migration Guide The `DynamicScene` format is changed to use custom serialize impls so old scene files will need updating: Old: ```ron ( resources: {}, entities: { 4294967299: ( components: { "bevy_render::camera::projection::OrthographicProjection": ( near: 0.0, far: 1000.0, viewport_origin: ( x: 0.5, y: 0.5, ), scaling_mode: WindowSize(1.0), scale: 1.0, area: ( min: ( x: -1.0, y: -1.0, ), max: ( x: 1.0, y: 1.0, ), ), ), }, ), }, ) ``` New: ```ron ( resources: {}, entities: { 4294967299: ( components: { "bevy_render::camera::projection::OrthographicProjection": ( near: 0.0, far: 1000.0, viewport_origin: (0.5, 0.5), scaling_mode: WindowSize(1.0), scale: 1.0, area: ( min: (-1.0, -1.0), max: (1.0, 1.0), ), ), }, ), }, ) ``` --------- Co-authored-by: Gino Valente <[email protected]>

pablo-lua

Very good PR, I just have some small questions :D

pablo-lua · 2024-10-07T21:48:11Z

crates/bevy_reflect/derive/src/enum_utility.rs

+                    return #FQResult::Err(
+                        #bevy_reflect_path::ReflectCloneError::FieldNotClonable {
+                            field: #field_id,
+                            variant: #FQOption::Some(::std::borrow::Cow::Borrowed(#variant_name)),
+                            container_type_path: ::std::borrow::Cow::Borrowed(<Self as #bevy_reflect_path::TypePath>::type_path())
+                        }
+                    )


If I understood it right, if we have an ignored field, we can only fallback to Derive impl, right?

pablo-lua · 2024-10-07T21:54:10Z

crates/bevy_reflect/src/impls/glam.rs

@@ -4,15 +4,15 @@ use bevy_reflect_derive::{impl_reflect, impl_reflect_value};
 use glam::*;

 impl_reflect!(
-    #[reflect(Debug, Hash, PartialEq, Default)]
+    #[reflect(Clone, Debug, Hash, PartialEq, Default)]


Question: Should we use #[reflect(Clone)] Whenever possible?

pablo-lua · 2024-10-07T22:00:54Z

crates/bevy_reflect/src/reflect.rs

+    /// Unlike [`Reflect::clone_value`], which often returns a dynamic representation of `Self`,
+    /// this method attempts create a clone of `Self` directly, if possible.
+    ///
+    /// If the clone cannot be performed, `None` is returned.


I think we must warn the user which cases can return None
From what I read in this PR, this returns None if Clone isn't implemented for all types, but when I first read the PR description, this left me very confused: How can Clone fail?

# Objective #16132 introduced entity cloning functionality, and while it works and is useful, it can be made faster. This is the promised follow-up to improve performance. ## Solution **PREFACE**: This is my first time writing `unsafe` in rust and I have only vague idea about what I'm doing. I would encourage reviewers to scrutinize `unsafe` parts in particular. The solution is to clone component data to an intermediate buffer and use `EntityWorldMut::insert_by_ids` to insert components without additional archetype moves. To facilitate this, `EntityCloner::clone_entity` now reads all components of the source entity and provides clone handlers with the ability to read component data straight from component storage using `read_source_component` and write to an intermediate buffer using `write_target_component`. `ComponentId` is used to check that requested type corresponds to the type available on source entity. Reflect-based handler is a little trickier to pull of: we only have `&dyn Reflect` and no direct access to the underlying data. `ReflectFromPtr` can be used to get `&dyn Reflect` from concrete component data, but to write it we need to create a clone of the underlying data using `Reflect`. For this reason only components that have `ReflectDefault` or `ReflectFromReflect` or `ReflectFromWorld` can be cloned, all other components will be skipped. The good news is that this is actually only a temporary limitation: once #13432 lands we will be able to clone component without requiring one of these `type data`s. This PR also introduces `entity_cloning` benchmark to better compare changes between the PR and main, you can see the results in the **showcase** section. ## Testing - All previous tests passing - Added test for fast reflect clone path (temporary, will be removed after reflection-based cloning lands) - Ran miri ## Showcase Here's a table demonstrating the improvement: | **benchmark** | **main, avg** | **PR, avg** | **change, avg** | | ----------------------- | ------------- | ----------- | --------------- | | many components reflect | 18.505 µs | 2.1351 µs | -89.095% | | hierarchy wide reflect* | 22.778 ms | 4.1875 ms | -81.616% | | hierarchy tall reflect* | 107.24 µs | 26.322 µs | -77.141% | | hierarchy many reflect | 78.533 ms | 9.7415 ms | -87.596% | | many components clone | 1.3633 µs | 758.17 ns | -45.937% | | hierarchy wide clone* | 2.7716 ms | 3.3411 ms | +20.546% | | hierarchy tall clone* | 17.646 µs | 20.190 µs | +17.379% | | hierarchy many clone | 5.8779 ms | 4.2650 ms | -27.439% | *: these benchmarks have entities with only 1 component ## Considerations Once #10154 is resolved a large part of the functionality in this PR will probably become obsolete. It might still be a little bit faster than using command batching, but the complexity might not be worth it. ## Migration Guide - `&EntityCloner` in component clone handlers is changed to `&mut ComponentCloneCtx` to better separate data. - Changed `EntityCloneHandler` from enum to struct and added convenience functions to add default clone and reflect handler more easily. --------- Co-authored-by: Alice Cecile <[email protected]> Co-authored-by: Chris Russell <[email protected]>

MrGVSV added C-Feature A new feature, making something new possible S-Blocked This cannot move forward until something else changes A-Reflection Runtime information about types D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels May 20, 2024

MrGVSV added this to the 0.15 milestone May 20, 2024