Replace PathBuf-based AssetPath resolution with URL-style segment resolver #22599
Conversation
Use a URL-style resolver (split on /, normalize . and ..) so resolve and resolve_embed are platform-independent. Stops PathBuf from applying filesystem rules (e.g. drive letters, backslash). Add tests for :, \\, rooted .., and a//b.
Tidy docs so identifiers and function names use backticks for doc_markdown. Gate normalize_path on the file_watcher feature so it is only built when that feature is enabled and is no longer reported as dead when it is off, avoiding allow(dead_code).
IQuick143
added
C-Bug
andriyDev
left a comment
andriyDev
left a comment
There was a problem hiding this comment.
I think this is fine, and gets us one step closer to AssetPath storing String instead of PathBuf.
crates/bevy_asset/src/path.rs
Outdated
| /* | ||
| "a/b" -> (false, ["a", "b"]) | ||
| "/a/b" -> (true, ["a", "b"]) | ||
| "C:file" -> (false, ["C:file"]) (no split on :) | ||
| "a\\b" -> (false, ["a\\b"]) (no split on \) | ||
| "a//b" -> ["a","","b"] | ||
| */ |
There was a problem hiding this comment.
This comment seems redundant with the test?
There was a problem hiding this comment.
kinda yes, it was a blank comment I made to know what I wanna do myself and forgot to remove it. Sorry about that.
crates/bevy_asset/src/path.rs
Outdated
| /* | ||
| function that splits only on /, and returns (bool, Vec<&str>): | ||
| bool = path starts with / | ||
| Vec<&str> = segments between / (define once whether you keep or drop "" from // or trailing / and stick to it) | ||
| */ |
There was a problem hiding this comment.
Can we make this a proper doc-comment?
There was a problem hiding this comment.
totally agree, on it
…9) in a more doc-proper way
|
Since there is no other modifications I guess it is ready to be merged right? or there is a remaining review? @andriyDev |
|
This still needs a second review. Anything not trivial needs two community reviews and then a final review from a maintainer. |
3 participants
Summary
This PR fixes #22420
Asset path resolution no longer relies on
PathBuf/Pathfor joining and normalization.It now uses URL-style resolver that operates only on
/-split path segments, ensuring identical behavior across platforms and avoiding filesystem-specific semantics.AssetPathis not a filesystem path. UsingPathBufintroduced platform-dependent behavior like Windows drive letters and backslashes.This change makes resolution predictable, testable, and explicit.
How it works
1. Splitting (only
/)split_asset_path_segments(path: &str) -> (is_rooted, segments)/:,\, and//are not specialExamples:
a/b->(false, ["a", "b"])/a/b->(true, ["a", "b"])C:file->(false, ["C:file"])a\b->(false, ["a\\b"])a//b->(false, ["a", "", "b"])2. Normalization (
.and..)normalize_asset_path_segments(segments, is_rooted).removed..pops previous segment when possible, otherwise preserved/a/b + ../c → /a/c/a + ../b → /b3. Joining & resolving
join_and_normalize_asset_path(...)resolve_embedreplace semantics/added (existing behavior preserved)4. resolve_from_parts
PathBufusage isPathBuf::from(resolved)at the endPathBuf::from(self.path())PathBuf::pushPath::strip_prefixnormalize_pathWhat stays the same
normalize_pathunchangedresolve,resolve_embed..underflowAssetPathstill stores aPath; only computation changedNew internal helpers
split_asset_path_segments(path_str: &str) -> (bool, Vec<&str>)normalize_asset_path_segments(segments: &[&str], is_rooted: bool) -> Vec<String>join_and_normalize_asset_path(...) -> StringTests
Unit
split_asset_path_segments:a/b,/a/b,C:file,a\b,a//bnormalize_asset_path_segments:.,.., underflow, rootedjoin_and_normalize_asset_path: replace, trailing slash, rootedRegression
C:file,a:b→a/b/C:file,a/b/a:bx\y,x\y/z→a/b/x\y,a/b/x\y/z..:/a/b + ../c → /a/c/a + ../b → /b/preserved:a//b → x/a//bCheck
cargo test -p bevy_asset(incl. doctests) passes