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

Consistent AssetPath Format #10511

Open
cart opened this issue Nov 11, 2023 · 3 comments
Open

Consistent AssetPath Format #10511

cart opened this issue Nov 11, 2023 · 3 comments
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Feature A new feature, making something new possible

Comments

@cart
Copy link
Member

cart commented Nov 11, 2023

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

Currently AssetPath is backed by Path, which is great if the goal is to support arbitrary filesystem paths across operating systems:

  • Windows: \foo\bar, C:\foo\bar
  • Everything Else: /foo/bar, /mnt/c/foo/bar

However that is not the goal of AssetPath. AssetPath exists to be an opinionated "cross platform virtual asset filesystem with support for multiple arbitrary asset sources and asset labels, which serves as a unique canonical asset identifier". Ideally, you can compare two "absolute AssetPaths", and if they are not equal, they are not the same asset.

This mismatch in goals can create problems. For example, when we create AssetPaths from hot-reload events on Windows, they come in the form \foo\bar.png. However users tend to use /foo/bar.png (and we encourage this pattern everywhere), which can create problems when considering paths as strings (like it did here #10500). As a cross-platform canonical path format, AssetPath has no business supporting windows-only drive letter syntax, as that location is not directly expressible on other operating systems. AssetPath already has a cross platform way to express other mounted filesystem locations (some_asset_source://).

What solution would you like?

We should build a new, consistent path abstraction for AssetPath and replace the current Path usage with that. It should support only / and be designed with the explicit goal of providing consistent canonical identifiers.

Like the current AssetPath impl, it should avoid allocating when a &static str is used to construct it, and it should have true copy on write behavior (#9729).

This will create a new problem: users that want to support absolute paths (ex: C:\path\to\image.png) can no longer directly use the default AssetSource (which is rooted at the local asset folder)). We need a way to support this, in the interest of building apps like "file editors" or loading scenes from arbitrary locations on disk.

I believe this should be accomplished via a combination of two approachs:

  1. App developers can already choose what the "default" asset source is. They can mount the root of the C drive on windows (and / on every other platform), enabling absolute paths to work by default (without the windows drive syntax).
  2. For windows "drive syntax", App developers can mount those drives as asset sources with equivalent names (ex: The AssetPath A://some/path.png would map to the windows path A:\some\path.png). We could then build AssetPath::from(Path) in a way that explicitly handles windows drive syntax and converts it to the equivalent AssetPath. We could also consider building "automount" functionality that will mount windows drives when they are requested.

One issue with (2) is that Bevy Asset V2 does not currently support mounting new asset sources at runtime. We'd almost certainly want to support that, as new drives can be connected at arbitrary times when an app is running.

@cart cart added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled A-Assets Load files from disk to use for things like images, models, and sounds and removed S-Needs-Triage This issue needs to be labelled labels Nov 11, 2023
@alice-i-cecile alice-i-cecile modified the milestone: 0.13 Nov 11, 2023
@afonsolage
Copy link
Contributor

afonsolage commented Nov 12, 2023

Windows supports both paths separators since Windows 7 IIRC. For instance, even on MS-DOS, I can navigate fine using forward slash:
image
So even things like cd / or cd C:/Users/ also works fine.

The problem is the returned path from Win32 API, which due to naming conventions, uses backslash \.

There are exceptions to this, mostly UNC, like:

  1. Volume names paths: \\?\Volume{26a21bda-a627-11d7-9931-806e6f6e6963}\
  2. Remote paths: \\my-network-server\something
  3. WSL paths: \\wsl.localhost\Ubuntu\home\my-user
  4. Serial ports: \\.\COM56
  5. Paths longer than MAX_PATH: \\?\C:\A\Really\Long\Path\With\Many\Sub\Dir\Subdirectories\And\BIGGER_FILE_NAME.AND_EVEN_BIGGER_EXTENSION

All of these can be solved on user side, by having a mounting letter on Windows. Let's say, for some reason, I want an asset from inside my WSL, which is located at /home/afonsolage/assets/, I could just do in MS-DOS: net use w: \\wsl.localhost\Ubuntu and from Bevy asset, use w:/home/afonsolage/assets/.

With all that being said, we probably still have to do something if we want to support paths returned from Win32 API, like when reading all files from a folder, WinAPI will use \ on returned paths, so we must take care of them. Also on future bevy_editor, this will have to be taken care. I saw too many times mixed paths like C:\Users\afonsolage/some_folder/path.

So as long Bevy new AssetPath take care of always converting \ to / and users don't use UNC directly, but through mount letter, we shouldn't require Bevy to support \ only /.

@viridia
Copy link
Contributor

viridia commented Nov 12, 2023

Internally, Path is an OSString - would the new non-Path-based AssetPath still keep this instead of regular String? I assume so.

The approach I was contemplating was to replace both Path and PathBuf with Bevy-specific types, although you might not need Path, instead AssetPath can just use an OSString directly. The resolve() method will need something akin to PathBuf, but it can be much, much simpler (resolve already does most of the work, we really just need push and pop).

Here's how I would proceed on this:

  1. Identify what the API should look like. We want to change the API as little as possible, but some things will likely have to change.
  2. Clearly delineate the places where we convert filesystem paths to AssetPaths and vice versa. Make sure we have comprehensive unit tests for those cases.
  3. If we need an AssetPathBuf for constructing paths from components, implement that and write tests for it.
  4. Replace Path with OSString in AssetPath. Re-write methods such as .parent() which need to scan the path for separators. Methods which need to return a Path will have to add conversion logic.

@viridia
Copy link
Contributor

viridia commented Jan 10, 2024

@cart It's still not clear to me whether the internal representation of a path should be str or OsStr. There are arguments in favor of both:

  • Most of the time, AssetPath is constructed from a str, and parse_internal operates on a str buffer. So this would argue in favor of it being str.
  • However, at some point the path is actually going to be converted into a filesystem path, which argues in favor of using OsStr.

Implementation-wise, str is probably slightly easier, but only slightly. There are basically 14 places in the code that need to be changed, all in one file.

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-Feature A new feature, making something new possible
Projects
None yet
Development

No branches or pull requests

4 participants