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

Normalise root path in file_watcher #12102

Conversation

theon
Copy link
Contributor

@theon theon commented Feb 24, 2024

Objective

  • I hit an issue using the file_watcher feature to hot reload assets for my game. The change in this PR allows me to now hot reload assets.
  • The issue stemmed from my project being a multi crate workspace project structured like so:
└── my_game
    ├── my_game_core
    │   ├── src
    │   └── assets
    ├── my_game_editor
    │   └── src/main.rs
    └── my_game
        └── src/main.rs
  • my_game_core is a crate that holds all my game logic and assets
  • my_game is the crate that creates the binary for my game (depends on the game logic and assets in my_game_core)
  • my_game_editor is an editor tool for my game (it also depends on the game logic and assets in my_game_core)

Whilst running my_game and my_game_editor from cargo during development I would use AssetPlugin like so:

default_plugins.set(AssetPlugin {
  watch_for_changes_override: Some(true),
  file_path: "../my_game_core/assets".to_string(),
  ..Default::default()
})

This works fine; bevy picks up the assets. However on saving an asset I would get the following panic from file_watcher. It wouldn't kill the app, but I wouldn't see the asset hot reload:

thread 'notify-rs debouncer loop' panicked at /Users/ian/.cargo/registry/src/index.crates.io-6f17d22bba15001f/bevy_asset-0.12.1/src/io/file/file_watcher.rs:48:58:
called `Result::unwrap()` on an `Err` value: StripPrefixError(())
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Solution

  • The solution is to collapse dot segments in the root asset path FileWatcher is using
  • There was already bevy code to do this in AssetPath, so I extracted that code so it could be reused in FileWatcher

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 ✨

@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 Feb 24, 2024
@theon theon force-pushed the normalise-root-path-file-watcher branch from c1d2c2b to bfa3523 Compare February 24, 2024 22:01
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

I'm on board: this is a reasonable bug fix, and I really like pulling out this code into a reusable method.

@theon theon force-pushed the normalise-root-path-file-watcher branch 2 times, most recently from afd0dc6 to 0bf2b29 Compare February 24, 2024 22:14
@theon theon force-pushed the normalise-root-path-file-watcher branch from 0bf2b29 to e638f6d Compare February 24, 2024 22:17
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

It would be nice to add a link here, but I'm happy with this now :)

Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

Good call!

However, I think this would make more sense implemented as a standalone function, rather than an extension trait. I find the .normalized() in let root = super::get_base_path().join(root).normalized(); to be very confusing, while if it was just a normal function call like normalize_path(super::get_base_path().join(root)), it's easier to understand it's not a method on PathBuf, and something defined by bevy instead.

@theon
Copy link
Contributor Author

theon commented Feb 25, 2024

Good call!

However, I think this would make more sense implemented as a standalone function, rather than an extension trait. I find the .normalized() in let root = super::get_base_path().join(root).normalized(); to be very confusing, while if it was just a normal function call like normalize_path(super::get_base_path().join(root)), it's easier to understand it's not a method on PathBuf, and something defined by bevy instead.

That's fair; happy to change that 👍

@theon theon force-pushed the normalise-root-path-file-watcher branch from f545653 to 1d9a711 Compare February 25, 2024 10:20
@theon theon force-pushed the normalise-root-path-file-watcher branch from 1d9a711 to ec09a72 Compare February 25, 2024 10:22
@theon
Copy link
Contributor Author

theon commented Feb 25, 2024

@nicopap I've make the change to a standalone function
@alice-i-cecile I added a link to the RFC in the docstings at the same time

@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 Feb 25, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 25, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 25, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 25, 2024
Merged via the queue into bevyengine:main with commit 14042b1 Feb 25, 2024
24 checks passed
@theon theon deleted the normalise-root-path-file-watcher branch February 25, 2024 16:46
msvbg pushed a commit to msvbg/bevy that referenced this pull request Feb 26, 2024
# Objective

- I hit an issue using the `file_watcher` feature to hot reload assets
for my game. The change in this PR allows me to now hot reload assets.
- The issue stemmed from my project being a multi crate workspace
project structured like so:
```
└── my_game
    ├── my_game_core
    │   ├── src
    │   └── assets
    ├── my_game_editor
    │   └── src/main.rs
    └── my_game
        └── src/main.rs
```

 - `my_game_core` is a crate that holds all my game logic and assets
- `my_game` is the crate that creates the binary for my game (depends on
the game logic and assets in `my_game_core`)
- `my_game_editor` is an editor tool for my game (it also depends on the
game logic and assets in `my_game_core`)

Whilst running `my_game` and `my_game_editor` from cargo during
development I would use `AssetPlugin` like so:

```rust
default_plugins.set(AssetPlugin {
  watch_for_changes_override: Some(true),
  file_path: "../my_game_core/assets".to_string(),
  ..Default::default()
})
```

This works fine; bevy picks up the assets. However on saving an asset I
would get the following panic from `file_watcher`. It wouldn't kill the
app, but I wouldn't see the asset hot reload:

```
thread 'notify-rs debouncer loop' panicked at /Users/ian/.cargo/registry/src/index.crates.io-6f17d22bba15001f/bevy_asset-0.12.1/src/io/file/file_watcher.rs:48:58:
called `Result::unwrap()` on an `Err` value: StripPrefixError(())
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
```

## Solution

- The solution is to collapse dot segments in the root asset path
`FileWatcher` is using
- There was already bevy code to do this in `AssetPath`, so I extracted
that code so it could be reused in `FileWatcher`
msvbg pushed a commit to msvbg/bevy that referenced this pull request Feb 26, 2024
# Objective

- I hit an issue using the `file_watcher` feature to hot reload assets
for my game. The change in this PR allows me to now hot reload assets.
- The issue stemmed from my project being a multi crate workspace
project structured like so:
```
└── my_game
    ├── my_game_core
    │   ├── src
    │   └── assets
    ├── my_game_editor
    │   └── src/main.rs
    └── my_game
        └── src/main.rs
```

 - `my_game_core` is a crate that holds all my game logic and assets
- `my_game` is the crate that creates the binary for my game (depends on
the game logic and assets in `my_game_core`)
- `my_game_editor` is an editor tool for my game (it also depends on the
game logic and assets in `my_game_core`)

Whilst running `my_game` and `my_game_editor` from cargo during
development I would use `AssetPlugin` like so:

```rust
default_plugins.set(AssetPlugin {
  watch_for_changes_override: Some(true),
  file_path: "../my_game_core/assets".to_string(),
  ..Default::default()
})
```

This works fine; bevy picks up the assets. However on saving an asset I
would get the following panic from `file_watcher`. It wouldn't kill the
app, but I wouldn't see the asset hot reload:

```
thread 'notify-rs debouncer loop' panicked at /Users/ian/.cargo/registry/src/index.crates.io-6f17d22bba15001f/bevy_asset-0.12.1/src/io/file/file_watcher.rs:48:58:
called `Result::unwrap()` on an `Err` value: StripPrefixError(())
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
```

## Solution

- The solution is to collapse dot segments in the root asset path
`FileWatcher` is using
- There was already bevy code to do this in `AssetPath`, so I extracted
that code so it could be reused in `FileWatcher`
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.

3 participants