Use FromReflect when extracting entities in dynamic scenes

[yrns]

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

#![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:

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

(
  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),
          ),
        ),
      },
    ),
  },
)

[MrGVSV]

Looks good to me!

This is actually something I originally wanted to add as part of the FromReflect ergonomics RFC/implementation (for the reflection deserializer, not necessarily the one for bevy_scene), so hopefully we can solve this problem at the root as well. I'm also hoping #13432 can help here eventually too.

[alice-i-cecile]

Could you provide an example before and after scene for the migration guide? We don't have great tools here yet, but we should be able to do better.

[alice-i-cecile]

FYI, clippy is failing: https://github.com/bevyengine/bevy/actions/runs/10863643853/job/30148024354?pr=15174 I'll merge this once that's passing though.

[yrns]

Could you provide an example before and after scene for the migration guide? We don't have great tools here yet, but we should be able to do better.

Updated. Interesting that viewport_origin (a Vec2) implements Serialize, but still serializes with reflect because it does not impl ReflectSerialize. Whereas Rect does. Should ReflectSerialize (and ReflectDeserialize) be implemented for all foreign Reflect types that also impl Serialize and Deserialize?

[alice-i-cecile]

Should ReflectSerialize (and ReflectDeserialize) be implemented for all foreign Reflect types that also impl Serialize and Deserialize?

I believe so yes.

[yrns]

Without a rigorous way to check every type, I just did a text scan and found only the glam types were missing. The bug is not as bad as I thought because, as far as I can tell, only the glam types have a differing serialization format (they serialize as sequences not maps). If the format is the same for reflect/native serialization the bug should not occur. I updated the migration guide which includes viewport_origin looking right.

[alice-i-cecile]

Great, thank you!