Fix #11111 by making asset settings part of AssetPath

[greeble-dev]

Objective

Fix #11111.

Why A Draft?

The PR is basically working and tested, but parts are unfinished and the design is likely to be controversial. I'm hoping to get feedback before I finish it up. The remaining work is mostly error handling and documentation.

The main issues will be that custom settings are serialized for hashing/equality, and asset paths with settings can't be round-tripped via text.

Background

Bevy identifies assets at rest with AssetPath, and assets in memory with Handle. Assets are loaded by calling AssetServer::load(AssetPath) -> Handle. The (unstated?) contract is that multiple calls to load with the same AssetPath will return the same Handle.

This causes surprising behaviour for users who call AssetServer::load_with_settings - a load that takes an AssetPath and a closure that customises the asset loader's settings. If an asset with the same AssetPath is already in memory then load_with_settings returns that asset's Handle and doesn't run the loader - the settings are effectively ignored. Or the reverse can happen - a plain load can return an asset that was previously customised by load_with_settings.

This behaviour can occur even if the user doesn't directly call load_with_settings. One example is the glTF importer, which calls load_with_settings on external texture files to apply sampler settings. If the user loads two different glTFs that share the same texture file but with different settings, then one of the glTFs wins and the other is broken.

I've previously considered a few simpler solutions, but decided they weren't viable.

Solution

This PR moves the loader settings into AssetPath. Now they're part of the asset's identity, so different settings will result in a different Handle.

pub struct AssetPath<'a> {
     source: AssetSourceId<'a>,
     path: CowArc<'a, Path>,
     label: Option<CowArc<'a, str>>,
+    settings: Option<Arc<ErasedSettings>>,
 }

pub struct ErasedSettings {
    value: Box<dyn Settings>,
    id: ErasedSettingsId,
}

impl PartialEq for ErasedSettings {
    fn eq(&self, other: &Self) -> bool {
        self.id == other.id
    }
}

AssetServer::load_with_settings is preserved for backwards compatibility, but can be replaced by AssetPath::with_settings.

 let path = AssetPath::from("path/to/image.ktx2");
-let settings = |s: &mut ImageLoaderSettings| s.is_srgb = true;
-let handle = asset_server.load_with_settings(path, settings);
+let settings = ImageLoaderSettings { is_srgb: true, ..default() };
+let handle = asset_server.load(path.with_settings(settings));

Problems

💀💀💀 Equality and hashing of settings is done by serializing to RON 💀💀💀.

• That's... not great, to put it mildly. But it's the least worst solution I could come up with.
• We need AssetPath to contain an erased settings value and implement Hash and Eq.
  • Serializing + type id gives us something to hash, and equality is done by comparing hashes.
  • Settings are already required to implement Serialize in practice (for meta file support), so this is backwards compatible.
  • And meta files already use RON serialization, so we're not adding dependencies.
• The alternative is to make settings implement Hash and Eq themselves.
  • This is painful and not backwards compatible.
• See the performance section for some more notes.

Asset paths are no longer guaranteed to be round-trippable via text.

• Previously, any AssetPath formatted by Display could be parsed by AssetPath::parse.
• This is no longer possible if the path has settings.
• Maybe that's a reasonable sacrifice.
• There's also a spicy alternative - the text path could include the settings as RON.

Asset identity has some asterisks.

• There's cases where the same settings can produce asset values that are equal but duplicated in memory.
  • 1: path versus path.with_settings(SomeSettings::default())
  • 2: The asset has a meta and the custom settings are the same as the meta's settings.
• In practice this might be acceptable. The asset values are still correct, just duplicated.
• But it remains a potential footgun, particularly if the user intends to mutate the asset.

load_with_settings is not fully backwards compatible.

• Previously, the settings closure allowed partial override of settings provided by a meta file.
• Now, the settings in the meta file are completely overridden.
• This is arguably necessary to get asset identity right without extra complication.
  • If partial meta overrides are part of the identity then AssetServer::load can't immediately return a Handle - it has to wait until the meta file is loaded (a la load_untyped).
• I suspect that few users depend on partially overriding meta settings, but that's just a hunch.
• load_with_settings is fully backwards compatible if the asset doesn't have a meta file.

Fixing asset identity can reveal bugs.

• Previously, doing a load_with_settings and load on the same asset path would return the same Handle.
• Users may have deliberately or accidentally relied on this behaviour.
• The glTF loader is one example (see later section).
• Also the alter_sprites and alter_mesh examples.
• I don't see a way to avoid this.

Performance.

• I haven't done any benchmarks yet.
• I'd expect a negligible decrease in performance when there are no custom settings.
  • AssetPath equality/hashing just needs to do a bit more to check the None settings.
• Custom settings will be more expensive.
  • Maybe this is acceptable.
    • Anyone using runtime settings has already made a choice to prefer convenience/flexibility over performance.
  • The data isn't much different - a boxed closure has become a boxed value.
    • Memory size is likely bigger as it's a full value per Handle rather than a function pointer.
  • The value has to be serialized and hashed for comparison.
    • This happens synchronously inside AssetServer::load - it must be done immediately to return the right Handle.
    • Currently allocates a string, although maybe that could be replaced with a small vec or more direct hashing.
    • Or we bite the bullet and settings have to implement Hash + Eq.
  • The settings are in an Arc, while other AssetPath members are a CowArc.
    • This will cost performance for settings that could be 'static or borrowed.
    • I tried using CowArc but ran into mysterious errors about traits not being satisfied. Maybe this can be fixed.
• The size of AssetPath increases by 8 bytes (72 -> 80).
  • If the Arc became CowArc then it would be +24 bytes.
  • (Side note: Option<CowArc> being 24 bytes is unfortunate? 15 of those bytes are unused)

Lack of test coverage.

• I've tested:
  • CI.
  • PRs Add test for mipmap filters #19095 and Add test that repros #11111 (different loader settings produce same asset) #19094.
  • All the examples that call load_with_settings, and various examples that load glTFs or do asset work.
    • Skipped alter_mesh due to Mesh not showing(or updating) in alter_mesh example #18967.
• But I still don't feel too confident about the changes - these tests only cover so much.
• In particular, I don't have a test for nested glTF files - I believe some users have implemented this and it's a complex case.

glTF importer issues

• The glTF importer relied on a load_with_settings and a later load returning the same handle.
• This PR solves the issue by creating AssetPath once with the right settings, which adds some annoying plumbing.
• Could be split into a prerequisite PR.

Removing MetaTransform is debatable.

• I chose to remove MetaTransform because it seems incompatible with asset identity.
• Removing it should be reasonably safe as it was only used to implement custom settings, and users can't add their own transforms.
• But maybe there's a good reason to keep it that I'm not aware of.
• I'd be fine with putting it back in, even if that's just to make the PR simpler.

Maybe this PR is too big.

• It could be split into:
  • 1: Restructure glTF to always use the same AssetPath.
  • 2: Update examples that depend on load and load_with_settings equivalence.
  • 3: Add asset settings to AssetPath.
  • 4: (Optionally) remove MetaTransform.
  • 5: (Optionally) update all examples to prefer AssetPath::with_settings over AssetServer::load_with_settings.

Alternatives

So this PR has a bunch of problems. What are the alternatives?

First, lets define the asset system by three constraints:

1. AssetPath and Handle are both ways of identifying an asset value at rest or in memory. A Handle can be mapped to exactly one AssetPath at runtime, and vice-versa.
2. Asset paths can be round-tripped via text, and that text is reasonably simple and human writeable.
3. Asset paths reference the source asset - the file from which the asset value is derived, even if it goes through sub-asset splitting and loader settings.

The current asset system sacrifices constraint 1, so paths and handles don't properly identify an asset value. This is untenable - arguably constraint 1 is the founding principle of any asset system.

This PR sacrifices constraint 2 - asset paths now awkwardly include the settings (either as a string or hash).

I don't think there's a way for the engine to satisfy constraints 2 and 3 and also support non-trivial asset processing. Maybe that's a valid strategy - there is precedent for engines that push all complex asset processing onto the user - but I'm assuming it's not the strategy Bevy has chosen.

That leaves one alternative - what happens if we sacrifice constraint 3?

Meta All The Things

What if apps can load meta files directly, and meta files are more flexible in what they represent?

• AssetPath stays the same and doesn't need to contain settings.
• Loading an asset does not automatically try to find a matching meta file.
  • If I load foo.gltf then I always get a Gltf with the default loader settings.
  • (This change is maybe optional, and might be avoided for backwards compatibility)
• Instead, asset paths can reference meta files.
  • If I load AssetPath::from("foo.gltf.meta") then I get an asset with that meta's settings.
• Meta files can optionally give an explicit source file path.
  • I can have a bar.meta that points to foo.gltf.
  • I can have an alternative_bar.meta that also points to foo.gltf but with different settings.
• Meta files can optionally give a label.
  • I can have a bar_mesh_1.meta that points to foo.gltf#Mesh1.
• (Optional) Meta files can reference meta files.
  • I can have a bar_meshlet_1.meta that points to bar_mesh_1.meta but applies meshlet processing.
• Runtime settings and labels are done by constructing an AssetMeta in memory and loading it.
  • Maybe explicitly: AssetServer::load_with_meta(AssetMeta) -> Handle.
  • Or maybe there's a special in-memory AssetSource, so the interface is AssetServer::register_meta(AssetMeta) -> AssetPath.
  • (I suspect this is going to be complicated in practice, but there's plenty of design space).

In terms of user experience, I'd expect the Bevy Editor to include tools for generating and managing meta files. Most users would not edit meta files as text.

Pros:

• Asset paths stay simple.
• Source assets can be interpreted in different ways. Two meta files can reference the same source asset but with different settings/labels.
• Smooth upgrade path from "choose settings at runtime" to "choose settings at build time" - runtime metas become meta files.
• Might partially solve some aspects of one to many assets. Processed and labelled assets always correspond to a single meta file.
• Meta files and source files can be in different folders, allowing source files to be in different SCM repos or different SCMs entirely.
  • This can be a requirement for large source file repos.
  • Could even go so far as to support http or other remote sources in meta files.

Cons:

• Some loss of backwards compatibility, similar to this PR.
• Runtime metas could be a bit clunky and inefficient.
• Probably a bunch of issues I haven't thought of.

There's some precedent for this approach in Unreal's asset system - Unreal games always reference intermediate .uasset files instead of source files. Although there's also plenty of differences, and Unreal's asset system is not all gravy.

Summary

Overall, I think leaning into meta files is better than this PR, but also a bigger and more controversial change. Maybe this PR could be a short-term solution to #11111, then Meta All The Things is left open as a longer-term solution.