Move away from std::Path in bevy_assets

[lielfr]

Objective

• closes #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[bot]]

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]

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]

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]

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:

1. Dyn-compatible traits can't use impl T in arguments, so they need a concrete type to refer to.
2. 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.

[lielfr]

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]

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

[lielfr]

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]

See also this comment by cart from a couple years ago: https://github.com/bevyengine/bevy/pull/9528#issuecomment-1769212279

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.

[lielfr]

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]

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

[lielfr]

rebased from main again