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

Add is_file and is_directory to AssetIo, AssetServer and LoadContext #2106

Closed
wants to merge 8 commits into from

Conversation

geckoxx
Copy link
Contributor

@geckoxx geckoxx commented May 5, 2021

In a lot of cases there could be assets with relations to other assets. At the moment the loading of those related assets isn't possible inside the AssetLoader.
In the case that an asset could be in different locations it's also not possible to check if a file exists.

To handle those cases better this pull request extends the AssetIo trait by is_file and exposes is_file and is_directory in AssetServer and LoadContext.
It also changes the LoadContext to contain and expose the AssetServer .

@TheRawMeatball TheRawMeatball added A-Assets Load files from disk to use for things like images, models, and sounds C-Feature A new feature, making something new possible labels May 5, 2021
@mockersf
Copy link
Member

mockersf commented May 5, 2021

Hello, and thank you for your PR!

If I understand your example correctly, it should already be possible with this asset loader:

impl AssetLoader for RelatedAssetLoader {
    fn load<'a>(
        &'a self,
        bytes: &'a [u8],
        load_context: &'a mut LoadContext,
    ) -> BoxedFuture<'a, Result<(), anyhow::Error>> {
        Box::pin(async move {
            let custom_asset = ron::de::from_bytes::<CustomAsset>(bytes)?;
            let texture_path = AssetPath::new(custom_asset.texture.into(), None);
            let texture = load_context.get_handle(texture_path.clone());
            load_context.set_default_asset(
                LoadedAsset::new(RelatedAsset {
                    texture: Some(texture),
                })
                .with_dependencies(vec![texture_path]),
            );
            Ok(())
        })
    }

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

Would that work for you?

This part of Bevy is very under documented but may be completely changed soon

@geckoxx
Copy link
Contributor Author

geckoxx commented May 5, 2021

Thanks. Didn't know that. That helps a lot.

But it solves just one part of my problem.

For example the related Asset could be in different directories and the first found should be used:

CustomAsset (
    texture: "icon.png"
)
impl AssetLoader for RelatedAssetLoader {
    fn load<'a>(
        &'a self,
        bytes: &'a [u8],
        load_context: &'a mut LoadContext,
    ) -> BoxedFuture<'a, Result<(), anyhow::Error>> {
        Box::pin(async move {
            let possible_dirs = ["textures", "branding"];
            let custom_asset = ron::de::from_bytes::<CustomAsset>(bytes)?;
            let texture_path = possible_dirs
                .iter()
                .map(|dir| format!("{}/{}", dir, custom_asset.texture))
                .find_map(|texture_path| {
                    load_context
                        .is_file(&texture_path)
                        .then(|| AssetPath::new(texture_path.into(), None))
                });
            load_context.set_default_asset(
                LoadedAsset::new(RelatedAsset {
                    texture: texture_path
                        .as_ref()
                        .map(|texture_path| load_context.get_handle(texture_path.clone())),
                })
                .with_dependencies(texture_path.into_iter().collect()),
            );
            Ok(())
        })
    }

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

So it's not realy about related assets but to be able to check if a file or directory exists.

@geckoxx geckoxx changed the title Allow related assets to be loaded inside an asset loader Add is_file and is_directory to AssetIo, AssetServer and LoadContext May 5, 2021
@mockersf
Copy link
Member

mockersf commented May 5, 2021

I would rather not expose those two methods on the asset server, but otherwise looks good to me 👍

Comment on lines +42 to 43
fn is_file(&self, path: &Path) -> bool;
fn is_directory(&self, path: &Path) -> bool;
Copy link
Contributor

@NathanSWard NathanSWard May 5, 2021

Choose a reason for hiding this comment

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

This may be out of scope for this PR, however I'd be wary of adding on functions like this to the AssetIo trait. This could keep expanding and making the class larger and larger.

An alternative to consider it have a metadata function which

fn metadata(&self, path: &Path) -> std::io::Result<std::fs::Metadata>;

This way the metadata function coalesces the is_directory/is_file methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. But std::fs::Metadata cannot be used with archives like in bevy_assetio_zip. Am i right?

Copy link
Contributor

@NathanSWard NathanSWard May 5, 2021

Choose a reason for hiding this comment

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

Hmm, I'm not very familiar with bevy_assetio_zip, though after taking a brief look at the implementation, it uses a Box<dyn AssetIo> under the hood which would allow you to call metadata() on it.
So I'm not sure how using std::fs::Metadata would conflict with the crate, but again I could be wrong haha 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it was a bad example. 😄 My projects for example uses multiple archives and simulates a filesystem on top of it. So I cannot use std::fs::Metadata.

Copy link
Member

Choose a reason for hiding this comment

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

AssetIo is a "virtual filesystem", but it isn't a real filesystem as defined by std::fs. I'm cool with a Metadata struct that holds "virtual filesystem metadata" for a given path, but we should define that ourselves and scope it specifically to things that any "virtual filesystem" could provide. Probably just "is_file" and "is_directory" for now.

Comment on lines +517 to +523
pub fn is_directory<P: AsRef<Path>>(&self, path: P) -> bool {
self.server.asset_io.is_directory(path.as_ref())
}

pub fn is_file<P: AsRef<Path>>(&self, path: P) -> bool {
self.server.asset_io.is_file(path.as_ref())
}
Copy link
Contributor

@NathanSWard NathanSWard May 5, 2021

Choose a reason for hiding this comment

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

I'm not a fan of this on the AssetServer class, as it seems to break the responsibility of the struct. (They're fine on AssetIo though :)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah rather than mirror AssetIo methods here, we should probably just add an asset_io accessor method to AssetServer (no need for asset_io_mut because it currently has no mut methods).

@geckoxx
Copy link
Contributor Author

geckoxx commented May 6, 2021

Closed in favour of #2123

@geckoxx geckoxx closed this May 6, 2021
bors bot pushed a commit that referenced this pull request May 2, 2022
This is a replacement for #2106

This adds a `Metadata` struct which contains metadata information about a file, at the moment only the file type.
It also adds a `get_metadata` to `AssetIo` trait and an `asset_io` accessor method to `AssetServer` and `LoadContext`

I am not sure about the changes in `AndroidAssetIo ` and `WasmAssetIo`.
exjam pushed a commit to exjam/bevy that referenced this pull request May 22, 2022
This is a replacement for bevyengine#2106

This adds a `Metadata` struct which contains metadata information about a file, at the moment only the file type.
It also adds a `get_metadata` to `AssetIo` trait and an `asset_io` accessor method to `AssetServer` and `LoadContext`

I am not sure about the changes in `AndroidAssetIo ` and `WasmAssetIo`.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
This is a replacement for bevyengine#2106

This adds a `Metadata` struct which contains metadata information about a file, at the moment only the file type.
It also adds a `get_metadata` to `AssetIo` trait and an `asset_io` accessor method to `AssetServer` and `LoadContext`

I am not sure about the changes in `AndroidAssetIo ` and `WasmAssetIo`.
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-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants