Skip to content

Move away from std::Path in bevy_assets #19133

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.

Sign up for GitHub

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
+154 −61
Open

Move away from std::Path in bevy_assets #19133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2a4435e
change AssetPath to use &str
lielfr May 9, 2025
9531004
remove more usages of PathBuf
lielfr May 9, 2025
8e8903d
cleanup
lielfr May 9, 2025
3073d77
fix lints
lielfr May 9, 2025
7ec7345
get rid of unused function and use string version instead
lielfr May 9, 2025
6638211
remove itertools unused dependency
lielfr May 9, 2025
399d813
fix build in embedded_watcher
lielfr May 9, 2025
d7c06fb
remove lossy conversions, fix linting and doc tests, add more tests f…
lielfr May 9, 2025
2a4df37
PR fixes - constants for path separator, UTF-8 instead of unicode for…
lielfr May 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions crates/bevy_asset/src/io/embedded/embedded_watcher.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use crate::io::{
memory::Dir,
AssetSourceEvent, AssetWatcher,
};
use alloc::{boxed::Box, sync::Arc, vec::Vec};
use alloc::{boxed::Box, string::ToString, sync::Arc, vec::Vec};
use bevy_platform::collections::HashMap;
use core::time::Duration;
use notify_debouncer_full::{notify::RecommendedWatcher, Debouncer, RecommendedCache};
Expand Down Expand Up @@ -39,7 +39,14 @@ impl EmbeddedWatcher {
root_paths,
last_event: None,
};
let watcher = new_asset_event_debouncer(root, debounce_wait_time, handler).unwrap();
let watcher = new_asset_event_debouncer(
root.to_str()
.expect("non UTF-8 characters found in path")
.to_string(),
debounce_wait_time,
handler,
)
.unwrap();
Self { _watcher: watcher }
}
}
Expand Down
13 changes: 8 additions & 5 deletions crates/bevy_asset/src/io/file/file_watcher.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ use crate::{
path::normalize_path,
};
use alloc::borrow::ToOwned;
use alloc::string::String;
use alloc::vec::Vec;
use core::time::Duration;
use crossbeam_channel::Sender;
use notify_debouncer_full::{
Expand Down Expand Up @@ -31,11 +33,12 @@ pub struct FileWatcher {
impl FileWatcher {
/// Creates a new [`FileWatcher`] that watches for changes to the asset files in the given `path`.
pub fn new(
path: PathBuf,
path: String,
sender: Sender<AssetSourceEvent>,
debounce_wait_time: Duration,
) -> Result<Self, notify::Error> {
let root = normalize_path(&path).canonicalize()?;
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this canonicalize is important here. It makes paths with dots work correctly "../../my_assets_dir". Relevant PR: #18345 (hey look its me! I forgot all about this lol)

Should FileWatcher even be dealing with asset paths? This seems like a case where a file system path actually does make sense? I'm not sure though.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not familiar enough (yet?) with the codebase to decide, so I'd love some guidance here

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we would have to roll out own version of canonicalize to maintain being able to use ".." for relative paths

Copy link
Contributor

Choose a reason for hiding this comment

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

I looked into this a bit more, and I'm doubling down: I think FileWatcher should continue to deal with Path directly. I think the main thing we want to replace is the fact that our asset paths (as in, the ones that we pass into asset_server.load()) include some platform specific nonsense. That does not mean that our entire stack needs to deal only with strings - at some point, we need to convert that asset path into a concrete file path, so a Path instance.

So coming at it from the other angle, FileWatcher should be dealing with concrete paths because it is listening for real file system events for a particular directory. So I think we should revert what's happening in this file_watcher mod.

Copy link
Contributor

@viridia viridia Jun 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The plan that cart and I discussed a couple of years ago:

  • Asset server will certainly use FilePaths internally, for environments that have filesystems. For web environments, FilePaths may not be needed at all.
  • However, the semantics of FilePaths should not leak to the external asset server API. There should be a clear distinction between file paths and asset paths. In general, the upper layer uses asset paths, and the lower layer uses file paths.
  • Path manipulations on asset paths (concatenation, parent, etc) should use custom methods written for Bevy (these shouldn't be difficult, it's just string manipulation). So for example AssetPath::resolve shouldn't use FilePath methods.
  • Path manipulations on file paths should use FilePath methods - so for example adding the asset directory base path to the asset path would be a FilePath method.

let split_path: Vec<_> = path.split("/").collect();
let root = normalize_path(&split_path).join("/");
let watcher = new_asset_event_debouncer(
path.clone(),
debounce_wait_time,
Expand Down Expand Up @@ -72,7 +75,7 @@ pub(crate) fn get_asset_path(root: &Path, absolute_path: &Path) -> (PathBuf, boo
/// event management logic across filesystem-driven [`AssetWatcher`] impls. Each operating system / platform behaves
/// a little differently and this is the result of a delicate balancing act that we should only perform once.
pub(crate) fn new_asset_event_debouncer(
root: PathBuf,
root: String,
debounce_wait_time: Duration,
mut handler: impl FilesystemEventHandler,
) -> Result<Debouncer<RecommendedWatcher, RecommendedCache>, notify::Error> {
Expand Down Expand Up @@ -253,7 +256,7 @@ pub(crate) fn new_asset_event_debouncer(

pub(crate) struct FileEventHandler {
sender: Sender<AssetSourceEvent>,
root: PathBuf,
root: String,
last_event: Option<AssetSourceEvent>,
}

Expand All @@ -263,7 +266,7 @@ impl FilesystemEventHandler for FileEventHandler {
}
fn get_path(&self, absolute_path: &Path) -> Option<(PathBuf, bool)> {
let absolute_path = absolute_path.canonicalize().ok()?;
Some(get_asset_path(&self.root, &absolute_path))
Some(get_asset_path(Path::new(&self.root), &absolute_path))
}

fn handle(&mut self, _absolute_paths: &[PathBuf], event: AssetSourceEvent) {
Expand Down
4 changes: 3 additions & 1 deletion crates/bevy_asset/src/io/source.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -545,7 +545,9 @@ impl AssetSource {
if path.exists() {
Some(Box::new(
super::file::FileWatcher::new(
path.clone(),
path.to_str()
.expect("non UTF-8 characters found in path")
.to_string(),
sender,
file_debounce_wait_time,
)
Expand Down
Loading