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

Fix panic when using .load_folder() with absolute paths #9490

Merged
merged 1 commit into from
Aug 28, 2023

Conversation

rlidwka
Copy link
Contributor

@rlidwka rlidwka commented Aug 19, 2023

Fixes #9458.

On case-insensitive filesystems (Windows, Mac, NTFS mounted in Linux, etc.), a path can be represented in a multiple ways:

  • c:\users\user\rust\assets\hello\world
  • c:/users/user/rust/assets/hello/world
  • C:\USERS\USER\rust\assets\hello\world

If user specifies a path variant that doesn't match asset folder path bevy calculates, path.strip_prefix() will fail, as demonstrated below:

dbg!(Path::new("c:/foo/bar/baz").strip_prefix("c:/foo"));
// Ok("bar/baz")

dbg!(Path::new("c:/FOO/bar/baz").strip_prefix("c:/foo"));
// StripPrefixError(())

This commit rewrites the code in question in a way that prefix stripping is no longer necessary.

I've tested with the following paths on my computer:

let res = asset_server.load_folder("C:\\Users\\user\\rust\\assets\\foo\\bar");
dbg!(res);

let res = asset_server.load_folder("c:\\users\\user\\rust\\assets\\foo\\bar");
dbg!(res);

let res = asset_server.load_folder("C:/Users/user/rust/assets/foo/bar");
dbg!(res);

On case-insensitive filesystems (Windows, Mac, NTFS mounted
in Linux, etc.), a path can be represented in a multiple ways:

 - `c:\users\user\rust\assets\hello\world`
 - `c:/users/user/rust/assets/hello/world`
 - `C:\USERS\USER\rust\assets\hello\world`

If user specifies a path variant that doesn't match asset folder path
bevy calculates, `path.strip_prefix()` will fail, as demonstrated
below:

```rs
dbg!(Path::new("c:/foo/bar/baz").strip_prefix("c:/foo"));
// Ok("bar/baz")

dbg!(Path::new("c:/FOO/bar/baz").strip_prefix("c:/foo"));
// StripPrefixError(())
```

This commit rewrites the code in question in a way that prefix
stripping is no longer necessary.
@github-actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

@bushrat011899
Copy link
Contributor

bushrat011899 commented Aug 19, 2023

Just tested some examples, nice change! As a note, it does change the normal behaviour of read_directory when given a path that is relative to the root. Now it will always return absolute paths instead of paths relative to AssetIo::root_path. As the examples still work I don't think this is a breaking change, but it does change how this function works beyond just fixing the panic!.

Edit: This was mistaken on my part, please disregard!

@rlidwka
Copy link
Contributor Author

rlidwka commented Aug 19, 2023

Now it will always return absolute paths instead of paths relative to AssetIo::root_path.

It will return relative path if path parameter is relative (that's the most common case, and there're no changes there).

It will return absolute path if path parameter is absolute (before it was sometimes relative, and sometimes panic).

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Assets Load files from disk to use for things like images, models, and sounds labels Aug 19, 2023
@alice-i-cecile
Copy link
Member

@bushrat011899 if you're happy with the change and think it's ready to merge, can you please leave a review using Github's system? Bevy works on "community reviews", and your opinion will get us halfway there to merging this :)

Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simple solution, thanks for making it!

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Aug 20, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 28, 2023
Merged via the queue into bevyengine:main with commit 94c67af Aug 28, 2023
23 of 24 checks passed
@Shatur
Copy link
Contributor

Shatur commented Aug 28, 2023

Great, this fixes #5770.

rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
…#9490)

Fixes bevyengine#9458.

On case-insensitive filesystems (Windows, Mac, NTFS mounted in Linux,
etc.), a path can be represented in a multiple ways:

 - `c:\users\user\rust\assets\hello\world`
 - `c:/users/user/rust/assets/hello/world`
 - `C:\USERS\USER\rust\assets\hello\world`

If user specifies a path variant that doesn't match asset folder path
bevy calculates, `path.strip_prefix()` will fail, as demonstrated below:

```rs
dbg!(Path::new("c:/foo/bar/baz").strip_prefix("c:/foo"));
// Ok("bar/baz")

dbg!(Path::new("c:/FOO/bar/baz").strip_prefix("c:/foo"));
// StripPrefixError(())
```

This commit rewrites the code in question in a way that prefix stripping
is no longer necessary.

I've tested with the following paths on my computer:

```rs
let res = asset_server.load_folder("C:\\Users\\user\\rust\\assets\\foo\\bar");
dbg!(res);

let res = asset_server.load_folder("c:\\users\\user\\rust\\assets\\foo\\bar");
dbg!(res);

let res = asset_server.load_folder("C:/Users/user/rust/assets/foo/bar");
dbg!(res);
```
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-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

load_folder() panics when given an absolute path but load() does not
4 participants