bevyengine
Use Asset Path Extension for `AssetLoader` Disambiguation
Objective
- Fixes #11638
- See here for details on the cause of this issue.
Solution
- Modified
AssetLoadersto capture possibility of multipleAssetLoaderregistrations operating on the sameAssettype, but different extensions. - Added an algorithm which will attempt to resolve via
AssetLoadername, thenAssettype, then by extension. If at any point multiple loaders fit a particular criteria, the next criteria is used as a tie breaker.
I tried an extended example of the previous review I did, it does not work:
Example
// my-asset
RonAsset (
name: "Cube 1",
translation: (0.1, 0.9, 2.4)
)
// my-asset-2
RonOtherAsset (
name: "Cube 2",
translation: (0.8, 0.3, 0.9)
)
// my-asset.ronasset
RonAsset (
name: "Cube 3",
translation: (0.8, 0.3, 0.9)
)
// my-asset.also-ronasset
RonAsset (
name: "Cube 4",
translation: (0.8, 3.3, 0.9)
)
use bevy::{
asset::{io::Reader, ron, AssetLoader, AsyncReadExt, LoadContext},
prelude::*,
reflect::TypePath,
utils::{thiserror, thiserror::Error, BoxedFuture},
};
use serde::Deserialize;
#[derive(Asset, TypePath, Debug, Deserialize, Component)]
pub struct RonAsset {
pub name: String,
pub translation: Vec3,
}
#[derive(Asset, TypePath, Debug, Deserialize, Component)]
pub struct RonOtherAsset {
pub name: String,
pub translation: Vec3,
}
#[derive(Default)]
pub struct RonLoaderNoExt;
#[derive(Default)]
pub struct RonLoaderWithExt;
#[derive(Default)]
pub struct RonLoaderWithOtherExt;
#[derive(Default)]
pub struct RonLoaderNoExtOtherAsset;
#[non_exhaustive]
#[derive(Debug, Error)]
pub enum RonLoaderError {
/// An [IO](std::io) Error
#[error("Could not load asset: {0}")]
Io(#[from] std::io::Error),
/// A [RON](ron) Error
#[error("Could not parse RON: {0}")]
RonSpannedError(#[from] ron::error::SpannedError),
}
impl AssetLoader for RonLoaderNoExt {
type Asset = RonAsset;
type Settings = ();
type Error = RonLoaderError;
fn load<'a>(
&'a self,
reader: &'a mut Reader,
_settings: &'a (),
_load_context: &'a mut LoadContext,
) -> BoxedFuture<'a, Result<Self::Asset, Self::Error>> {
Box::pin(async move {
let mut bytes = Vec::new();
reader.read_to_end(&mut bytes).await?;
let custom_asset = ron::de::from_bytes::<RonAsset>(&bytes)?;
Ok(custom_asset)
})
}
}
impl AssetLoader for RonLoaderNoExtOtherAsset {
type Asset = RonOtherAsset;
type Settings = ();
type Error = RonLoaderError;
fn load<'a>(
&'a self,
reader: &'a mut Reader,
_settings: &'a (),
_load_context: &'a mut LoadContext,
) -> BoxedFuture<'a, Result<Self::Asset, Self::Error>> {
Box::pin(async move {
let mut bytes = Vec::new();
reader.read_to_end(&mut bytes).await?;
let custom_asset = ron::de::from_bytes::<RonOtherAsset>(&bytes)?;
Ok(custom_asset)
})
}
}
// same asset but with ext
impl AssetLoader for RonLoaderWithExt {
type Asset = RonAsset;
type Settings = ();
type Error = RonLoaderError;
fn load<'a>(
&'a self,
reader: &'a mut Reader,
_settings: &'a (),
_load_context: &'a mut LoadContext,
) -> BoxedFuture<'a, Result<Self::Asset, Self::Error>> {
Box::pin(async move {
let mut bytes = Vec::new();
reader.read_to_end(&mut bytes).await?;
let custom_asset = ron::de::from_bytes::<RonAsset>(&bytes)?;
Ok(custom_asset)
})
}
fn extensions(&self) -> &[&str] {
&["ronasset"]
}
}
// same asset but with other ext
impl AssetLoader for RonLoaderWithOtherExt {
type Asset = RonAsset;
type Settings = ();
type Error = RonLoaderError;
fn load<'a>(
&'a self,
reader: &'a mut Reader,
_settings: &'a (),
_load_context: &'a mut LoadContext,
) -> BoxedFuture<'a, Result<Self::Asset, Self::Error>> {
Box::pin(async move {
let mut bytes = Vec::new();
reader.read_to_end(&mut bytes).await?;
let custom_asset = ron::de::from_bytes::<RonAsset>(&bytes)?;
Ok(custom_asset)
})
}
fn extensions(&self) -> &[&str] {
&["also-ronasset"]
}
}
fn main() {
App::new()
.add_plugins(DefaultPlugins)
.init_asset::<RonAsset>()
.init_asset_loader::<RonLoaderNoExt>()
.init_asset_loader::<RonLoaderWithExt>()
.init_asset_loader::<RonLoaderWithOtherExt>()
.init_asset::<RonOtherAsset>()
.init_asset_loader::<RonLoaderNoExtOtherAsset>()
.add_systems(Startup, setup)
.add_systems(Update, (set_cube_translations, set_cube_translation2))
.run();
}
#[derive(Debug, Component)]
struct Cube;
fn setup(
mut commands: Commands,
mut meshes: ResMut<Assets<Mesh>>,
mut materials: ResMut<Assets<StandardMaterial>>,
asset_server: Res<AssetServer>,
) {
// circular base
commands.spawn(PbrBundle {
mesh: meshes.add(shape::Circle::new(4.0)),
material: materials.add(Color::WHITE),
transform: Transform::from_rotation(Quat::from_rotation_x(-std::f32::consts::FRAC_PI_2)),
..default()
});
// cube 1
commands.spawn((
PbrBundle {
mesh: meshes.add(shape::Cube { size: 1.0 }),
material: materials.add(Color::rgb_u8(124, 144, 255)),
..default()
},
asset_server.load::<RonAsset>("data/my-asset"),
));
// cube 2
commands.spawn((
PbrBundle {
mesh: meshes.add(shape::Cube { size: 1.0 }),
material: materials.add(Color::rgb_u8(124, 244, 255)),
..default()
},
asset_server.load::<RonOtherAsset>("data/my-asset-2"),
));
// cube 3
commands.spawn((
PbrBundle {
mesh: meshes.add(shape::Cube { size: 1.0 }),
material: materials.add(Color::rgb_u8(124, 244, 10)),
..default()
},
asset_server.load::<RonAsset>("data/my-asset.ronasset"),
));
// cube 4
commands.spawn((
PbrBundle {
mesh: meshes.add(shape::Cube { size: 1.0 }),
material: materials.add(Color::rgb_u8(10, 244, 255)),
..default()
},
asset_server.load::<RonAsset>("data/my-asset.also-ronasset"),
));
// light
commands.spawn(PointLightBundle {
point_light: PointLight {
intensity: 250_000.0,
shadows_enabled: true,
..default()
},
transform: Transform::from_xyz(4.0, 8.0, 4.0),
..default()
});
// camera
commands.spawn(Camera3dBundle {
transform: Transform::from_xyz(-2.5, 4.5, 9.0).looking_at(Vec3::ZERO, Vec3::Y),
..default()
});
}
fn set_cube_translations(
mut transform_via_asset: Query<(&mut Transform, &Handle<RonAsset>)>,
ron_assets: Res<Assets<RonAsset>>,
) {
for (mut transform, asset_handle) in &mut transform_via_asset {
let Some(asset) = ron_assets.get(asset_handle) else {
continue;
};
if transform.translation != asset.translation {
info!("Updating {}", asset.name);
transform.translation = asset.translation;
}
}
}
fn set_cube_translation2(
mut transform_via_asset: Query<(&mut Transform, &Handle<RonOtherAsset>)>,
ron_assets: Res<Assets<RonOtherAsset>>,
) {
for (mut transform, asset_handle) in &mut transform_via_asset {
let Some(asset) = ron_assets.get(asset_handle) else {
continue;
};
if transform.translation != asset.translation {
info!("Updating {}", asset.name);
transform.translation = asset.translation;
}
}
}
The scenario:
- One extensionless assetloader for asset
RonAsset - Two loaders for
RonAssetwith (different) extensions - One extensionless loader for
RonOtherAsset
Run by cargo run --example hot_asset_reloading --features="file_watcher"
Output:
2024-02-02T09:06:15.703950Z INFO bevy_winit::system: Creating new window "App" (0v1)
2024-02-02T09:06:17.356811Z INFO bevy_asset::server: duplicate preregistration for type `bevy_render::texture::image::Image`.
2024-02-02T09:06:17.543573Z INFO bevy_asset::server: duplicate registration for type `hot_asset_reloading::RonAsset`.
2024-02-02T09:06:17.543593Z INFO bevy_asset::server: duplicate registration for type `hot_asset_reloading::RonAsset`.
2024-02-02T09:06:17.598346Z ERROR bevy_asset::server: no `AssetLoader` found for file with no extension
2024-02-02T09:06:17.608998Z INFO hot_asset_reloading: Updating Cube 2
2024-02-02T09:06:17.609035Z INFO hot_asset_reloading: Updating Cube 3
2024-02-02T09:06:17.609052Z INFO hot_asset_reloading: Updating Cube 4
I've marked this as draft for the moment since it needs some unit tests and a QA pass. Unfortunately it's not as simple as I had hoped to resolve, but I will continue working on it and try and get something as soon as possible.
I tried an extended example of the previous review I did, it does not work: Example
The scenario:
* One extensionless assetloader for asset `RonAsset` * Two loaders for `RonAsset` _with_ (different) extensions * One extensionless loader for `RonOtherAsset`Run by
cargo run --example hot_asset_reloading --features="file_watcher"Output:
2024-02-02T09:06:15.703950Z INFO bevy_winit::system: Creating new window "App" (0v1) 2024-02-02T09:06:17.356811Z INFO bevy_asset::server: duplicate preregistration for type `bevy_render::texture::image::Image`. 2024-02-02T09:06:17.543573Z INFO bevy_asset::server: duplicate registration for type `hot_asset_reloading::RonAsset`. 2024-02-02T09:06:17.543593Z INFO bevy_asset::server: duplicate registration for type `hot_asset_reloading::RonAsset`. 2024-02-02T09:06:17.598346Z ERROR bevy_asset::server: no `AssetLoader` found for file with no extension 2024-02-02T09:06:17.608998Z INFO hot_asset_reloading: Updating Cube 2 2024-02-02T09:06:17.609035Z INFO hot_asset_reloading: Updating Cube 3 2024-02-02T09:06:17.609052Z INFO hot_asset_reloading: Updating Cube 4
The issue here is that there are multiple AssetLoader's capable of loading the my-asset file as a RonAsset (three to be exact). The previous resolution algorithm basically gave up if no single loader could be found matching the supplied criteria (in this case, type of Some(RonAsset) and extension of None). I've changed the resolution algorithm to instead throw a warning in this situation, and then select the most recently added loader that matches the type. I've also added some unit tests which explicitly check for this case to make it clear how this behaves.
I believe this is now ready for review. I've refactored the AssetLoaders type into its own file. I've chosen to do this since adding unit tests for the AssetLoader resolution process was rather difficult while its data was stored in AssetLoaders, but its functionality was implemented in AssetServer. This separation was previously proposed and seemed to be generally accepted, but I removed it from an earlier PR to help get the core feature to land during merge conflict resolution.
More than half of this PR consists of unit tests for the resolution behaviour. To work around using async testing, I'm using block_on and channels to check which AssetLoader gets selected during resolution.
To the best of my testing, hot reloading, aliased loaders, and meta file overrides all work as expected, but please let me know if there's anything that I have missed!
Some key behaviours for the AssetLoader resolution process that may be of interest to reviewers:
- If an
AssetLoadername is specified, it will always be used. This is also the case with meta files anyway so I expect this to be a safe assumption. - If an asset type is specified, excluding condition 1 above and asset path labels, it must be type
AssetLoader::Assetto match. This is also a safe assumption in my opinion, since otherwise theUntypedHandlewill fail to convert to a type ofHandle<A>and cause the loading to fail. - If two or more loaders are able to handle the same asset type, the tie will be broken using the file extension in the asset path. If this cannot be done, the most recently registered loader will be used, and a warning emitted. I believe the warning is appropriate, but I am open to removing it if this is expected functionality.
- If an asset type is not specified, then the most recently registered loader handling the declared extension will be used. If none can be found, then an
Errwill be thrown.
I tried the example I posted again, and I'm a bit unsure if I'm hitting a problem or intended behaviour.
Each time I update my-asset for the RonAsset type, the console prints:
2024-02-05T08:47:00.764606Z WARN bevy_asset::server::loaders: Multiple AssetLoaders found for Asset: Some(TypeId { t: 11099438548126506434038829544924945672 }); Path: Some(data/my-asset); Extension: None
The registered loaders are:
RonLoaderNoExt: Loads typeRonAsset, no extensionsRonLoaderWithExt: Loads typeRonAsset, extension must beronassetRonLoaderWithOtherExt: Loads typeRonAsset, extension must bealso-ronasset
And another type:
RonOtherAsset: Loads typeRonOtherAsset, no extensions
The three first loaders map to the same type, but they have no overlap in extensions required (unless all loaders implicitly can load without extension?).
The last loader shouldn't create any issues I think.
So should there in this case be any disambiguity?
I tried the example I posted again, and I'm a bit unsure if I'm hitting a problem or intended behaviour.
Each time I update
my-assetfor theRonAssettype, the console prints:2024-02-05T08:47:00.764606Z WARN bevy_asset::server::loaders: Multiple AssetLoaders found for Asset: Some(TypeId { t: 11099438548126506434038829544924945672 }); Path: Some(data/my-asset); Extension: None
The registered loaders are:
* `RonLoaderNoExt`: Loads type `RonAsset`, no extensions * `RonLoaderWithExt`: Loads type `RonAsset`, extension must be `ronasset` * `RonLoaderWithOtherExt`: Loads type `RonAsset`, extension must be `also-ronasset`And another type:
* `RonOtherAsset`: Loads type `RonOtherAsset`, no extensionsThe three first loaders map to the same type, but they have no overlap in extensions required (unless all loaders implicitly can load without extension?).
The last loader shouldn't create any issues I think.
So should there in this case be any disambiguity?
Yes this is how I've intended this functionality to work. In essence, a lack of extension means "we don't know what the extension is", so any AssetLoader with a matching type is a valid loader. Since you have 3 asset loaders for the type RonAsset (one with no extension, and two with different extensions), any of those 3 would be valid targets for the asset missing an extension. The warning is there explicitly to call out that this situation has been hit and might not be intended.
This is analogous to having 3 AssetLoader's with the same extension in prior versions of Bevy. Except previously, duplicate loaders were used as replacements entirely, whereas now we have a new field (asset type) to increase the chance that a particular loader is a unique loader to be used.
Perhaps your assumption was instead that if a loader specified no extensions, then it would be the one used for extensionless assets? Not saying that's wrong or a bad assumption, just calling out that it's different to my assumptions and I want to clarify.
Yes this is how I've intended this functionality to work. In essence, a lack of extension means "we don't know what the extension is", so any
AssetLoaderwith a matching type is a valid loader. Since you have 3 asset loaders for the typeRonAsset(one with no extension, and two with different extensions), any of those 3 would be valid targets for the asset missing an extension. The warning is there explicitly to call out that this situation has been hit and might not be intended.This is analogous to having 3
AssetLoader's with the same extension in prior versions of Bevy. Except previously, duplicate loaders were used as replacements entirely, whereas now we have a new field (asset type) to increase the chance that a particular loader is a unique loader to be used.Perhaps your assumption was instead that if a loader specified no extensions, then it would be the one used for extensionless assets? Not saying that's wrong or a bad assumption, just calling out that it's different to my assumptions and I want to clarify.
I think my confusion arose because listing which extensions are supported by a loader is very explicit:
&["ron", "foo", "json"]
but extensionless support is implicit, and somehow that feels surprising to me.
But now that I know I understand what happens in the example, and as long as docs clarify this then I believe it should be ok.
I think my confusion arose because listing which extensions are supported by a loader is very explicit:
&["ron", "foo", "json"]but extensionless support is implicit, and somehow that feels surprising to me.
But now that I know I understand what happens in the example, and as long as docs clarify this then I believe it should be ok.
That's my bad for not making my own assumptions clear, thank you for pointing it out! My interpretation on the extension list is it is the preferred extensions of a loader. Since meta files allowed you to choose a loader by name (ignoring the extension), you could already load an asset with a different extension. Further, since you could have overlapping extension lists, your loader wasn't guaranteed to receive all asset paths matching the list, so it was already quite a loose definition.
I'm adding a short note to the AssetLoader::extensions method explicitly stating:
Note that users of this
AssetLoadermay choose to load files with a non-matching extension.
Which should clear up confusion on the implementation side of AssetLoader.
As for users (ones calling AssetServer::load, etc.), I think this behaviour better matches expectation. Now if you try to load an asset of type A, the AssetServer will succeed in more cases than before (missing extension, wrong extension), and not fail in ways it wasn't already (as in, the number of error cases has reduced).