Fix embedded asset path manipulation

[bonsairobo]

Objective

Fixes #10377

Solution

Use Path::strip_prefix instead of str::split. Avoid any explicit "/" characters in path manipulation.

---

Changelog

• Added: example of embedded asset loading
• Added: support embedded assets in external crates
• Fixed: resolution of embedded assets
• Fixed: unexpected runtime panic during asset path resolution

Migration Guide

No API changes.

[github-actions]

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

[github-actions]

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

[github-actions]

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

[github-actions]

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

[github-actions]

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

[bonsairobo]

After submitting this issue for std::file, I'm wondering if embedded assets will work if the crate is being used as a crates.io dependency. For context:

If you pass src/lib.rs to rustc as filename and you have a module foo at src/foo.rs, then file!() will return src/foo.rs when invoked inside that module. If you pass /path/to/lib.rs then it will return /path/to/foo.rs when invoked inside the foo module.

I think this means you might try to do this in src/plugin.rs:

embedded_asset!(app, "my_asset.png");

...

let asset = server.load("embedded://my_crate/my_asset.png");

And rustc is invoked with the source file path as an absolute path /path/to/crates/my_crate/src/lib.rs, so the path returned from file!() is /path/to/crates/my_crate/src/plugin.rs. This wouldn't have a predictable src/ prefix, so the macro would panic.

I think this is a case where the old behavior could have worked properly (not on Windows).

We might want to add a condition that checks if the file!() output is an absolute path.

It might be challenging to test this 😓 .

[bonsairobo]

I've addressed the external crate issue. I was able to test this by creating a git repo with a crate that embeds an asset, loads it, and spawns it. Then I made another local crate that depends on this git repo and adds the plugin. As anticipated, it would fail to load the asset without the f8e8e0d fix. The fix... fixes it.

This was a relatively involved integration test. If we want to keep this around, we could transfer ownership of the test repo to the bevyengine org and make another example.

[bonsairobo]

WASM CI failure looks unrelated to my changes.

[alice-i-cecile]

@viridia could I get your review here?

[viridia]

@viridia could I get your review here?

So I guess I'm being brought into this because of the prior discussion about wanting to remove the use of Path and PathBuf from AssetPath and creating our own implementation which will be platform-agnostic. While this PR kind of moves us in the opposite direction (adding another dependency on Path), I'm not too concerned about that. As long as there are tests which ensure that the behavior is correct, I'm not worried about replacing the logic later.

As far as the rest, I'm not familiar enough with the details of embedded assets to give a useful opinion. I guess I would ask whether this logic is something that belongs in AssetPath?

[bonsairobo]

wanting to remove the use of Path and PathBuf from AssetPath and creating our own implementation which will be platform-agnostic

I'm surprised by this. Isn't the whole point of std::path to be platform-agnostic?

[viridia]

I'm surprised by this. Isn't the whole point of std::path to be platform-agnostic?

@bonsairobo The problem is that AssetPaths aren't really paths - they are URLs. They eventually get converted into filesystem paths (depending on the asset source), but the rules for manipulating and combining asset paths are subtly different than the rules for filesystem paths. Path and PathBuf understand backslashes and recognize windows drive letters as absolute paths. We actually don't want asset paths to have those behaviors - an asset path which starts with a windows drive letter is meaningless to Bevy.

[bonsairobo]

@viridia OK sounds like AssetPath is trying to be some pseudo-Url such that the url crate is presumably not the right abstraction. I'm still coming up to speed on this design, could you share any links to discussions about that?

RE: This PR. Anything we get from std::file!() needs to be parsed as an OS-specific path string, which Path does for us. I don't think we should try to re-invent this wheel. It makes sense to me that AssetPath has a from_path method for whenever a local file path needs to be registered with the asset system. I don't know exactly where this boundary should be, but I think as long as we're parsing raw strs (or OsStrs) that represent OS-specific file paths, we should use Path.

[viridia]

Sorry for not responding sooner, dealing with some health issues. The latest discussion is #10511.

[alice-i-cecile]

@shanecelis @doonv could I get a review on this from one or both of you? I'd like to unblock the linked PR.

[doonv]

I'm not really a fan of the "double path" thing where you have to first have the path inside the embedded_asset! macro, and then the path every time you want to access the asset.

I'd rather have something more similar to what we had earlier with load_internal_asset!. Where you stored the handle in a static variable.

[bonsairobo]

@doonv

I'm not really a fan of the "double path" thing where you have to first have the path inside the embedded_asset! macro, and then the path every time you want to access the asset.

I'd rather have something more similar to what we had earlier with load_internal_asset!. Where you stored the handle in a static variable.

I think that's out of scope for this PR, which is not changing the public API.

[shanecelis]

@doonv Re: Handle vs double path

I've converted some code from using load_internal_asset!() to embedded_asset!() and there was a benefit I noted using a path with embedded_asset!(). For instance I had this code from kayak_ui:

#[derive(AsBindGroup, Asset, TypePath, Debug, Clone)]
pub struct MyUIMaterial {}
impl MaterialUI for MyUIMaterial {
    fn fragment_shader() -> bevy::render::render_resource::ShaderRef {
        "rainbow_shader.wgsl".into()
    }
}

Now kayak_ui's module that has the handles to its shaders as pub const, so they're accessible and useable in the trait above. But what if you wanted to dynamically load an asset? You can get the Handle<Shader> but where would you put it? In the scenario above fragment_shader() doesn't use a &self, so you'd need to access some other way, and forgive me but I don't really know a way other than static mut, which seems bad. With the embedded_asset!() you can do something that is nearly the same:

#[derive(AsBindGroup, Asset, TypePath, Debug, Clone)]
pub struct MyUIMaterial {}
impl MaterialUI for MyUIMaterial {
    fn fragment_shader() -> bevy::render::render_resource::ShaderRef {
        "embedded://custom_shader/rainbow_shader_embed.wgsl".into()
    }
}

I thought that was a win for embedded_asset!() compared to load_internal_asset!(). I also like the assets in general are accessible to the user, without the original provider ensuring that you have access to the handles. However, the double-path issue is real; in fact, you might even call it triple-path.

But I agree the many paths—a file system source path, an asset-path, an embedded://crate-name/asset-path—is confusing, especially at first. One thing I would really like is if embedded_asset!() or a variant would return the string that it constructs as the asset's key, e.g., "embedded://crate-name/asset-path". That way a user could add dbg!(embedded_asset!(...)) if they're running into trouble to see what URI their asset is being keyed to.

[shanecelis]

I added some more tests in a PR bonsairobo#1 to this PR to exercise its behavior on error conditions. I never provoked an unwrap() None error, so I'm happy with that. And I prefer this to my own PR #11340 which merely reports errors. This PR interrogates the paths as Paths, which should alleviate some cross platform woes.

[ryanpeach]

Is this going into the next release?

[alice-i-cecile]

Unlikely: it needs two community approvals before it can merged by maintainers. If this interests you, please feel free to leave a code review!

[bonsairobo]

Unlikely: it needs two community approvals before it can merged by maintainers. If this interests you, please feel free to leave a code review!

@alice-i-cecile

I wasn't aware of this process. Is there anything blocking approval by @doonv and @shanecelis?

[shanecelis]

No, nothing blocking. I’ll be happy to approve. I added some tests and exercised it. It’d be nice for this to make it in. I don’t see the approve button here on mobile. Will search for it on my computer shortly.

[alice-i-cecile]

Go to the "Files changed" tab, then "Review changes", then "Approve" :)

[shanecelis]

Done. Thank you, @alice-i-cecile. Great talk on reflection by the way.