Reflect and (de)serialize resources in scenes

[Davier]

Objective

Fixes #3576.
Enables editors/inspectors to show and edit resources.

Solution

• change the scene format to include resources:

(
  resources: [
    // List of resources here, same fomat as an entity's list of components
  ],
  entities: [
    // Previous scene format here
  ],
)

• enable reflection on a resource type (by adding ReflectResource)

#[derive(Reflect)]
#[reflect(Resource)]
struct MyResourse;

• add a vec of reflected resources to DynamicScenes and implement their (de)serialization
• adapt Scenes and DynamicScenes to actually import/export reflected resources from the world

Remaining work

• write some tests
• modify the scene example to show resources too
• gather feedback on a list of bevy's resources that should be reflected

Notes

I ignored non send resources as I don't think they should be loaded from scenes, but it may make sense to reflect them so that they are accessible to editors/inspectors.

[alice-i-cecile]

This is looking very good. That was easier than I expected, and should be incredibly useful. If you can comfortably add a method / pattern to access resource / non-resource archetypes in a less fragile way that would be lovely, but I can live without it.

I'll work on compiling a list of resources to reflect now.

[alice-i-cecile]

#[(derive(Resource)) would improve both finding and reflecting resources dramatically. Well for now, manual search it is.

Reflect and Serialize

• add trait bounded methods to State: this is critical for a lot of game-saving use cases
• add trait bounded methods to Events
• FrameTimeDiagnosticsState and LogDiagnosticsState: might be useful?
• LogSettings: probably useful
• Time: this may be a nuisance due to the Duration and Instant fields, but is very very useful to have
• SpriteSettings
• Msaa
• AmbientLight
• WgpuOptions
• ScheduleRunnerSettings
• WinitConfig
• ClearColor
• DefaultTaskPoolOptions
• ReportExecutionOrderAmbiguities
• WindowDescriptor
• AssetServerSettings
• FixedTimestepState
• Input<T>: serialization could be handy for networking
• RenderGraph
• PipelineCompiler
• RenderResourceBinding
• AssetRenderResourceBindings
• ActiveCameras
• WireframeConfig
• DefaultTextPipeline
• SceneSpawner
• FlexSurface

Just Reflect

• Assets<T>: I don't think we can store this state on disk reasonably
• AssetServer: ditto
• EntityLabels: Entity identifiers are opaque
• IOTaskPool, AsyncComputeTaskPool, ComputeTaskPool: can't be stored on disk'
• TypeRegistryArc: I think that storing this on disk is a bit too meta?
• Audio<T>: I think this isn't going to be useful without a loaded asset?
• Windows and WinitWindows: not meaningful to store

Nothing

• CiTestingConfig: maybe?? I don't mind having reflect on this

[alice-i-cecile]

Overall, it looks like Reflect should probably just be a trait bound for resources: there's virtually always a valid use case for it.

Serialize is likely not appropriate for resources that are about working with assets or physical hardware (or when dealing with Entity identifiers), but probably fitting for everything else.

[Davier]

Overall, it looks like Reflect should probably just be a trait bound for resources: there's virtually always a valid use case for it.

It's possible to use bevy_ecs without bevy_reflect, so we can't do that. (Also, I've be told previously that reflection should be opt-in, not opt-out or mandatory.)

Just Reflect

Reflection and serialization are closely related, I'm not sure we can have the first without the second with the current derive macro (as discussed on discord, but I'm repeating it here for others). I'll investigate.

Thank you @alice-i-cecile for the research! I'll start by reflecting the simple "config"-only resources, but I might leave the ones with actual content to their own PRs.

[jakobhellermann]

For motivation, these changes (only ReflectResource, not the scene stuff) make this possible:

All that's needed is

#[derive(Reflect, Default)]
#[reflect(Resource)]
struct Msaa {

app.register_type::<Msaa>()

[Davier]

I did a pass over every resource, and selected those that only contain configuration (i.e. no state), and that can be changed after startup: Msaa, WireframeConfig, AmbientLight, DirectionalLightShadowMap, PointLightShadowMap, ClearColor, and GamepadSettings.

I have not yet reflected GamepadSettings because I don't know if the "serialize" feature is still relevant or should be removed.

Reflecting ClearColor and GamepadSettings adds new dependencies on bevy_reflect, should they be optional dependencies?

I'm not sure these are useful to reflect: CiTestingConfig and ReportExecutionOrderAmbiguities.

These resources are only read during startup: ScheduleRunnerSettings, AssetServerSettings, WgpuOptions, WinitConfig, DefaultTaskPoolOptions, WindowDescriptor, and LogSettings. Should we reflect them even though modifications are without any effect? It could be useful but also misleading.

These resources contain state, so I think they deserve discussion on what part should or should not be reflected in their own PRs: State<T>, Events<T>, LogDiagnosticsState, IoTaskPool, AsyncComputeTaskPool, ComputeTaskPool, ActiveCamera, DefaultTextPipeline, SceneSpawner, FlexSurface, Windows, WinitWindows, TypeRegistryArc, Audio<AudioSource>, Diagnostics, Input<T>, (for KeyCode, MouseButton, GamepadButton), Touches, Axis<T>, for GamepadAxis, GamepadButton), Gamepads, VisiblePointLights, FrameTimeDiagnosticsState, Time, FixedTimesteps,

[jakobhellermann]

Reflecting ClearColor and GamepadSettings adds new dependencies on bevy_reflect, should they be optional dependencies?

IMO they can just depend on it unconditionally, just like bevy_asset, bevy_gltf etc. do. You'll have it in your dependency tree anyways.

[jakobhellermann]

Currently this PR will put every reflectable resource into the scene, right? I don't think that this is a very good policy, as there are a lot of resources like Input<KeyCode> that you don't want in your serialized scene.

What do you think about splitting this PR into one for ReflectResource and another one for allowing (de-)serialization of resources in scenes?
There former is fully implemented and as far as I can tell ready to merge, while the latter needs more thought put into it.

[Davier]

Currently this PR will put every reflectable resource into the scene, right? I don't think that this is a very good policy, as there are a lot of resources like Input<KeyCode> that you don't want in your serialized scene.

You are talking about using DynamicScene::from_world(world: &World, type_registry: &TypeRegistryArc) right? Indeed, that will copy all components and resources that are registered in the given type_registry (i.e. not the world's type_registry). I think it would be surprising if it didn't.
I do agree that you usually don't want to save all the reflected resources, but I think you also don't want to save all the reflected components. I expect people to make a custom type_registry, and only register the components and resources they want to be included in the scene (so the default is not all of them, but none of them).
Do you think it would be enough to document it properly, and make an example that explicitly shows how to use a custom type_registry?

For your example of Input<KeyCode>, I think there are cases when it would makes sense to serialize it. For instance, if you automatically save a game state, you might want to save what the user was pressing to keep things consistent.

What do you think about splitting this PR into one for ReflectResource and another one for allowing (de-)serialization of resources in scenes? There former is fully implemented and as far as I can tell ready to merge, while the latter needs more thought put into it.

Both are implemented, I just need some time to rebase, and write some examples and tests. I can split the PR in two if we don't have a consensus on the second part 🙂 (I'll probably be to busy to do it until the end of march though)

[alice-i-cecile]

I think it would be surprising if it didn't.

Agreed.

Do you think it would be enough to document it properly, and make an example that explicitly shows how to use a custom type_registry?

IMO yes.

I can split the PR in two if we don't have a consensus on the second part

I think this is a good idea regardless; the functionality is related but distinct, and optimizing for merge-ability is a good call.

[alice-i-cecile]

@Davier if you have the chance, can you split out the reflection changes from the "add resources to scenes"? The former is uncontroversial, but the latter needs more consideration.

[jakobhellermann]

ReflectResource might actually have become obsolete with #4447 and #4475, since instead of

let reflect_resource = type_registry.get_type_data::<ReflectResource>(type_id)?;
let resource: &dyn Reflect = reflect_resource.reflect_resource(world)?;
reflect_resource.insert_resource(world, some_resource_value /* dyn Reflect */);
reflect_resource.remove_resource(world);

you can do

let reflect_from_ptr = type_registry.get_type_data::<ReflectFromPtr>(type_id)?;

let component_id = world.get_id(type_id)?;
let resource: Ptr<'_> = world.get_resource_by_id(component_id);
let resource: &dyn Reflect = unsafe { reflect_from_ptr.as_reflect(resource) }; // SAFE: resource is of the type that the `ReflectFromPtr` was made for
world.insert_resource_by_id(component_id, some_resource_value /* OwnedPtr */);
world.remove_resource_by_id(component_id);

[alice-i-cecile]

Very cool, but I think there's a lot of value in having a fully safe API for a relatively common task.

[alice-i-cecile]

Closing in favor of #5175 as it is much less controversial. We can tackle "should resources be stored in scenes" when we tackle more comprehensive design reviews of how scenes and prefabs work.