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

Use Asset Path Extension for AssetLoader Disambiguation #11644

Conversation

bushrat011899
Copy link
Contributor

@bushrat011899 bushrat011899 commented Feb 1, 2024

Objective

Solution

  • Modified AssetLoaders to capture possibility of multiple AssetLoader registrations operating on the same Asset type, but different extensions.
  • Added an algorithm which will attempt to resolve via AssetLoader name, then Asset type, then by extension. If at any point multiple loaders fit a particular criteria, the next criteria is used as a tie breaker.

Added algorithm which will attempt to resolve via `AssetLoader` name, then `Asset` type, then by extension. If at any point multiple loaders fit a particular criteria, the next criteria is used as a tie breaker.
@Kanabenki Kanabenki added C-Bug An unexpected or incorrect behavior A-Assets Load files from disk to use for things like images, models, and sounds labels Feb 1, 2024
@torsteingrindvik
Copy link
Contributor

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

@alice-i-cecile alice-i-cecile added this to the 0.13 milestone Feb 2, 2024
@bushrat011899 bushrat011899 marked this pull request as draft February 3, 2024 04:52
@bushrat011899
Copy link
Contributor Author

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.

@bushrat011899
Copy link
Contributor Author

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.

@bushrat011899 bushrat011899 marked this pull request as ready for review February 5, 2024 07:20
@bushrat011899
Copy link
Contributor Author

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:

  1. If an AssetLoader name 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.
  2. If an asset type is specified, excluding condition 1 above and asset path labels, it must be type AssetLoader::Asset to match. This is also a safe assumption in my opinion, since otherwise the UntypedHandle will fail to convert to a type of Handle<A> and cause the loading to fail.
  3. 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.
  4. 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 Err will be thrown.

@torsteingrindvik
Copy link
Contributor

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 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 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?

@bushrat011899
Copy link
Contributor Author

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 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 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?

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.

@torsteingrindvik
Copy link
Contributor

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.

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.

@bushrat011899
Copy link
Contributor Author

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 AssetLoader may 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).

Copy link
Contributor

@torsteingrindvik torsteingrindvik left a comment

Choose a reason for hiding this comment

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

Tried the example I posted which still works (and I understand why there are disambiguities now). Nice with tests.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Solid work: this is a nice refactor / fix and well-tested.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 12, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 12, 2024
Merged via the queue into bevyengine:main with commit 7b5a4ec Feb 12, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong asset loader is used after extensionless assets change
4 participants