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

load_folder() panics when given an absolute path but load() does not #9458

Closed
iliags opened this issue Aug 16, 2023 · 4 comments · Fixed by #9490
Closed

load_folder() panics when given an absolute path but load() does not #9458

iliags opened this issue Aug 16, 2023 · 4 comments · Fixed by #9490
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-Implementation This issue is ready for an implementation PR. Go for it!

Comments

@iliags
Copy link

iliags commented Aug 16, 2023

Bevy version

0.11

The issue is currently present in the main branch as well:
https://github.com/bevyengine/bevy/blob/b6a9d8eba7807b63be525e423911c0217706f13a/crates/bevy_asset/src/io/file_asset_io.rs#L112C4-L123C6

What you did

server.load_folder("C:\\Absolute\\Path\\To\\Folder\\")

What went wrong

thread 'Compute Task Pool (3)' panicked at 'called `Result::unwrap()` on an `Err` value: StripPrefixError(())', C:\<user>\.cargo\registry\src\index.crates.io-6f17d22bba15001f\bevy_asset-0.11.0\src\io\file_asset_io.rs:120:47
  • What were you expecting?
    • The folder contents are loaded into untyped handles or throw an error for invalid types
  • What actually happened?
    • Bevy panics when given an absolute path to a folder

Additional information

I'm using toml files as project files (like Unreal/Unity/Godot) and want to load and watch the files in the project folder for changes; this folder can be anywhere on the filesystem. The 'read_directory' function assumes that the provided path is relative to self and appends the provided path to the self root path which turns into "C:\Path\To\Executable\C:\Absolute\Path\To\Folder\" and panics.

 fn read_directory(
        &self,
        path: &Path,
    ) -> Result<Box<dyn Iterator<Item = PathBuf>>, AssetIoError> {
        let root_path = self.root_path.to_owned();

        // This panics if path is absolute
        Ok(Box::new(fs::read_dir(root_path.join(path))?.map(
            move |entry| {
                let path = entry.unwrap().path();
                path.strip_prefix(&root_path).unwrap().to_owned()
            },
        )))
    }

The load function works as expected but the load_folder variation panics which is inconsistent behavior. I think the best fix would be to auto-detect absolute paths and add the root path if it's not. If that's not in the cards then an optional boolean parameter for is_absolute could work.

@iliags iliags added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Aug 16, 2023
@alice-i-cecile alice-i-cecile added 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 Aug 16, 2023
@alice-i-cecile
Copy link
Member

What OS are you on? File system bugs are sometimes OS specific, so it's good to record it.

@iliags
Copy link
Author

iliags commented Aug 16, 2023

I'm on windows but it will have the same behavior on linux. The issue is that load_folder adds the root path regardless of if the provided path is absolute or not while load does not.

The workaround that I found is to set the root directory to the project path; if that's the intended use then maybe it would be better to add a line or two in the docs rather than a code fix.

Edit:
Allowing multiple asset roots (key value pair) could be a better solution because it would allow unrelated asset sources to be used in one project.

@bushrat011899
Copy link
Contributor

bushrat011899 commented Aug 17, 2023

he 'read_directory' function assumes that the provided path is relative to self and appends the provided path to the self root path which turns into "C:\Path\To\Executable\C:\Absolute\Path\To\Folder" and panics.

I don't believe this is true. According to the Path::join docs, a.join(b) will yield b if b is already an absolute path. The real error is that Path::strip_prefix will return an error if you attempt to strip a prefix that does not match the current path. You can't strip a from b if b does not already start with a.

To follow-up on your example:

let a: Path; // "C:\Path\To\Executable\"
let b: Path; // "C:\Absolute\Path\To\Folder\"

let c = a.join(b); // "C:\Absolute\Path\To\Folder\"
let d = c.strip_prefix(&a); // Error: `c` does not start with `a`

The minimal change that would resolve this bug would be to construct a relative path from a to b. It looks like the most appropriate solution for that problem is using pathdiff::diff_paths.

fn read_directory(
    &self,
    path: &Path,
) -> Result<Box<dyn Iterator<Item = PathBuf>>, AssetIoError> {
    let root_path = self.root_path.to_owned();
    Ok(Box::new(fs::read_dir(root_path.join(path))?.map(
        move |entry| {
            let path = entry.unwrap().path();
            pathdiff::diff_paths(path, root_path).unwrap()
        },
    )))
}

@alice-i-cecile alice-i-cecile added the S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! label Aug 17, 2023
@iliags
Copy link
Author

iliags commented Aug 17, 2023

I'm new to rust so I'm not used to a sane(er) standard library; this is nice to learn.

github-merge-queue bot pushed a commit that referenced this issue Aug 28, 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:

```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);
```
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this issue 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-Implementation This issue is ready for an implementation PR. Go for it!
Projects
None yet
3 participants