Create generic RON format saving and loading for both reflected data and strongly-typed data.

[andriyDev]

Objective

• We currently have no generic "box" to read and write asset data as.
  • For context, Godot has its tres format that can store any Resource type. In the editor, users just click a "new resource" button, and that resources gets saved as a tres file that they can load in their game.
  • Unreal has a similar DataAsset base class to store arbitrary data.

Solution

• Create TypedRonLoader and TypedRonSaver.
  • These are for cases where you know what type you want to store. This requires that for each type that you want to load, you need to add a new loader for it (ideally with a unique file extension).
  • This is basically equivalent to using ron::to_string and writing the result to a file.
• Create RonLoader and RonSaver.
  • These all you to save and load any reflected asset type. You just need to reflect the Asset and you're off to the races.
  • This is similar to Godot's tres files.
  • This loader/saver also uses the HandleSerializeProcessor which means asset handles will directly work with it!

One big caveat: when using the RonLoader, we need to load from whatever.ron#Typed. Since AssetLoaders currently need to know their asset type (since we select loaders by the asset type we want to load, see #25664 for more), we need to "erase" the type that we're trying to load. In this case, we're using a subasset to do that type erasure. @greeble-dev has a clever idea to avoid this, but I'd rather not block on that (and also a migration of just "drop the #Typed" doesn't seem too big a deal).

This problem does not affect the TypedRonLoader, though I don't like recommending this one because A) it's not self-describing, meaning you could just see a .ron file and have no idea what type it holds, and B) it doesn't support handles.

Testing

• Added tests to show that we can save and then load the resulting file.
• Added an example (and displaced the previous custom_asset example into custom_asset_loader).
  • This basically shows the Godot workflow: we just add the RonLoader (only need to do this once for the whole project), and then we can immediately load our asset type from disk!

[Diddykonga]

I lean towards expanding BSN to cover this rather than introducing Ron. Adding a brand new path feels like extra complexity and choices for little gain, when we could invest that effort into improving BSN instead. Discord context here:
https://discord.com/channels/691052431525675048/692572690833473578/1548752650580000808

[github-actions]

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

[mgi388]

@andriyDev what does this mean? Do you have an example?

This loader/saver also uses the HandleSerializeProcessor which means asset handles will directly work with it!

[andriyDev]

@mgi388 See https://github.com/bevyengine/bevy/pull/25770/changes#diff-49716114e0a6c03b90246e68f8adffe922fbc1d1712ce7925785f9ba7a8c9a13R668 (line 668). This allows you to save and load asset handles through the RonLoader/RonSaver. That is not true for the TypedRonLoader/TypedRonSaver.

[mgi388]

@andriyDev what does this mean? Do you have an example?

This loader/saver also uses the HandleSerializeProcessor which means asset handles will directly work with it!

I thought it might be this. Does it work with any path, e.g., when the asset path is an image? Asking because I have a bunch of assets with custom loaders and they have fields like “sprite_path: foo.png” which I then convert to a Handle inside the loader.

[andriyDev]

@mgi388 Yes it works for any handle type! It'll just call asset_server.load(path).

[mgi388]

@mgi388 See https://github.com/bevyengine/bevy/pull/25770/changes#diff-49716114e0a6c03b90246e68f8adffe922fbc1d1712ce7925785f9ba7a8c9a13R668 (line 668). This allows you to save and load asset handles through the RonLoader/RonSaver. That is not true for the TypedRonLoader/TypedRonSaver.

@andriyDev Neat. Yeah I could probably use it to replace this kind of custom asset loader I have:

(
    book_sprite_sheet_path: "path/to/something.png",
    inventory_sprite_sheet_path: "path/to/something.png",
    world_sprite_sheet_path: "path/to/something.png",
    items: {...}
)

Details

```rust #[derive(Asset, ...)] // ... pub struct MagicItemsAsset { book_sprite_sheet_path: String, inventory_sprite_sheet_path: String, world_sprite_sheet_path: String, #[serde(skip)] pub book_sprite_sheet: Handle, #[serde(skip)] pub inventory_sprite_sheet: Handle, #[serde(skip)] pub world_sprite_sheet: Handle, #[serde(default)] pub items: HashMap, }

impl AssetLoader for MagicItemsAssetLoader {
type Asset = MagicItemsAsset;
type Settings = ();
type Error = MagicItemsAssetLoaderError;
async fn load(
&self,
reader: &mut dyn Reader,
settings: &Self::Settings,
load_context: &mut LoadContext<'>,
) -> Result<Self::Asset, Self::Error> {
let mut bytes = Vec::new();
reader.read_to_end(&mut bytes).await?;

    let mut magic_items = ron::de::from_bytes::<MagicItemsAsset>(&bytes)?;

    magic_items.book_sprite_sheet =
        load_context.load(magic_items.book_sprite_sheet_path.clone());

    magic_items.inventory_sprite_sheet =
        load_context.load(magic_items.inventory_sprite_sheet_path.clone());

    magic_items.world_sprite_sheet =
        load_context.load(magic_items.world_sprite_sheet_path.clone());

    for item in magic_items.items.values_mut() {
        if let MagicItemKind::SpellBook {
            inventory_sprite_sheet_path,
            inventory_sprite_sheet,
            ..
        } = &mut item.kind
        {
            *inventory_sprite_sheet = load_context.load(inventory_sprite_sheet_path.clone());
        }
    }

    Ok(magic_items)
}

fn extensions(&self) -> &[&str] {
    &["magic-items.ron"]
}

}

</details>

[mgi388]

At first I didn't really understand what this whole paragraph is trying to tell me.

Then (I think!) I realized that you're actually saying that you can make your RON bytes look like this:

( 
    first_field: "abc",
    second_field: 10,
    handle_field: Path("some_other_path.gltf")
)

(as long as you use "a unique extension, setting an explicit loader in the meta file, or always loading
with the correct T when calling AssetServer::load")

If that is the case, I think it would be worth showing both forms (in full) so that readers can compare. I currently derive serde on my existing types, so I'd be switching to the typed form so that I don't need to include "my_crate::MyData" things in my RON bytes, or include #Typed.

[andriyDev]

One thing to note is that the handle stuff only works with the reflected form, not the TypedRonLoader. That's part of why I think we should prioritize the RonLoader and why I don't know if we should include an example with the typed form.

[mgi388]

So what would:

( 
    first_field: "abc",
    second_field: 10,
    handle_field: Path("some_other_path.gltf")
)

do for handle_field?

Either way, I guess the notes should say that "handle resolution won't work with the TypedRonLoader".

[andriyDev]

handle_field doesn't impl Serialize or Deserialize, so you just won't be able to add the TypedRonLoader in the first place.

Added an example to the release notes, with a caveat to show that handles don't work.

[viridia]

I'm in favor of this; there are use cases that I can envision that would be more appropriate for RON than BSN. BSN can only serialize data structures built out of entities and components, and RON files would be used for complex structures that don't meet that criteria.

[stuartparmenter]

Excited for this, it will make my life easier! A few error checking/edge cases I think we should fix but otherwise looks good to me!

[greeble-dev]

Looks good, and I'm personally on-board with the idea that a generic RON loader/saver is a good feature even if .bsn happens.

As @andriyDev mentioned in the PR, I do have an alternative implementation that avoids the need for #Typed. But I wasn't planning on PRing that until after assets-as-entities lands because the changes are bigger (although it doesn't depend on assets-as-entities, I'm just avoiding conflicts). More details on Discord: https://discord.com/channels/691052431525675048/749332104487108618/1547320058370588752 . If this PR lands then I'll attempt to unify them.

[greeble-dev]

I'm personally not a fan of the unwrap here, even if it requires malicious behavior - I think cases like that should still have an error. But would still approve the PR with the unwrap since I might be an outlier there.

[greeble-dev]

Suggested change

	pub struct RonLoader {
	pub struct ReflectRonLoader {

I'd consider renaming RonLoader and TypedRonLoader to ReflectRonLoader and SerdeRonLoader. I feel like "one loader uses reflection and the other loader uses serde" is the biggest thing that users need to understand when choosing one, so best to put it right in the name.

[andriyDev]

Mentioned in the other comment, but I'll reiterate here: I think the RonLoader should be the default, so naming it more generically is better. Naming them ReflectRonLoader and SerdeRonLoader will make the question more muddy since now they are "on equal footing". A user might just reflexively pick the SerdeRonLoader because they know serde and not reflection, and then be left with IMO a subpar experience.

[andriyDev]

Sorry, just to drive this point home. In the "Bevy has an editor" future, I expect that ~all assets will impl Reflect and so the reflection-based loader will be a no-brainer. At which point calling it ReflectRonLoader will be kinda pointless, and a little confusing.

[mgi388]

Some thoughts on this. Some quoted comments extracted from other threads.

Re:

I feel pretty strongly that we shouldn't be advertising the typed loader very much

If this is the case, it feels more like TypedRonLoader should just be left as an example, or remain external to Bevy. "Not advertising" it feels a bit like Bevy conceding to storing some dead code that doesn't quite work.

Re:

and then accidentally shooting themselves in the foot wrt not supporting handles

I wonder if this itself is a strong indication that either the names and/or API isn't right here. These two things appear to be the same, except one has a bad footgun. As an anecdote: That my first impression when I reviewed the first version of this. Further, the serde-based one is the one I personally would prefer to use because a) of how the actual RON file on disk looks nicer/simpler (doesn't require Rust crate name in an asset) and b) because it means I can keep my Rust asset structs reflects optional and so out of release builds. These are just personal reasons, I don't expect everyone to share these.

At a glance, I like the names ReflectRonLoader and SerdeRonLoader better. As a consumer/user, I don't quite get the meaning of the "typed" in TypedRonLoader. I can infer it's something to do with Bevy typed/untyped things, but it sounds wrong to me because both the RON on disk bytes and the Rust struct that each of these maps to looks no more or less "typed" than the other.

I don't think it makes sense for me to leave an approval either way, so I won't, though I do hope the thoughts/opinions here help to shape the solution either way, but I have no strong opinions on whether or how this lands (I'll either use it if it suits my project, or I won't if it doesn't suit).

[andriyDev]

I think TypedRonLoader still has a place for some use-cases, so I'd rather support those. Having to continue to rely on an external crate for this seems quite sad (or needing to migrate all their assets to the RonLoader format).

doesn't require Rust crate name in an asset

This is already fixed by #25773 ! So it's already much simpler.

I don't quite get the meaning of the "typed" in TypedRonLoader

IMO this is almost "working as intended". The confusion kinda discourages new users from depending on it while allowing "advanced" users to take advantage of its features.

I also think "Typed" aligns well with bevy_asset. Like for example a Handle has a generic argument, whereas an UntypedHandle does not (implying that the opposite must be typed).

[greeble-dev]

What if this PR added a RonLoaderPlugin that registers RonLoader? That's arguably a bit more ergonomic and nudges the user towards the reflect loader by default.

-    .init_asset_loader::<RonLoader>()
+    .add_plugins(RunLoaderPlugin)

On RonLoader vs ReflectRonLoader, I agree that RonLoader is better if we're encouraging it as the default. Although if it's added by a plugin then the user never sees the type name...

On TypedRonLoader vs SerdeRonLoader, I still think the latter is better. I disagree on using confusion to push users towards the reflect loader - the fact the the reflect loader has to be registered once for all types should be enough to sell users on the idea. SerdeRonLoader is honest about what it does.

(Side note: I think UntypedHandle is a misleading name too - it does have a type! I want to change it to ErasedHandle, but that's another PR waiting on assets-as-entities to avoid conflicts...)

[greeble-dev]

Clicking approve as I think the PR is basically there. I have a few disagreements about names (#25770 (comment)) but they're not blockers.