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 14 commits into
base: main
Choose a base branch
from
Open

Move away from std::Path in bevy_assets #19133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
3d03097
change AssetPath to use &str
lielfr May 9, 2025
4ef08b7
remove more usages of PathBuf
lielfr May 9, 2025
48e0691
cleanup
lielfr May 9, 2025
c205b00
fix lints
lielfr May 9, 2025
3adf527
get rid of unused function and use string version instead
lielfr May 9, 2025
af99513
remove itertools unused dependency
lielfr May 9, 2025
ec47b4c
fix build in embedded_watcher
lielfr May 9, 2025
b1733fd
remove lossy conversions, fix linting and doc tests, add more tests f…
lielfr May 9, 2025
449d2ac
PR fixes - constants for path separator, UTF-8 instead of unicode for…
lielfr May 11, 2025
d831e32
fix build
lielfr Jun 8, 2025
2a48d7c
add file_name method
lielfr Jun 8, 2025
da9ecbc
AssetReaderError: move from PathBuf to String
lielfr Jun 8, 2025
f73f88f
get rid of more path function usages from AssetPath
lielfr Jun 8, 2025
27e9bde
remove unused imports
lielfr Jun 8, 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
14 changes: 8 additions & 6 deletions crates/bevy_asset/src/io/file/file_asset.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ impl AssetReader for FileAssetReader {
let full_path = self.root_path.join(path);
File::open(&full_path).await.map_err(|e| {
if e.kind() == std::io::ErrorKind::NotFound {
AssetReaderError::NotFound(full_path)
AssetReaderError::NotFound(full_path.to_path_buf().to_str().unwrap().to_owned())
} else {
e.into()
}
Expand All @@ -50,7 +50,7 @@ impl AssetReader for FileAssetReader {
let full_path = self.root_path.join(meta_path);
File::open(&full_path).await.map_err(|e| {
if e.kind() == std::io::ErrorKind::NotFound {
AssetReaderError::NotFound(full_path)
AssetReaderError::NotFound(full_path.to_path_buf().to_str().unwrap().to_owned())
} else {
e.into()
}
Expand Down Expand Up @@ -92,7 +92,9 @@ impl AssetReader for FileAssetReader {
}
Err(e) => {
if e.kind() == std::io::ErrorKind::NotFound {
Err(AssetReaderError::NotFound(full_path))
Err(AssetReaderError::NotFound(
full_path.to_path_buf().to_str().unwrap().to_owned(),
))
} else {
Err(e.into())
}
Expand All @@ -102,9 +104,9 @@ impl AssetReader for FileAssetReader {

async fn is_directory<'a>(&'a self, path: &'a Path) -> Result<bool, AssetReaderError> {
let full_path = self.root_path.join(path);
let metadata = full_path
.metadata()
.map_err(|_e| AssetReaderError::NotFound(path.to_owned()))?;
let metadata = full_path.metadata().map_err(|_e| {
AssetReaderError::NotFound(path.to_path_buf().to_str().unwrap().to_owned())
})?;
Ok(metadata.file_type().is_dir())
}
}
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
14 changes: 10 additions & 4 deletions crates/bevy_asset/src/io/file/sync_file_asset.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,9 @@ impl AssetReader for FileAssetReader {
Ok(file) => Ok(FileReader(file)),
Err(e) => {
if e.kind() == std::io::ErrorKind::NotFound {
Err(AssetReaderError::NotFound(full_path))
Err(AssetReaderError::NotFound(
full_path.to_str().unwrap().to_owned(),
))
} else {
Err(e.into())
}
Expand All @@ -120,7 +122,9 @@ impl AssetReader for FileAssetReader {
Ok(file) => Ok(FileReader(file)),
Err(e) => {
if e.kind() == std::io::ErrorKind::NotFound {
Err(AssetReaderError::NotFound(full_path))
Err(AssetReaderError::NotFound(
full_path.to_str().unwrap().to_owned(),
))
} else {
Err(e.into())
}
Expand Down Expand Up @@ -164,7 +168,9 @@ impl AssetReader for FileAssetReader {
}
Err(e) => {
if e.kind() == std::io::ErrorKind::NotFound {
Err(AssetReaderError::NotFound(full_path))
Err(AssetReaderError::NotFound(
full_path.to_str().unwrap().to_owned(),
))
} else {
Err(e.into())
}
Expand All @@ -176,7 +182,7 @@ impl AssetReader for FileAssetReader {
let full_path = self.root_path.join(path);
let metadata = full_path
.metadata()
.map_err(|_e| AssetReaderError::NotFound(path.to_owned()))?;
.map_err(|_e| AssetReaderError::NotFound(path.to_str().unwrap().to_owned()))?;
Ok(metadata.file_type().is_dir())
}
}
Expand Down
12 changes: 9 additions & 3 deletions crates/bevy_asset/src/io/memory.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,9 @@ impl AssetReader for MemoryAssetReader {
data,
bytes_read: 0,
})
.ok_or_else(|| AssetReaderError::NotFound(path.to_path_buf()))
.ok_or_else(|| {
AssetReaderError::NotFound(path.to_path_buf().to_str().unwrap().to_owned())
})
}

async fn read_meta<'a>(&'a self, path: &'a Path) -> Result<impl Reader + 'a, AssetReaderError> {
Expand All @@ -304,7 +306,9 @@ impl AssetReader for MemoryAssetReader {
data,
bytes_read: 0,
})
.ok_or_else(|| AssetReaderError::NotFound(path.to_path_buf()))
.ok_or_else(|| {
AssetReaderError::NotFound(path.to_path_buf().to_str().unwrap().to_owned())
})
}

async fn read_directory<'a>(
Expand All @@ -317,7 +321,9 @@ impl AssetReader for MemoryAssetReader {
let stream: Box<PathStream> = Box::new(DirStream::new(dir));
stream
})
.ok_or_else(|| AssetReaderError::NotFound(path.to_path_buf()))
.ok_or_else(|| {
AssetReaderError::NotFound(path.to_path_buf().to_str().unwrap().to_owned())
})
}

async fn is_directory<'a>(&'a self, path: &'a Path) -> Result<bool, AssetReaderError> {
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_asset/src/io/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ use thiserror::Error;
#[derive(Error, Debug, Clone)]
pub enum AssetReaderError {
/// Path not found.
#[error("Path not found: {}", _0.display())]
NotFound(PathBuf),
#[error("Path not found: {}", _0)]
NotFound(alloc::string::String),

/// Encountered an I/O error while loading an asset.
#[error("Encountered an I/O error while loading asset: {0}")]
Expand Down
10 changes: 7 additions & 3 deletions crates/bevy_asset/src/io/processor_gated.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ impl ProcessorGatedReader {
let infos = self.processor_data.asset_infos.read().await;
let info = infos
.get(path)
.ok_or_else(|| AssetReaderError::NotFound(path.path().to_owned()))?;
.ok_or_else(|| AssetReaderError::NotFound(path.internal_path().to_owned()))?;
Ok(info.file_transaction_lock.read_arc().await)
}
}
Expand All @@ -61,7 +61,9 @@ impl AssetReader for ProcessorGatedReader {
match process_result {
ProcessStatus::Processed => {}
ProcessStatus::Failed | ProcessStatus::NonExistent => {
return Err(AssetReaderError::NotFound(path.to_owned()));
return Err(AssetReaderError::NotFound(
path.to_path_buf().to_str().unwrap().to_owned(),
));
}
}
trace!("Processing finished with {asset_path}, reading {process_result:?}",);
Expand All @@ -81,7 +83,9 @@ impl AssetReader for ProcessorGatedReader {
match process_result {
ProcessStatus::Processed => {}
ProcessStatus::Failed | ProcessStatus::NonExistent => {
return Err(AssetReaderError::NotFound(path.to_owned()));
return Err(AssetReaderError::NotFound(
path.to_path_buf().to_str().unwrap().to_owned(),
));
}
}
trace!("Processing finished with {process_result:?}, reading meta for {asset_path}",);
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
8 changes: 4 additions & 4 deletions crates/bevy_asset/src/loader.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use core::any::{Any, TypeId};
use downcast_rs::{impl_downcast, Downcast};
use ron::error::SpannedError;
use serde::{Deserialize, Serialize};
use std::path::{Path, PathBuf};
use std::path::PathBuf;
use thiserror::Error;

/// Loads an [`Asset`] from a given byte [`Reader`]. This can accept [`AssetLoader::Settings`], which configure how the [`Asset`]
Expand Down Expand Up @@ -357,7 +357,7 @@ impl<'a> LoadContext<'a> {
/// # use bevy_reflect::TypePath;
/// # #[derive(Asset, TypePath, Default)]
/// # struct Image;
/// # let load_context: LoadContext = panic!();
/// # let mut load_context: LoadContext = panic!();
/// let mut handles = Vec::new();
/// for i in 0..2 {
/// let mut labeled = load_context.begin_labeled_asset();
Expand Down Expand Up @@ -459,8 +459,8 @@ impl<'a> LoadContext<'a> {
}

/// Gets the source path for this load context.
pub fn path(&self) -> &Path {
self.asset_path.path()
pub fn path(&self) -> &str {
self.asset_path.internal_path()
}

/// Gets the source asset path for this load context.
Expand Down
Loading
Loading