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

Wrong asset loader is used after extensionless assets change #11638

Closed
rparrett opened this issue Jan 31, 2024 · 3 comments · Fixed by #11644
Closed

Wrong asset loader is used after extensionless assets change #11638

rparrett opened this issue Jan 31, 2024 · 3 comments · Fixed by #11644
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Bug An unexpected or incorrect behavior
Milestone

Comments

@rparrett
Copy link
Contributor

rparrett commented Jan 31, 2024

Bevy version

main, after #10153

What you did

With the following fork/branch of bevy_common_assets:
https://github.com/rparrett/bevy_common_assets/tree/bevy13

cargo run --example multiple_formats --features=json,ron

What went wrong

The ron file is parsed as json and throws an error.

2024-01-31T22:52:54.621165Z ERROR bevy_asset::server: Failed to load asset 'trees.level.ron' with asset loader 'bevy_common_assets::json::JsonAssetLoader<multiple_formats::Level>': Could not parse the JSON: expected value at line 1 column 1
@rparrett rparrett added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Jan 31, 2024
@JMS55 JMS55 added A-Assets Load files from disk to use for things like images, models, and sounds and removed S-Needs-Triage This issue needs to be labelled labels Feb 1, 2024
@JMS55 JMS55 added this to the 0.13 milestone Feb 1, 2024
@rparrett
Copy link
Contributor Author

rparrett commented Feb 1, 2024

Or here's a smaller repro:

Open
use bevy::utils::thiserror;
use bevy::{
    asset::{io::Reader, AssetLoader, LoadContext},
    prelude::*,
    reflect::TypePath,
    utils::BoxedFuture,
};
use thiserror::Error;

#[non_exhaustive]
#[derive(Debug, Error)]
#[error("error")]
pub struct GenericLoaderError;

#[derive(Asset, TypePath, Debug, Default)]
pub struct C(String);

#[derive(Default)]
pub struct ALoader;

impl AssetLoader for ALoader {
    type Asset = C;
    type Settings = ();
    type Error = GenericLoaderError;
    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 { Ok(C("A".to_string())) })
    }

    fn extensions(&self) -> &[&str] {
        &["a"]
    }
}

#[derive(Default)]
pub struct BLoader;

impl AssetLoader for BLoader {
    type Asset = C;
    type Settings = ();
    type Error = GenericLoaderError;
    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 { Ok(C("B".to_string())) })
    }

    fn extensions(&self) -> &[&str] {
        &["b"]
    }
}

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .init_resource::<State>()
        .init_asset::<C>()
        .register_asset_loader(ALoader)
        .register_asset_loader(BLoader)
        .add_systems(Startup, setup)
        .add_systems(Update, print_on_load)
        .run();
}

#[derive(Resource, Default)]
struct State {
    handle_a: Handle<C>,
    handle_b: Handle<C>,
    printed: bool,
}

fn setup(mut state: ResMut<State>, asset_server: Res<AssetServer>) {
    state.handle_a = asset_server.load("data/custom.a");
    state.handle_b = asset_server.load("data/custom.b");
}

fn print_on_load(mut state: ResMut<State>, a_assets: Res<Assets<C>>) {
    if state.printed {
        return;
    }

    let a_asset = a_assets.get(&state.handle_a);
    let b_asset = a_assets.get(&state.handle_b);

    if b_asset.is_none() && b_asset.is_none() {
        return;
    }

    info!("Loaded: {:?} {:?}", a_asset.unwrap(), b_asset.unwrap());

    state.printed = true;
}

Before #10153:
Loaded: C("A") C("B").
After:
Loaded: C("B") C("B")

@bushrat011899
Copy link
Contributor

Hi, I believe the issue here is caused by your two different loaders producing the same asset type C. In the extension-less asset PR, asset loader resolution will short-circuit file extensions in favour of asset type resolution when that information is available. This is effectively the inverse problem to having two asset loaders registered for the same file extension.

I believe this can be fixed by amending AssetLoaders to store a list of MaybeAssetLoader's in type_id_to_loader and select 1 (probably last added). Then during asset loader resolution check if there are multiple loaders for that type, and then use the file extension to attempt to break the deadlock.

I'm terribly sorry this popped up for you, I'll make a PR based on my outline here and hopefully get that fixed soon!

@monax3
Copy link

monax3 commented Feb 10, 2024

I believe I'm hitting a related issue but in my case it's a custom Image loader hitting unreachables. Minimal repro follows:

use bevy::asset::io::Reader;
use bevy::asset::{AssetLoader, LoadContext};
use bevy::prelude::*;
use bevy::utils::BoxedFuture;

#[derive(Default)]
struct UnreachableLoader;

impl AssetLoader for UnreachableLoader {
    type Asset = Image;
    type Error = std::io::Error;
    type Settings = ();

    fn load<'a>(
        &'a self,
        _reader: &'a mut Reader<'_>,
        _settings: &'a Self::Settings,
        _load_context: &'a mut LoadContext<'_>,
    ) -> BoxedFuture<'a, Result<Self::Asset, Self::Error>> {
        Box::pin(async { Ok(Image::default()) })
    }

    fn extensions(&self) -> &[&str] { &["unreachable"] }
}

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .init_asset_loader::<UnreachableLoader>()
        .run();
}
bevy-f7ffde730c324c74\afa7b5c\crates\bevy_asset\src\server\mod.rs:164:47:
internal error: entered unreachable code

The above PR also fixes my issue so I'm mainly posting this for posterity.

github-merge-queue bot pushed a commit that referenced this issue Feb 12, 2024
# Objective

- Fixes #11638
- See
[here](#11638 (comment))
for details on the cause of this issue.

## 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.
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants