Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add AssetPath::resolve() #9473

Closed
viridia opened this issue Aug 17, 2023 · 3 comments · Fixed by #9528
Closed

Add AssetPath::resolve() #9473

viridia opened this issue Aug 17, 2023 · 3 comments · Fixed by #9528
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Usability A simple quality-of-life change that makes Bevy easier to use D-Trivial Nice and easy! A great choice to get started with Bevy

Comments

@viridia
Copy link
Contributor

viridia commented Aug 17, 2023

What problem does this solve or what need does it fill?

Assets which contain references to other assets should support relative paths. This enables an entire group of linked assets to be moved around the filesystem without breaking the relationship.

To facilitate this, I propose adding a method to AssetPath which accepts a relative path string, and produces a new absolute path. Asset loaders can then use this method when resolving internal path references.

What solution would you like?

The resolve() method would take a single argument, which is a relative path string. It produces a new path using self as the base path, with the following rules:

  • If the relative path starts with "#", then the new path consists of the base path with the label portion replaced. This is used for paths that point to other assets within the same file.
  • If the relative path starts with "./" or "../", then the new path is computed relative to the directory of the asset path, using the standard path resolution rules. So if the base path is "a/b/c#d", and the relative path is "./x" then the new path is "a/b/x".
  • If the relative path starts with any other character, it is treated as an absolute path.

What alternative(s) have you considered?

This can also be done as a standalone function, however it's a pretty handy function to have, and is small, so I thought it would make sense to add this to Bevy.

@viridia viridia added C-Enhancement A new feature S-Needs-Triage This issue needs to be labelled labels Aug 17, 2023
@alice-i-cecile alice-i-cecile added A-Assets Load files from disk to use for things like images, models, and sounds C-Usability A simple quality-of-life change that makes Bevy easier to use D-Trivial Nice and easy! A great choice to get started with Bevy and removed C-Enhancement A new feature S-Needs-Triage This issue needs to be labelled labels Aug 17, 2023
@bushrat011899
Copy link
Contributor

I think this is pretty related to #9458, and a similar approach to the solution in #9478 could be used to generate the new absolute path using pathdiff::diff_paths. This would remove the burden from Bevy to discern relative vs absolute paths, and have divergent behaviour based on that.

@viridia
Copy link
Contributor Author

viridia commented Aug 18, 2023

I think this is pretty related to #9458, and a similar approach to the solution in #9478 could be used to generate the new absolute path using pathdiff::diff_paths. This would remove the burden from Bevy to discern relative vs absolute paths, and have divergent behaviour based on that.

I'm not sure about that. I think we may be talking about different definitions of "absolute". Bevy asset paths are in some ways more like URIs than filesystem paths, although they do get translated into filesystem paths eventually. For example, the asset label #name is more like a URI fragment identifier, and is undoubtedly inspired by that. Thus, when I see an asset key that consists of just the string "#name", I generally would assume that this means a labeled asset within the same file as the current asset. Filesystem paths have no such feature.

Perhaps we need new terminology. I was using the word "absolute" to mean a full asset descriptor, relative to the "base" asset directory; I wasn't referring to a path that had a window drive letter or something equivalent.

The other issue with using a filesystem library for manipulating paths is whether or not this entails platform-specific behavior, especially around backslashes. I'll admit that I don't know a lot about what the Bevy team intends, but I imagine that it would be a better developer experience to have asset paths be platform-independent, and let the asset manager handle the details of transforming them into platform paths.

@viridia
Copy link
Contributor Author

viridia commented Aug 21, 2023

I'm thinking about taking a crack at this myself, since I've already written this function (and tests) for my own app. Mainly I just need to turn it into a method on AssetPath instead of a standalone function.

However, I haven't contributed to Bevy before and might need some hand-holding. I guess asking on the Discord is the best way to do that.

One question: my implementation only handles "./" and "../" at the beginning of the relative path - it doesn't try to canonicalize relative paths that internally contain "/../" (and I'm not sure why anyone would make a path like this). Do you think that's good enough?

github-merge-queue bot pushed a commit that referenced this issue Oct 26, 2023
# Objective

Fixes #9473 

## Solution

Added `resolve()` method to AssetPath. This method accepts a relative
asset path string and returns a "full" path that has been resolved
relative to the current (self) path.

---------

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
ameknite pushed a commit to ameknite/bevy that referenced this issue Nov 6, 2023
# Objective

Fixes bevyengine#9473 

## Solution

Added `resolve()` method to AssetPath. This method accepts a relative
asset path string and returns a "full" path that has been resolved
relative to the current (self) path.

---------

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this issue Jan 9, 2024
# Objective

Fixes bevyengine#9473 

## Solution

Added `resolve()` method to AssetPath. This method accepts a relative
asset path string and returns a "full" path that has been resolved
relative to the current (self) path.

---------

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Usability A simple quality-of-life change that makes Bevy easier to use D-Trivial Nice and easy! A great choice to get started with Bevy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants