Move away from std::Path in bevy_assets #19133

lielfr · 2025-05-09T00:33:34Z

Objective

closes Move away from std::Path in bevy_assets #19079

Solution

changes AssetPath to use &str internally instead of keeping a Path
some external-facing methods still use either Path or PathBuf, please let me know if these should be changed as well

Testing

all existing tests seem to pass
added one test for PartialEq implementation

github-actions · 2025-05-09T00:33:46Z

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

alice-i-cecile · 2025-05-09T00:42:53Z

some external-facing methods still use either Path or PathBuf, please let me know if these should be changed as well

My intuition is that these should generally be something like impl Into<AssetPath>, but I don't know this code very well at all.

lielfr · 2025-05-09T00:43:52Z

some external-facing methods still use either Path or PathBuf, please let me know if these should be changed as well

My intuition is that these should generally be something like impl Into<AssetPath>, but I don't know this code very well at all.

let's wait for other reviewers then :)

bushrat011899

I love that you've taken this on! Just a couple of minor points mostly, but the biggest concern I have is we should probably add more tests that cover the operations we previously relied on Path for. e.g., parent(), path_components(), etc.

bushrat011899 · 2025-05-09T00:55:04Z

crates/bevy_asset/src/path.rs

    label: Option<CowArc<'a, str>>,
 }

+/// `PartialEq` needs to be derived manually for backwards compatibility.
+/// As `path` used to be `std::path::Path`, equality was tricky with a trailing slash.
+/// For example, "martin/stephan#dave" should be equal to "martin/stephan/#dave".


This is a very interesting point to raise.
Future PR: Should we keep this kind of equality? martin/stephan#dave reads to me as the asset stephan with the label dave, while martin/stephan/#dave reads like an asset with no name but labelled as dave. I think we should consider changing semantics to make these degenerate states unrepresentable.

My opinion is we should not support this behaviour. I am fine to leave this for another PR, but I would appreciate if you (@lielfr) opened the PR after this one lands. At least it would be a good place to discuss thoughts on this.

I can remove it, it will just require changing some of the tests to account for it

+1 for not supporting the behaviour. However if we do make this change, two asset paths martin/stephan#dave and martin/stephan/#dave which compare as different would map to Paths which are equal and return the same file.

Given we're inventing our own platform independent version of paths, maybe we could always require trailing slashes for directories.

crates/bevy_asset/Cargo.toml

bushrat011899 · 2025-05-09T01:15:51Z

some external-facing methods still use either Path or PathBuf, please let me know if these should be changed as well

I was thinking about this myself. For now, I agree the path forward is impl Into<AssetPath>, since that is our canonical Path type. But, I also think there's room to consider a new type similar to AssetSourceId that is just for the path part of an AssetPath (I'm choosing the awful name AssetPathId). The reason is two-fold:

Dyn-compatible traits can't use impl T in arguments, so they need a concrete type to refer to.
Traits like AssetLoader don't need to know about the AssetSourceId part of an AssetPath. It's information that does not matter to them.

So in the longer term, I would propose something like:

pub struct AssetPathId<'a>(CowArc<'a, str>);

Then AssetPath would be updated to store AssetPathId for path, and all methods currently working with Path would be swapped to AssetPathId, impl Into<AssetPathId>, AssetPath, or impl Into<AssetPath> based on their dyn-compatibility and information requirements.

andriyDev · 2025-05-09T06:03:22Z

crates/bevy_asset/src/path.rs

    label: Option<CowArc<'a, str>>,
 }

+/// `PartialEq` needs to be derived manually for backwards compatibility.
+/// As `path` used to be `std::path::Path`, equality was tricky with a trailing slash.
+/// For example, "martin/stephan#dave" should be equal to "martin/stephan/#dave".


My opinion is we should not support this behaviour. I am fine to leave this for another PR, but I would appreciate if you (@lielfr) opened the PR after this one lands. At least it would be a good place to discuss thoughts on this.

andriyDev · 2025-05-09T06:05:38Z

crates/bevy_asset/src/io/file/file_watcher.rs

        sender: Sender<AssetSourceEvent>,
        debounce_wait_time: Duration,
    ) -> Result<Self, notify::Error> {
-        let root = normalize_path(&path).canonicalize()?;


I believe this canonicalize is important here. It makes paths with dots work correctly "../../my_assets_dir". Relevant PR: #18345 (hey look its me! I forgot all about this lol)

Should FileWatcher even be dealing with asset paths? This seems like a case where a file system path actually does make sense? I'm not sure though.

I'm not familiar enough (yet?) with the codebase to decide, so I'd love some guidance here

I think we would have to roll out own version of canonicalize to maintain being able to use ".." for relative paths

I looked into this a bit more, and I'm doubling down: I think FileWatcher should continue to deal with Path directly. I think the main thing we want to replace is the fact that our asset paths (as in, the ones that we pass into asset_server.load()) include some platform specific nonsense. That does not mean that our entire stack needs to deal only with strings - at some point, we need to convert that asset path into a concrete file path, so a Path instance.

So coming at it from the other angle, FileWatcher should be dealing with concrete paths because it is listening for real file system events for a particular directory. So I think we should revert what's happening in this file_watcher mod.

The plan that cart and I discussed a couple of years ago:

Asset server will certainly use FilePaths internally, for environments that have filesystems. For web environments, FilePaths may not be needed at all.

However, the semantics of FilePaths should not leak to the external asset server API. There should be a clear distinction between file paths and asset paths. In general, the upper layer uses asset paths, and the lower layer uses file paths.

Path manipulations on asset paths (concatenation, parent, etc) should use custom methods written for Bevy (these shouldn't be difficult, it's just string manipulation). So for example AssetPath::resolve shouldn't use FilePath methods.

Path manipulations on file paths should use FilePath methods - so for example adding the asset directory base path to the asset path would be a FilePath method.

andriyDev · 2025-05-09T06:17:09Z

crates/bevy_asset/src/path.rs

@@ -255,7 +288,7 @@ impl<'a> AssetPath<'a> {
    /// Gets the path to the asset in the "virtual filesystem".
    #[inline]
    pub fn path(&self) -> &Path {


Should we be returning a str here instead? The comment says "path to the asset in the virtual filesystem", which implies we shouldn't be returning a native OS path.

it's possible but will require more extensive changes throughout bevy_asset. Is it ok to do this in the scope of this PR, or is it too big?

I think we should remove this in this PR. It will be more changes, but it will ensure we aren't accidentally converting to paths somewhere.

mcobzarenco

Thanks for taking on this. As far as I can tell, this PR still leaves Paths in the public API, I imagine the final version to use String/str everywhere. But splitting into multiple PRs may be worthwhile.

In addition to the other reviews above, my main comment is to avoid the lossy conversions -- changing the public APIs to take Strings in the same PR would avoid having to do the conversions. I would also love to see tests for std functions for path manipulation we replaced.

crates/bevy_asset/src/io/embedded/embedded_watcher.rs

crates/bevy_asset/src/io/source.rs

crates/bevy_asset/src/path.rs

lielfr · 2025-05-09T22:17:48Z

I would also love to see tests for std functions for path manipulation we replaced.

which tests? I saw there are already some that cover parent and some other manipulations

lielfr · 2025-05-09T22:19:16Z

the force-push was a result of a rebase from upstream

mcobzarenco

Thank you for the changes! I think I meant testing the new equality behaviour, but I now see there is one test for it.

I had a few other questions/thoughts, given we're essentially rolling our own platform independent version of paths, does it make sense to invest in some niceties for manipulating different parts? Not in this PR necessarily, although I think we probably have to roll our own canonicalize in this PR as @andriyDev pointed out.

crates/bevy_asset/src/io/embedded/embedded_watcher.rs

mcobzarenco · 2025-05-10T18:55:54Z

crates/bevy_asset/src/io/file/file_watcher.rs

        sender: Sender<AssetSourceEvent>,
        debounce_wait_time: Duration,
    ) -> Result<Self, notify::Error> {
-        let root = normalize_path(&path).canonicalize()?;


I think we would have to roll out own version of canonicalize to maintain being able to use ".." for relative paths

mcobzarenco · 2025-05-10T19:01:17Z

crates/bevy_asset/src/path.rs

    label: Option<CowArc<'a, str>>,
 }

+/// `PartialEq` needs to be derived manually for backwards compatibility.
+/// As `path` used to be `std::path::Path`, equality was tricky with a trailing slash.
+/// For example, "martin/stephan#dave" should be equal to "martin/stephan/#dave".


+1 for not supporting the behaviour. However if we do make this change, two asset paths martin/stephan#dave and martin/stephan/#dave which compare as different would map to Paths which are equal and return the same file.

Given we're inventing our own platform independent version of paths, maybe we could always require trailing slashes for directories.

crates/bevy_asset/src/path.rs

mcobzarenco · 2025-05-10T19:14:05Z

crates/bevy_asset/src/path.rs

+            return true;
+        }
+        let mut simplified = Vec::new();
+        for component in self.path_components() {


Would it be more readable and potentially more reusable path parsing if we roll our own version of path components with an enum?

it's possible, but I'm not sure about this. Do the other maintainers also agree?

I don't have a strong opinion here, leaning to no for simplicity?

lielfr · 2025-05-11T00:16:36Z

regarding implementing our own canonicalize, does it also need to consider symlinks? are these even possible in AssetPath?

I assume it does need to support . and .., and convert relative paths to absolute paths (or, are there absolute paths here at all?)

Regarding PartialEq and the possibility of requiring trailing slashes, does that need to be an unrecoverable error (panic)?

viridia · 2025-05-16T19:09:13Z

See also this comment by cart from a couple years ago: #9528 (comment)

Basically he and I agreed that we should get rid of Path; it has more features than we want, and some of those features produce unexpected and inappropriate behavior when used as an asset path. (Example, asset paths with drive letters should not be supported). Unfortunately, this work never got done.

andriyDev · 2025-06-01T06:42:14Z

crates/bevy_asset/src/io/file/file_watcher.rs

        sender: Sender<AssetSourceEvent>,
        debounce_wait_time: Duration,
    ) -> Result<Self, notify::Error> {
-        let root = normalize_path(&path).canonicalize()?;


I looked into this a bit more, and I'm doubling down: I think FileWatcher should continue to deal with Path directly. I think the main thing we want to replace is the fact that our asset paths (as in, the ones that we pass into asset_server.load()) include some platform specific nonsense. That does not mean that our entire stack needs to deal only with strings - at some point, we need to convert that asset path into a concrete file path, so a Path instance.

So coming at it from the other angle, FileWatcher should be dealing with concrete paths because it is listening for real file system events for a particular directory. So I think we should revert what's happening in this file_watcher mod.

andriyDev · 2025-06-01T06:43:53Z

crates/bevy_asset/src/path.rs

@@ -255,7 +288,7 @@ impl<'a> AssetPath<'a> {
    /// Gets the path to the asset in the "virtual filesystem".
    #[inline]
    pub fn path(&self) -> &Path {


I think we should remove this in this PR. It will be more changes, but it will ensure we aren't accidentally converting to paths somewhere.

andriyDev · 2025-06-01T06:48:03Z

crates/bevy_asset/src/path.rs

+            return true;
+        }
+        let mut simplified = Vec::new();
+        for component in self.path_components() {


I don't have a strong opinion here, leaning to no for simplicity?

andriyDev · 2025-06-01T06:58:04Z

crates/bevy_asset/src/path.rs

-pub(crate) fn normalize_path(path: &Path) -> PathBuf {
-    let mut result_path = PathBuf::new();
-    for elt in path.iter() {
+pub(crate) fn normalize_path<'b>(path: &'b [&'b str]) -> Vec<&'b str> {


I might be missing something... It seems like we don't normalize our asset paths today. I think that means if you load "asset.txt" and "blah/../asset.txt", they are treated as different assets.

Doesn't need to be addressed in this PR, just realized this grossness.

lielfr · 2025-06-08T19:33:42Z

sorry for the delay.
Regarding file_watcher, I tried to change normalize_path to be generic, like this:

pub(crate) fn normalize_path<'b, P>(path: &'b [P]) -> Vec<P>
where
    P: Into<&'b str> + Copy,
{
    let mut result_path = Vec::new();
    for &elt in path {
        if elt.into() == "." {
            // Skip
        } else if elt.into() == ".." {
            if result_path.is_empty() {
                // Preserve ".." if insufficient matches (per RFC 1808).
                result_path.push(elt);
            } else {
                result_path.pop();
            }
        } else {
            result_path.push(elt);
        }
    }
    result_path
}

but it doesn't seem to work. Will it be ok to create a separate function for PathBuf? or am I missing something?

lielfr · 2025-06-08T21:05:46Z

I tried to change the readers to use AssetPath instead of Path, but so far have not found a way out of lifetime hell

…or Path replacement methods

… encoding, test for empty vs root paths

lielfr · 2025-06-11T23:09:55Z

rebased from main again

alice-i-cecile requested review from cart and andriyDev May 9, 2025 00:41

alice-i-cecile added A-Assets Load files from disk to use for things like images, models, and sounds C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 9, 2025

alice-i-cecile requested a review from bushrat011899 May 9, 2025 00:42

bushrat011899 reviewed May 9, 2025

View reviewed changes

andriyDev reviewed May 9, 2025

View reviewed changes

mcobzarenco reviewed May 9, 2025

View reviewed changes

crates/bevy_asset/src/io/embedded/embedded_watcher.rs Outdated Show resolved Hide resolved

crates/bevy_asset/src/io/source.rs Outdated Show resolved Hide resolved

crates/bevy_asset/src/path.rs Outdated Show resolved Hide resolved

crates/bevy_asset/src/path.rs Outdated Show resolved Hide resolved

lielfr force-pushed the asset-path-nostd branch from 8c5f81c to 97ba16a Compare May 9, 2025 22:18

mcobzarenco reviewed May 10, 2025

View reviewed changes

lielfr force-pushed the asset-path-nostd branch from 006f751 to 2a4df37 Compare May 11, 2025 00:11

andriyDev mentioned this pull request Jun 1, 2025

Bare Minimum no_std support for bevy_asset #19070

Open

andriyDev reviewed Jun 1, 2025

View reviewed changes

lielfr force-pushed the asset-path-nostd branch from 2a4df37 to 643a1aa Compare June 8, 2025 20:42

lielfr added 5 commits June 12, 2025 02:08

change AssetPath to use &str

3d03097

remove more usages of PathBuf

4ef08b7

cleanup

48e0691

fix lints

c205b00

get rid of unused function and use string version instead

3adf527

lielfr added 9 commits June 12, 2025 02:08

remove itertools unused dependency

af99513

fix build in embedded_watcher

ec47b4c

remove lossy conversions, fix linting and doc tests, add more tests f…

b1733fd

…or Path replacement methods

PR fixes - constants for path separator, UTF-8 instead of unicode for…

449d2ac

… encoding, test for empty vs root paths

fix build

d831e32

add file_name method

2a48d7c

AssetReaderError: move from PathBuf to String

da9ecbc

get rid of more path function usages from AssetPath

f73f88f

remove unused imports

27e9bde

lielfr force-pushed the asset-path-nostd branch from 0ee520c to 27e9bde Compare June 11, 2025 23:08

Uh oh!

Move away from std::Path in bevy_assets #19133

Are you sure you want to change the base?

Move away from std::Path in bevy_assets #19133

Conversation

lielfr commented May 9, 2025

Objective

Solution

Testing

Uh oh!

github-actions bot commented May 9, 2025

Uh oh!

alice-i-cecile commented May 9, 2025

Uh oh!

lielfr commented May 9, 2025

Uh oh!

bushrat011899 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bushrat011899 commented May 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viridia Jun 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcobzarenco left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lielfr commented May 9, 2025

Uh oh!

lielfr commented May 9, 2025

Uh oh!

mcobzarenco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lielfr commented May 11, 2025

Uh oh!

viridia Jun 1, 2025 •

edited

Loading

mcobzarenco left a comment •

edited

Loading

lielfr commented Jun 8, 2025 •

edited

Loading