Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(De) serialize resources in scenes #6846

Merged
merged 14 commits into from
Mar 20, 2023
Merged

(De) serialize resources in scenes #6846

merged 14 commits into from
Mar 20, 2023

Conversation

Carbonhell
Copy link
Contributor

@Carbonhell Carbonhell commented Dec 4, 2022

Objective

Co-Authored-By: davier bricedavier@gmail.com
Fixes #3576.
Adds a resources field in scene serialization data to allow de/serializing resources that have reflection enabled.

Solution

Most of this code is taken from a previous closed PR: #3580. Most of the credit goes to @Davier , what I did was mostly getting it to work on the latest main branch of Bevy, along with adding a few asserts in the currently existing tests to be sure everything is working properly.

This PR changes the scene format to include resources in this way:

(
  resources: {
    // List of resources here, keyed by resource type name.
  },
  entities: [
    // Previous scene format here
  ],
)

An example taken from the tests:

(
  resources: {
    "bevy_scene::serde::tests::MyResource": (
      foo: 123,
    ),
  },
  entities: {
    // Previous scene format here
  },
)

For this, a resources fields has been added on the DynamicScene and the DynamicSceneBuilder structs. The latter now also has a method named extract_resources to properly extract the existing resources registered in the local type registry, in a similar way to extract_entities.


Changelog

Added: Reflect resources registered in the type registry used by dynamic scenes will now be properly de/serialized in scene data.

Migration Guide

Since the scene format has been changed, the user may not be able to use scenes saved prior to this PR due to the resources scene field being missing. To preserve backwards compatibility, I will try to make the resources fully optional so that old scenes can be loaded without issue.

TODOs

  • I may have to update a few doc blocks still referring to dynamic scenes as mere container of entities, since they now include resources as well.
  • I want to make the resources key optional, as specified in the Migration Guide, so that old scenes will be compatible with this change. Since this would only be trivial for ron format, I think it might be better to consider it in a separate PR/discussion to figure out if it could be done for binary serialization too.
  • I suppose it might be a good idea to add a resources in the scene example so that users will quickly notice they can serialize resources just like entities.

@MrGVSV MrGVSV added C-Feature A new feature, making something new possible A-Scenes Serialized ECS data stored on the disk labels Dec 5, 2022
crates/bevy_scene/src/dynamic_scene_builder.rs Outdated Show resolved Hide resolved
crates/bevy_scene/src/dynamic_scene_builder.rs Outdated Show resolved Hide resolved
crates/bevy_scene/src/dynamic_scene_builder.rs Outdated Show resolved Hide resolved
crates/bevy_scene/src/scene.rs Outdated Show resolved Hide resolved
crates/bevy_scene/src/scene.rs Outdated Show resolved Hide resolved
crates/bevy_scene/src/scene.rs Outdated Show resolved Hide resolved
crates/bevy_scene/src/serde.rs Outdated Show resolved Hide resolved
crates/bevy_scene/src/serde.rs Show resolved Hide resolved
@Carbonhell Carbonhell marked this pull request as draft December 5, 2022 11:40
refactor: apply review suggestions
feat: add example in scene.rs for resource de/serialization
# Conflicts:
#	crates/bevy_scene/src/dynamic_scene.rs
@Carbonhell Carbonhell marked this pull request as ready for review December 6, 2022 22:49
crates/bevy_scene/src/dynamic_scene.rs Outdated Show resolved Hide resolved
crates/bevy_scene/src/dynamic_scene.rs Outdated Show resolved Hide resolved
crates/bevy_scene/src/dynamic_scene_builder.rs Outdated Show resolved Hide resolved
crates/bevy_scene/src/dynamic_scene_builder.rs Outdated Show resolved Hide resolved
crates/bevy_scene/src/serde.rs Outdated Show resolved Hide resolved
crates/bevy_scene/src/serde.rs Outdated Show resolved Hide resolved
refactor: ReflectArraySerializer -> SceneMapSerializer, ReflectDeserializer -> SceneMapDeserializer, ReflectVisitor -> SceneMapVisitor
Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few remaining comments, but overall LGTM!

crates/bevy_scene/src/dynamic_scene.rs Outdated Show resolved Hide resolved
crates/bevy_scene/src/serde.rs Outdated Show resolved Hide resolved
crates/bevy_scene/src/serde.rs Outdated Show resolved Hide resolved
@mockersf
Copy link
Member

I'm not sure writing a scene to a world should always write the resources. Unlike entities, this would overwrite existing resources. This seems like a desirable behaviour sometimes but not always.

@MrGVSV
Copy link
Member

MrGVSV commented Dec 11, 2022

I'm not sure writing a scene to a world should always write the resources. Unlike entities, this would overwrite existing resources. This seems like a desirable behaviour sometimes but not always.

Theoretically this should only overwrite the resources selected by the user in their passed AppTypeRegistry, right? Is there a reason they would choose to serialize a resource but not have it be deserialized? I guess if you want to write just the entities to a particular world, but I'm not sure 🤔

@mockersf
Copy link
Member

mockersf commented Dec 11, 2022

Yes, but that would mean having multiple registries depending on what you would want to do. It's easier to think with one registry per scene.

I am not sure having the same behavior for entities and resources makes sense, writing the entities only create new ones, writing resources replace them. Having that being implicit doesn't feel nice, I may be wrong.

A reason to not want to rewrite them would be for a reusable scene that can need a few resources on its first spawn, but can reuse them after

@MrGVSV
Copy link
Member

MrGVSV commented Dec 13, 2022

A reason to not want to rewrite them would be for a reusable scene that can need a few resources on its first spawn, but can reuse them after

Yeah this is what I think this is really needed for. For scenes that are more like save files, this really helps restore the full world state (well, more or less).

But I think this type of scene will generally be treated much differently than a transient one (e.g. networking) or ones that are written to a world multiple times (e.g. prefabs). So in these cases, we can expect them to rely on different filters. And this can be made a lot nicer with something like #6793 where we have an actual filter type rather than multiple registries.

So imo this PR would be really nice to have.

@Carbonhell
Copy link
Contributor Author

I'm not sure writing a scene to a world should always write the resources. Unlike entities, this would overwrite existing resources. This seems like a desirable behaviour sometimes but not always.

Sorry for the late answer. I understand this might not always be the desired behaviour, but I think it's an acceptable default behaviour. The reason for this is that resources represent globally unique data in a Bevy app, therefore it seems natural to me that deserialization implies overwriting the current values with the loaded ones. The only other ideas that I can think of would be either manually handling deserializing a resource, or having some sort of scene-scoped resource.

  1. The manual deserialization doesn't seems to be doable right now in an easy way, if I understood correctly. There needs to be some sort of hook prior to writing a dynamic scene to a world, to allow the user to be able to access both the old resource data and the loaded one, to decide what the final data should be. Since DynamicScene.write_to_world_with has a mutable ref to World, this should be doable I think.
  2. Scene scoped resources sound complex to do and I'm honestly not sure if it would be worth it and what changes it would imply for bevy_ecs.

As @MrGVSV stated, using a specific AppTypeRegistry is the most obvious answer to circumvent this problem. I think this issue is similar to the one I had in my project, where using the world AppTypeRegistry caused the resulting serialized scene data to be bloated with components which the user may want to omit to, instead, recreate them on load manually (like in my case for my app). With my limited use of registries, I also think it should be unique and creating one for each use case within a single app feels confusing. In my opinion, the registry should specify which reflected types are registered, not which ones the user might be interested in de/serializing at a specific time.
For this reason. I believe moving the latter functionality to a filter type like @MrGVSV suggested in his draft PR (#6793) is the best solution for both problems (component serialization bloat and specifying which resources to de/serialize). Maybe manual deserialization for resources could also be implemented so that the user can implement some sort of trait (or reuse something like FromReflect?) to specify exactly what data a loaded resource should have.

# Conflicts:
#	crates/bevy_scene/src/dynamic_scene_builder.rs
…source

feat: specific test for DynamicSceneBuilder extract_resource
@Carbonhell
Copy link
Contributor Author

I resolved the conflicts, added a test in the dynamic_scene_builder.rs to test the extract_method in detail and removed the world argument in favor of self.original_world to be consistent with the extract_entities method. The PR should be ready, I believe.

Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I agree with François, implicitly adding resources seems like a potential footgun. But with the current bevy tools, there is simply no way of not doing it this way.

The ability to load resources from scenes is necessary first before being capable of developing tools to filter resources when loading them, if we wait the tools before adding loading, then the snake bites the tail of the chicken's egg and we will wait forever.

crates/bevy_scene/src/dynamic_scene_builder.rs Outdated Show resolved Hide resolved
crates/bevy_scene/src/serde.rs Outdated Show resolved Hide resolved
crates/bevy_scene/src/dynamic_scene.rs Show resolved Hide resolved
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Controversial There is active debate or serious implications around merging this PR labels Jan 8, 2023
# Conflicts:
#	crates/bevy_scene/src/dynamic_scene_builder.rs
#	crates/bevy_scene/src/serde.rs
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation is great and I agree with the sentiment expressed here: this is a good first step into the space. I strongly suspect we will need more involved and flexible resource workflows in our scenes (ex: we need to support inline assets, which can't just overwrite the current asset state), but this is a reasonable first step.

@cart cart enabled auto-merge March 20, 2023 21:14
@cart cart added this pull request to the merge queue Mar 20, 2023
@cart cart merged commit 7b38de0 into bevyengine:main Mar 20, 2023
@MrGVSV MrGVSV mentioned this pull request Mar 28, 2023
10 tasks
@Selene-Amanita Selene-Amanita added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Scenes Serialized ECS data stored on the disk C-Feature A new feature, making something new possible M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Controversial There is active debate or serious implications around merging this PR
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Use scenes to serialize and deserialize resources
7 participants