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

Process Asset File Extensions With Multiple Dots #1277

Merged
merged 1 commit into from
Jan 25, 2021

Conversation

zicklag
Copy link
Member

@zicklag zicklag commented Jan 21, 2021

Fixes #1276.

Not sure if this is how we want it or not, but this implements my suggestion for #1276. Totally open to suggestions on better ways to do this.

@zicklag zicklag force-pushed the multi-dot-asset-loaders branch 2 times, most recently from 20b7cc6 to a8bf9b9 Compare January 21, 2021 03:47
@rparrett
Copy link
Contributor

rparrett commented Jan 21, 2021

This might produce surprising results when using filenames containing periods, e.g. some-asset-1.2.3.png (not that I condone that sort of thing)

Seems like with that particular filename as an example, we might want to check .png first, then 3.png and finally 2.3.png and return the first error unless we do find a loader.

edit: oops, that all seems a bit reversed.

@zicklag
Copy link
Member Author

zicklag commented Jan 21, 2021

@bjorn3 and @rparrett, just pushed a commit that should address both of your notes. I think that will take care of any nasty surprises and allow for the use-case that I was hoping for.

@rparrett
Copy link
Contributor

Oops, I think my original comment may have been slightly nonsensical.

I've been playing around with this as well as a learning exercise. I ended up adding some tests in my branch that might be useful.

https://github.com/rparrett/bevy/blob/multiple-dots/crates/bevy_asset/src/asset_server.rs

@zicklag
Copy link
Member Author

zicklag commented Jan 21, 2021

Oh, well, actually, I noticed that my example doesn't handle the test.1.character.yml case. 🤔 Did your version end up working, it might be better than mine. If it works, you could just submit a PR to replace this one. You've got tests too, which is good! 😄

@rparrett
Copy link
Contributor

rparrett commented Jan 21, 2021

Did your version end up working

I'm not sure! I haven't tested it at all beyond those simple unit tests. I'm not sure if my implementation is good and free of side effects. I'm a total newbie at error juggling in rust.

edit: actually, pretty sure it's returning the wrong error in the case that no asset loader is found for a multi-dot filename.

Comment on lines 142 to 155
// If the filename has multiple dots, try to match on a multi-dot file extension
if s.split('.').count() > 1 {
if let Ok(loader) = self.get_asset_loader(&s[s.find('.').unwrap() + 1..]) {
return Ok(loader);
}
}

// Just get the file extension normaly if that doesn't work
let extension = &s[s
.rfind('.')
.ok_or(AssetServerError::MissingAssetLoader(None))?
+ 1..];

self.get_asset_loader(extension)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// If the filename has multiple dots, try to match on a multi-dot file extension
if s.split('.').count() > 1 {
if let Ok(loader) = self.get_asset_loader(&s[s.find('.').unwrap() + 1..]) {
return Ok(loader);
}
}
// Just get the file extension normaly if that doesn't work
let extension = &s[s
.rfind('.')
.ok_or(AssetServerError::MissingAssetLoader(None))?
+ 1..];
self.get_asset_loader(extension)
let mut ext = s;
while let Some(idx) = ext.find('.') {
ext = &ext[idx + 1..];
if let Ok(loader) = self.get_asset_loader(ext) {
return Ok(loader);
}
}
Err(AssetServerError::MissingAssetLoader(None))

For foo.bar.sprite.png this would try bar.sprite.png, sprite.png and then png as extensions to get a loader for.

Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't see #1277 (comment) before, but this seems cleaner to me.

Copy link
Contributor

@rparrett rparrett Jan 21, 2021

Choose a reason for hiding this comment

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

I agree, but it seems like we'd want to to preserve the final e.g. AssetServerError::MissingAssetLoader("png") from get_asset_loader

Copy link
Contributor

@rparrett rparrett Jan 21, 2021

Choose a reason for hiding this comment

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

I added a test over in my branch for that, if that is indeed the behavior we want.

rparrett@a6a94f7

Copy link
Member Author

@zicklag zicklag Jan 21, 2021

Choose a reason for hiding this comment

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

Here's a modification of @rparrett's version that is nicely readable unlike my initial version and leaves great error messages. 😃

        let s = path
            .as_ref()
            .file_name()
            .ok_or(AssetServerError::MissingAssetLoader(None))?
            .to_str()
            .ok_or(AssetServerError::MissingAssetLoader(None))?;

        // given the filename asset-v1.2.3.png, produce an iterator of file extensions that looks
        // like this: 2.3.png, 3.png, png
        let extensions = s.match_indices('.').map(|(i, _)| &s[i + 1..]);

        // Map extensions to loaders for each extension
        let loaders = extensions.clone().map(|ext| self.get_asset_loader(ext));

        // Default the result to an error if a mathching extension cannot be found
        let mut result = Err(AssetServerError::MissingAssetLoader(Some(format!(
            "Could not find asset loader for any of these file extensions: {}",
            extensions.collect::<Vec<_>>().join(", ")
        ))));

        // search for any successful asset loader, stopping at the first successful
        for loader_result in loaders {
            if loader_result.is_ok() {
                result = loader_result;
            }
        }

        // Return the final result
        result

Combine that with the tests and I think we're in good shape.

If you guys like that one, I suggest that @rparrett just open a PR with those changes and we go with that. I tested the logic in my use-case and it works great. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the inner value of MissingAssetLoader is meant to be the extension rather than an error message. #[derive(Error)] handles formatting the message, right?

It's just unfortunate that right now that the extension is not displayed there. I'm not sure if you can actually get the inner value of the optional string from the #[error()] attribute though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do agree that the direction bjorn is going with the extension checking seems way cleaner though.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just unfortunate that right now that the extension is not displayed there. I'm not sure if you can actually get the inner value of the optional string from the #[error()] attribute though.

This should do it, I think. It's ugly, though.

#[error("no AssetLoader found{}", .0.as_ref().map(|x| format!(" for the following extension(s): {}", x)).unwrap_or(String::new()))]
    MissingAssetLoader(Option<String>),

I do agree that the direction bjorn is going with the extension checking seems way cleaner though.

Yep, you're right. That makes more sense. I had missed that. I need to pay more attention. 😃

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, this is better:

/// Errors that occur while loading assets with an AssetServer
#[derive(Error, Debug)]
pub enum AssetServerError {
    #[error("asset folder path is not a directory")]
    AssetFolderNotADirectory(String),
    #[error("no AssetLoader found{}", format_missing_asset_ext(.0))]
    MissingAssetLoader(Option<String>),
    #[error("the given type does not match the type of the loaded asset")]
    IncorrectHandleType,
    #[error("encountered an error while loading an asset")]
    AssetLoaderError(anyhow::Error),
    #[error("`PathLoader` encountered an error")]
    PathLoaderError(#[from] AssetIoError),
}

fn format_missing_asset_ext(ext: &Option<String>) -> String {
    return ext
        .as_ref()
        .map(|x| format!(" for the following extension(s): {}", x))
        .unwrap_or(String::new());
}

@zicklag
Copy link
Member Author

zicklag commented Jan 21, 2021

There, I made a combined updated version incorporating all of your feedback and my updates.

And here's an example error message:

no AssetLoader found for the following extension(s): 1.chaacter.yml, chaacter.yml, yml

@cart
Copy link
Member

cart commented Jan 22, 2021

Before merging we should probably double check with @kabergstrom to make sure that atelier can support this scenario.

@zicklag
Copy link
Member Author

zicklag commented Jan 22, 2021

Address the review points. Also, I just stole @rparrett 's tests for this PR and @bjorn3 's implementation for the extension matching. All I did was the error formatter. 🤷 if you guys want to make commits that I can merge them into this PR so you get your names on it let me know.

@rparrett
Copy link
Contributor

Don't sell yourself short! Documenting an issue, submitting a solution and wrangling a bunch of reviews and ad-hoc contributions is heroic effort in my book.

@rparrett
Copy link
Contributor

As far as I can tell, atelier-assets also can't do this, but it seems like it might be easy enough to add.

This chunk of code seems like the place to look.

https://github.com/amethyst/atelier-assets/blob/master/daemon/src/daemon.rs#L32

@zicklag
Copy link
Member Author

zicklag commented Jan 25, 2021

@rparrett good find. That looks like almost exactly what we are doing here in Bevy, so it should be just as simple to update there as it was here. 👍

@cart
Copy link
Member

cart commented Jan 25, 2021

Cool. So I guess the biggest question is "would @kabergstrom accept the change", which we should answer before merging.

@kabergstrom
Copy link

I like it! 😊👍

@zicklag
Copy link
Member Author

zicklag commented Jan 25, 2021

Cool! I just pushed an update resolving the merge conflicts.

@cart cart merged commit b6485cc into bevyengine:master Jan 25, 2021
@zicklag zicklag deleted the multi-dot-asset-loaders branch January 25, 2021 20:38
rparrett pushed a commit to rparrett/bevy that referenced this pull request Jan 27, 2021
Process Asset File Extensions With Multiple Dots
Fixes bevyengine#1276
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow Loading Multi-dot Extensions for Asset Files
6 participants