Add AssetPath::resolve_path and resolve_embed_path methods #22416

Eyad3skr · 2026-01-07T12:27:38Z

Objective

Solution

Added resolve_path(&AssetPath) - equivalent to resolve(&str)
Added resolve_embed_path(&AssetPath) - equivalent to resolve_embed(&str)
Refactored resolve_internal to use shared resolve_from_parts helper
Added unit tests for equivalence
All semantic edge cases preserved (eg., label-only, or leading /)
No behavior changes to existing methods

Testing

Added 2 new equivalence tests proving resolve_path() ≡ resolve(&path.to_string())
All 20 tests pass: cargo test -p bevy_asset --lib path::tests
Test covers: relative paths, labels, leading /, explicit sources, RFC 1808 embed behavior

How to test

cargo test -p bevy_asset --lib path::tests

These new methods accept &AssetPath instead of &str, avoiding the need to stringify and re-parse when the caller already has an AssetPath. Fixes bevyengine#22239

github-actions · 2026-01-07T12:27:48Z

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 · 2026-01-07T20:01:10Z

Very clean work; thank you :)

viridia · 2026-01-07T20:07:05Z

My only concern is that this propagates a pattern that we've been wanting to get rid of for a long time (but have never got around to fixing).

Specifically, AssetPath::resolve should not use PathBuf. PathBuf does a lot of crazy things - like parsing windows drive letters - which simply don't apply to Bevy asset paths, and which are a potential source of bugs. It would be much better to have our own path resolution algorithm that operates on string slices, this would be fairly easy to write, easy to test, and the behavior would be predictable.

PathBuf makes sense when we are talking about actual file paths: that is, when an asset path gets transformed into a path used to actually open files. This happens inside the filesystem asset source, and is not part of the higher-level asset framework.

But asset paths themselves are not file paths, they are more like URLs.

Eyad3skr · 2026-01-07T20:26:40Z

Thanks @alice-i-cecile for the compliment and thanks @viridia for the detailed feedback, that actually makes sense.

I agree AssetPath is closer to a URL-like path than a filesystem path, and PathBuf/std::path normalization brings platform-specific behavior (e.g. Windows prefixes / drive letters) that shouldn’t apply at the asset-path layer.

This PR intentionally kept behavior identical to the existing resolve / resolve_embed implementation, and the new resolve_*_path methods reuse the same internal logic to avoid any semantic changes. That said, I understand the concern that refactoring here may further entrench the PathBuf-based approach.

I thought of 2 things could be done rn :

keep this PR scoped to the API improvement (accepting &AssetPath + equivalence tests), and I open a follow-up issue/PR to replace the resolution algorithm with a string-slice implementation, or
expand this PR to replace the PathBuf usage as part of the same change?

I’m happy to do either..

viridia · 2026-01-07T20:33:03Z

If we commit to doing a follow-up PR then it would alleviate my concerns.

Shatur

I think we can sneak this into 0.18 since it's not a breaking change, but I'd probably rename old methods into *_str and keep path variants as "default" in a follow-up.

Shatur · 2026-01-07T20:41:54Z

crates/bevy_asset/src/path.rs

+    ) -> AssetPath<'static> {
+        let mut base_path = PathBuf::from(self.path());
+        if replace && !self.path.to_str().unwrap().ends_with('/') {
+            // No error if base is empty


Why (per RFC 1808) in the comment was removed?

Shatur · 2026-01-07T20:43:33Z

crates/bevy_asset/src/path.rs

+    /// Resolves `path` relative to `self`, using the same rules as [`AssetPath::resolve`],
+    /// but without reparsing from a string.


Maybe change the description of resolve instead and mention that it parses the asset path from the string?

Eyad3skr · 2026-01-07T20:46:11Z

@viridia Tracking issue opened: #22420 . I’ll follow up with a PR replacing PathBuf resolution with a deterministic segment-based resolver.

alice-i-cecile · 2026-01-07T20:53:52Z

@Eyad3skr if you can tackle @Shatur's comments briefly I'll be happy to merge this and wait for the follow-up issue to address the PathBuf concerns :)

Eyad3skr · 2026-01-07T20:54:40Z

@Shatur @alice-i-cecile On it

…suit the method name change occured. replacing back the RFC 1808 reference.

Shatur · 2026-01-07T21:46:12Z

I meant to rename in a follow-up PR some time later because this would potentially allow us to include this change in 0.18. But if it's easier for you to do it now - it's okay, not that important to have this in 0.18 🙂

Eyad3skr · 2026-01-07T21:49:23Z

@Shatur oh really, sorry didn't catch this up! anyway I'm up for the night dodging studying for data engineering final, it is ok.

and sorry for the many mentions, we are more than 2 in the discussion, just so I don't mix things up.

abdlla3amer · 2026-01-08T01:22:11Z

Wonderful analogy with a high-level end-to-end object oriented solution Mr. @Eyad3skr

Eyad3skr · 2026-01-08T01:46:52Z

anybody got an idea of why CI / markdownlint (pull_request) fails?

…s CI test

alice-i-cecile · 2026-01-08T04:30:25Z

Markdown errors can be found starting https://github.com/bevyengine/bevy/actions/runs/20803366790/job/59752575411?pr=22416#step:4:156; you need to expand the markdown section when looking at the job :)

Thanks for your work here; I'll merge this once CI is passing!

Shatur · 2026-01-08T11:11:39Z

release-content/migration-guides/assetpath-resolve-semantics.md

+pull_requests: [22416]
+---
+
+# `AssetPath::resolve` and `resolve_embed` now take `&AssetPath`


I'd suggest to take a look at how other people write migration guides: https://github.com/bevyengine/bevy/tree/main/release-content/migration-guides
It's a bit verbose for such a simple change. I wouldn't even add "Before" and "After".

@Eyad3skr I didn't mean to remove the labels. I meant to remove the whole blocks with the before and after code.

so migration part I just write it in plain English?

Yeah, just a few sentences should be enough to explain the breaking change. Just take a look at the link I shared above.

passed it finally.. the linter was confused between the real title from template and my text because I used H1 after the title, so I reduced it to H2

@Shatur Ok I will see now

I think it's still overly verbose. Take a look at the migration guides I linked to you. You don't need headings, just write in plain English what was done (no need to write "# What was done"), how to migrate and why. It's obvious that if you have a string, it's more convenient to use the *_str methods.

fair enough, made it

Shatur · 2026-01-08T11:51:29Z

release-content/migration-guides/assetpath-resolve-semantics.md

+
+`AssetPath::resolve` and `AssetPath::resolve_embed` no longer accept `&str` and now take `&AssetPath` directly. The previous string-based APIs have been renamed to `resolve_str` and `resolve_embed_str`.
+
+This change avoids unnecessary string allocation and parsing when an `AssetPath` is already available. To migrate, pass an `AssetPath` directly to `resolve` or `resolve_embed`; when working with strings, use the corresponding `*_str` methods instead. No behavioral or semantic changes were introduced.


Almost perfect, last tiny change, can be applied directly from the browser 🙂 I think the last sentence doesn't add anything.

Suggested change

This change avoids unnecessary string allocation and parsing when an `AssetPath` is already available. To migrate, pass an `AssetPath` directly to `resolve` or `resolve_embed`; when working with strings, use the corresponding `*_str` methods instead. No behavioral or semantic changes were introduced.

This change avoids unnecessary string allocation and parsing when an `AssetPath` is already available. To migrate, pass an `AssetPath` directly to `resolve` or `resolve_embed`; when working with strings, use the corresponding `*_str` methods instead.

made it, the linter failed anyway, needed a trailing newline

@Shatur btw as for the issue #22420, ig it needs to be labeled and see if my description is right so I can go on with it?

I'll leave the guiding for #22420 to @viridia .

I want to note that PathBuf was already used and you just moved it into a separate method to re-use the logic for AssetPath. So you did a great job.
#22420 it's a separate, but related issue.

Perfect, waiting for any updates. I'm trying to catch up anyway so I got no problem getting assigned this one too, thanks for the review and your effort!

Shatur

Perfect now! Thank you.

Eyad3skr · 2026-01-08T11:59:19Z

Perfect now! Thank you.

Always! Thank you for the review, much appreciated!

Shatur · 2026-01-08T19:20:02Z

@alice-i-cecile this PR includes a breaking change, so we probably have to remove it from 0.18 😢
I suggested this one for 0.18 originally, but the author also included a rename. I don't think it's worth backporting the change without the rename for 0.18.

Eyad3skr added 2 commits January 7, 2026 14:16

Add AssetPath::resolve_path and resolve_embed_path methods

8c2cda9

These new methods accept &AssetPath instead of &str, avoiding the need to stringify and re-parse when the caller already has an AssetPath. Fixes bevyengine#22239

Add AssetPath::resolve_path and resolve_embed_path methods

828504a

These new methods accept &AssetPath instead of &str, avoiding the need to stringify and re-parse when the caller already has an AssetPath. Fixes bevyengine#22239

Eyad3skr added 2 commits January 7, 2026 14:54

fixing formatting with cargo fmt --all

2fa8817

removed blank space between docs and methods I created

61098c1

alice-i-cecile requested review from Shatur and viridia January 7, 2026 19:58

alice-i-cecile approved these changes Jan 7, 2026

View reviewed changes

alice-i-cecile added X-Uncontroversial This work is generally agreed upon D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Jan 7, 2026

Eyad3skr mentioned this pull request Jan 7, 2026

Replace PathBuf-based resolution in AssetPath::resolve with URL-like segment resolver #22420

Open

Shatur suggested changes Jan 7, 2026

View reviewed changes

viridia approved these changes Jan 7, 2026

View reviewed changes

alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 7, 2026

String variants renamed to *_str., modified namings in unit tests to …

34bd56e

…suit the method name change occured. replacing back the RFC 1808 reference.

Fix resolve_embed calls to use resolve_embed_str

5373d9b

fixing md linting error for ci in assetpath-resolve-semantics.md

65934b1

Eyad3skr added 2 commits January 8, 2026 03:25

confusing explanation of migaration fixed and removed blank space

005a371

linter is not linting

bfeeb6d

viridia approved these changes Jan 8, 2026

View reviewed changes

Eyad3skr added 3 commits January 8, 2026 04:06

linter may lint this time

b44f587

docs: modify migration guide for AssetPath::resolve API change to pas…

4e107a0

…s CI test

guidline title: '' to double-quotes

8343031

alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jan 8, 2026

Shatur suggested changes Jan 8, 2026

View reviewed changes

Eyad3skr added 5 commits January 8, 2026 13:18

fix markdown lint spacing

6b0b8c6

removed before and after

5fe21a8

reduced title from H1 to H2

7e69298

removed code block in md migration guideline

4fa10f3

last but not least everyone

1abc2f6

Shatur suggested changes Jan 8, 2026

View reviewed changes

Eyad3skr added 2 commits January 8, 2026 13:52

adding a trailing newline

ba14cda

adding a trailing newline

ede5156

Shatur approved these changes Jan 8, 2026

View reviewed changes

alice-i-cecile added this pull request to the merge queue Jan 8, 2026

alice-i-cecile added this to the 0.18 milestone Jan 8, 2026

Merged via the queue into bevyengine:main with commit adba91a Jan 8, 2026
38 checks passed

cart modified the milestones: 0.18, 0.19 Jan 9, 2026

		/// Resolves `path` relative to `self`, using the same rules as [`AssetPath::resolve`],
		/// but without reparsing from a string.


		`AssetPath::resolve` and `AssetPath::resolve_embed` no longer accept `&str` and now take `&AssetPath` directly. The previous string-based APIs have been renamed to `resolve_str` and `resolve_embed_str`.

		This change avoids unnecessary string allocation and parsing when an `AssetPath` is already available. To migrate, pass an `AssetPath` directly to `resolve` or `resolve_embed`; when working with strings, use the corresponding `*_str` methods instead. No behavioral or semantic changes were introduced. No newline at end of file

Uh oh!

Add AssetPath::resolve_path and resolve_embed_path methods #22416

Add AssetPath::resolve_path and resolve_embed_path methods #22416

Conversation

Eyad3skr commented Jan 7, 2026

Objective

Solution

Testing

How to test

Uh oh!

github-actions bot commented Jan 7, 2026

Uh oh!

alice-i-cecile commented Jan 7, 2026

Uh oh!

viridia commented Jan 7, 2026

Uh oh!

Eyad3skr commented Jan 7, 2026

Uh oh!

viridia commented Jan 7, 2026

Uh oh!

Shatur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shatur Jan 7, 2026 • 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!

Eyad3skr commented Jan 7, 2026

Uh oh!

alice-i-cecile commented Jan 7, 2026

Uh oh!

Eyad3skr commented Jan 7, 2026

Uh oh!

Shatur commented Jan 7, 2026

Uh oh!

Eyad3skr commented Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abdlla3amer commented Jan 8, 2026

Uh oh!

Eyad3skr commented Jan 8, 2026

Uh oh!

alice-i-cecile commented Jan 8, 2026

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!

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!

Shatur Jan 8, 2026 • 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!

Eyad3skr Jan 8, 2026 • 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!

Shatur Jan 8, 2026 • 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!

Shatur Jan 7, 2026 •

edited

Loading

Eyad3skr commented Jan 7, 2026 •

edited

Loading

Shatur Jan 8, 2026 •

edited

Loading

Eyad3skr Jan 8, 2026 •

edited

Loading

Shatur Jan 8, 2026 •

edited

Loading