Objective

Fix #11111 (AssetServer::load_with_settings can return the asset without the custom settings, along with other surprises).

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.

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 only create one copy of the asset, and 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 loaded 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 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));

This sounds straightforward, but there's a catch...

Why Is This Controversial?

I'm expecting this PR to be controversial because of some surprises hiding in ErasedSettings. Let's first explain how I got there.

• Correctly implementing load_with_settings requires identifying settings so they can be compared. There's no getting around this, and all the possible solutions have issues.
• Requiring the settings to implement PartialEq or Hash is an obvious solution, but this would be a new requirement and a pain for users to implement.
• However, settings are already required to implement Serialize, and we need a RON serializer for meta files.
• So my solution is... serialize the settings to RON and keep a hash of the text for comparison. 💀💀💀

That's got some major ick factor, but maybe it's the least worst solution? The alternatives I could think of are:

• PartialEq/Hash.
  • It's a pain, but arguably simpler in the long run. Could be done as a follow up PR.
• Keep MetaTransform and find some way to automatically use the boxed closure as an identity.
  • Maybe feasible but sounds risky.
• Require the user to explicitly provide some extra identifier.
  • E.g. associating each call to load_with_settings with some GUID.
  • Annoying to use, and easy to misuse.

Other Problems

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.

MetaTransform has been removed.

• I chose to remove MetaTransform because it seems incompatible with asset identity.
• Removing it should be safe as the engine only used it 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 could probably put it back in, even if that's just to punt on a decision and make this PR simpler.

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 next section).
• The alter_sprites and alter_mesh examples needed fixes.
• I don't see a way to avoid this.

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.

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 is 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.

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.