diff --git a/crates/bevy_asset/src/lib.rs b/crates/bevy_asset/src/lib.rs index f0205c9cd9027..a2be724fd1f50 100644 --- a/crates/bevy_asset/src/lib.rs +++ b/crates/bevy_asset/src/lib.rs @@ -1976,6 +1976,391 @@ mod tests { }); } + #[test] + fn error_on_immediate_load_of_self_dependency() { + let (mut app, dir, _source_events) = create_app_with_source_event_sender(); + let asset_server = app.world().resource::().clone(); + + dir.insert_asset_text(Path::new("abc.cool.ron"), ""); + + struct ImmediateSelfLoader; + + impl AssetLoader for ImmediateSelfLoader { + type Asset = TestAsset; + type Error = crate::loader::LoadDirectError; + type Settings = (); + + async fn load( + &self, + _: &mut dyn Reader, + _: &Self::Settings, + load_context: &mut LoadContext<'_>, + ) -> Result { + let asset_path = load_context.asset_path().clone(); + let loaded_asset = load_context + .loader() + .immediate() + .load::(asset_path) + .await?; + load_context.add_loaded_labeled_asset("myself".to_string(), loaded_asset); + Ok(TestAsset) + } + + fn extensions(&self) -> &[&str] { + &["ron"] + } + } + + app.init_asset::() + .register_asset_loader(ImmediateSelfLoader); + + let handle: Handle = asset_server.load("abc.cool.ron"); + + run_app_until(&mut app, |_world| match asset_server.load_state(&handle) { + LoadState::Loading => None, + LoadState::Failed(err) => { + assert!( + format!("{:?}", &err).contains("AssetDependentOnSelf"), + "Error did not contain AssetDependentOnSelf: {:?}", + &err + ); + Some(()) + } + state => panic!("Unexpected asset state: {state:?}"), + }); + } + + #[test] + fn error_on_unknown_type_immediate_load_of_self_dependency() { + let (mut app, dir, _source_events) = create_app_with_source_event_sender(); + let asset_server = app.world().resource::().clone(); + + dir.insert_asset_text(Path::new("abc.cool.ron"), ""); + + struct ImmediateSelfLoader; + + impl AssetLoader for ImmediateSelfLoader { + type Asset = TestAsset; + type Error = crate::loader::LoadDirectError; + type Settings = (); + + async fn load( + &self, + _: &mut dyn Reader, + _: &Self::Settings, + load_context: &mut LoadContext<'_>, + ) -> Result { + let asset_path = load_context.asset_path().clone(); + let loaded_asset = load_context + .loader() + .immediate() + .with_unknown_type() + .load(asset_path) + .await?; + let Ok(loaded_asset) = loaded_asset.downcast::() else { + panic!("Could not downcast to `TestAsset`") + }; + load_context.add_loaded_labeled_asset("myself".to_string(), loaded_asset); + Ok(TestAsset) + } + + fn extensions(&self) -> &[&str] { + &["ron"] + } + } + + app.init_asset::() + .register_asset_loader(ImmediateSelfLoader); + + let handle: Handle = asset_server.load("abc.cool.ron"); + + run_app_until(&mut app, |_world| match asset_server.load_state(&handle) { + LoadState::Loading => None, + LoadState::Failed(err) => { + assert!( + format!("{:?}", &err).contains("AssetDependentOnSelf"), + "Error did not contain AssetDependentOnSelf: {:?}", + &err + ); + Some(()) + } + state => panic!("Unexpected asset state: {state:?}"), + }); + } + + /// This is not a statement of intent but one of behavior: One may load + /// their own path deferred without error. It has the correct handle to + /// itself. And it can reload. + /// + /// I would have wanted this to produce an error. Instead it produces a + /// warning. + #[test] + fn no_error_on_deferred_load_of_self_dependency() { + let (mut app, dir, source_events) = create_app_with_source_event_sender(); + let asset_server = app.world().resource::().clone(); + + dir.insert_asset_text(Path::new("abc.cool.ron"), ""); + + #[derive(Asset, TypePath)] + pub struct TestAsset(Handle); + struct DeferredSelfLoader; + + impl AssetLoader for DeferredSelfLoader { + type Asset = TestAsset; + type Error = crate::loader::LoadDirectError; + type Settings = (); + + async fn load( + &self, + _: &mut dyn Reader, + _: &Self::Settings, + load_context: &mut LoadContext<'_>, + ) -> Result { + let asset_path = load_context.asset_path().clone(); + let loaded_asset = load_context.load::(asset_path); + Ok(TestAsset(loaded_asset)) + } + + fn extensions(&self) -> &[&str] { + &["ron"] + } + } + + app.init_asset::() + .register_asset_loader(DeferredSelfLoader); + + let handle: Handle = asset_server.load("abc.cool.ron"); + + run_app_until(&mut app, |world| match asset_server.load_state(&handle) { + LoadState::Loading => None, + LoadState::Loaded => { + let test_assets = world.resource::>(); + let asset = test_assets.get(&handle).unwrap(); + assert_eq!(handle, asset.0); + Some(()) + } + state => panic!("Unexpected asset state: {state:?}"), + }); + + run_app_until(&mut app, |world| { + let messages = collect_asset_events(world); + if messages.is_empty() { + return None; + } + assert_eq!( + messages, + [ + AssetEvent::LoadedWithDependencies { id: handle.id() }, + AssetEvent::Added { id: handle.id() } + ] + ); + Some(()) + }); + + // Sending an asset event should result in the asset being reloaded - resulting in a + // "Modified" message. + source_events + .send_blocking(AssetSourceEvent::ModifiedAsset(PathBuf::from( + "abc.cool.ron", + ))) + .unwrap(); + + run_app_until(&mut app, |world| { + let messages = collect_asset_events(world); + if messages.is_empty() { + return None; + } + assert_eq!( + messages, + [ + AssetEvent::LoadedWithDependencies { id: handle.id() }, + AssetEvent::Modified { id: handle.id() } + ] + ); + Some(()) + }); + } + + #[test] + fn no_error_on_read_bytes_of_self_dependency() { + let (mut app, dir, source_events) = create_app_with_source_event_sender(); + let asset_server = app.world().resource::().clone(); + + dir.insert_asset_text(Path::new("abc.cool.ron"), ""); + + struct ReadBytesSelfLoader; + + impl AssetLoader for ReadBytesSelfLoader { + type Asset = TestAsset; + type Error = crate::loader::LoadDirectError; + type Settings = (); + + async fn load( + &self, + _: &mut dyn Reader, + _: &Self::Settings, + load_context: &mut LoadContext<'_>, + ) -> Result { + let asset_path = load_context.asset_path().clone(); + let _bytes = load_context.read_asset_bytes(asset_path).await.unwrap(); + Ok(TestAsset) + } + + fn extensions(&self) -> &[&str] { + &["ron"] + } + } + + app.init_asset::() + .register_asset_loader(ReadBytesSelfLoader); + + let handle: Handle = asset_server.load("abc.cool.ron"); + + run_app_until(&mut app, |_world| match asset_server.load_state(&handle) { + LoadState::Loading => None, + LoadState::Loaded => Some(()), + state => panic!("Unexpected asset state: {state:?}"), + }); + + run_app_until(&mut app, |world| { + let messages = collect_asset_events(world); + if messages.is_empty() { + return None; + } + assert_eq!( + messages, + [ + AssetEvent::LoadedWithDependencies { id: handle.id() }, + AssetEvent::Added { id: handle.id() } + ] + ); + Some(()) + }); + + // Sending an asset event should result in the asset being reloaded - resulting in a + // "Modified" message. + source_events + .send_blocking(AssetSourceEvent::ModifiedAsset(PathBuf::from( + "abc.cool.ron", + ))) + .unwrap(); + + run_app_until(&mut app, |world| { + let messages = collect_asset_events(world); + if messages.is_empty() { + return None; + } + assert_eq!( + messages, + [ + AssetEvent::LoadedWithDependencies { id: handle.id() }, + AssetEvent::Modified { id: handle.id() } + ] + ); + Some(()) + }); + } + + /// This is not a statement of intent but one of behavior: One may load + /// their own path deferred of unknown type without error. It has the + /// correct handle to itself. And it can reload. + /// + /// I would have wanted this to produce an error. Instead it produces a + /// warning. + #[test] + fn no_error_on_unknown_type_deferred_load_of_self_dependency() { + let (mut app, dir, source_events) = create_app_with_source_event_sender(); + let asset_server = app.world().resource::().clone(); + + dir.insert_asset_text(Path::new("abc.cool.ron"), ""); + + #[derive(Asset, TypePath)] + pub struct TestAssetUD(UntypedHandle); + struct ImmediateSelfLoader; + + impl AssetLoader for ImmediateSelfLoader { + type Asset = TestAssetUD; + type Error = crate::loader::LoadDirectError; + type Settings = (); + + async fn load( + &self, + _: &mut dyn Reader, + _: &Self::Settings, + load_context: &mut LoadContext<'_>, + ) -> Result { + let asset_path = load_context.asset_path().clone(); + let untyped_handle: UntypedHandle = load_context + .loader() + .with_unknown_type() + .load(asset_path) + .into(); + + Ok(TestAssetUD(untyped_handle)) + } + + fn extensions(&self) -> &[&str] { + &["ron"] + } + } + + app.init_asset::() + .register_asset_loader(ImmediateSelfLoader); + + let handle: Handle = asset_server.load("abc.cool.ron"); + + run_app_until(&mut app, |world| match asset_server.load_state(&handle) { + LoadState::Loading => None, + LoadState::Loaded => { + let test_assets = world.resource::>(); + let asset = test_assets.get(&handle).unwrap(); + assert_eq!(handle.id(), asset.0.id().typed_unchecked::()); + // This one fails. + // assert_eq!(handle.id(), asset.0.id().typed::()); + Some(()) + } + state => panic!("Unexpected asset state: {state:?}"), + }); + + run_app_until(&mut app, |world| { + let messages = collect_asset_events(world); + if messages.is_empty() { + return None; + } + assert_eq!( + messages, + [ + AssetEvent::LoadedWithDependencies { id: handle.id() }, + AssetEvent::Added { id: handle.id() } + ] + ); + Some(()) + }); + + // Sending an asset event should result in the asset being reloaded - resulting in a + // "Modified" message. + source_events + .send_blocking(AssetSourceEvent::ModifiedAsset(PathBuf::from( + "abc.cool.ron", + ))) + .unwrap(); + + run_app_until(&mut app, |world| { + let messages = collect_asset_events(world); + if messages.is_empty() { + return None; + } + assert_eq!( + messages, + [ + AssetEvent::LoadedWithDependencies { id: handle.id() }, + AssetEvent::Modified { id: handle.id() } + ] + ); + Some(()) + }); + } + // validate the Asset derive macro for various asset types #[derive(Asset, TypePath)] pub struct TestAsset; diff --git a/crates/bevy_asset/src/loader.rs b/crates/bevy_asset/src/loader.rs index 90ee558f05dd6..eac91d2d674dc 100644 --- a/crates/bevy_asset/src/loader.rs +++ b/crates/bevy_asset/src/loader.rs @@ -305,6 +305,14 @@ pub enum LoadDirectError { dependency: AssetPath<'static>, error: AssetLoadError, }, + #[error(transparent)] + AssetDependentOnSelf(#[from] AssetDependentOnSelf), +} + +#[derive(Error, Debug, Clone)] +#[error("The asset at path `{}` loads itself as a dependent.", asset_path)] +pub struct AssetDependentOnSelf { + pub asset_path: AssetPath<'static>, } /// An error that occurs while deserializing [`AssetMeta`]. @@ -508,7 +516,9 @@ impl<'a> LoadContext<'a> { path: path.path().to_path_buf(), source, })?; - self.loader_dependencies.insert(path.clone_owned(), hash); + if self.asset_path != path { + self.loader_dependencies.insert(path.clone_owned(), hash); + } Ok(bytes) } @@ -534,6 +544,12 @@ impl<'a> LoadContext<'a> { loader: &dyn ErasedAssetLoader, reader: &mut dyn Reader, ) -> Result { + if self.asset_path == path { + return Err(AssetDependentOnSelf { + asset_path: self.asset_path.clone_owned(), + } + .into()); + } let loaded_asset = self .asset_server .load_with_meta_loader_and_reader( diff --git a/crates/bevy_asset/src/loader_builders.rs b/crates/bevy_asset/src/loader_builders.rs index 994eb33590bee..b5083026b83f2 100644 --- a/crates/bevy_asset/src/loader_builders.rs +++ b/crates/bevy_asset/src/loader_builders.rs @@ -4,11 +4,12 @@ use crate::{ io::Reader, meta::{meta_transform_settings, AssetMetaDyn, MetaTransform, Settings}, - Asset, AssetLoadError, AssetPath, ErasedAssetLoader, ErasedLoadedAsset, Handle, LoadContext, - LoadDirectError, LoadedAsset, LoadedUntypedAsset, UntypedHandle, + Asset, AssetDependentOnSelf, AssetLoadError, AssetPath, ErasedAssetLoader, ErasedLoadedAsset, + Handle, LoadContext, LoadDirectError, LoadedAsset, LoadedUntypedAsset, UntypedHandle, }; use alloc::{borrow::ToOwned, boxed::Box, sync::Arc}; use core::any::TypeId; +use tracing::warn; // Utility type for handling the sources of reader references enum ReaderRef<'a> { @@ -304,6 +305,8 @@ impl NestedLoader<'_, '_, StaticTyped, Deferred> { /// [`with_unknown_type`]: Self::with_unknown_type pub fn load<'c, A: Asset>(self, path: impl Into>) -> Handle { let path = path.into().to_owned(); + + let is_self_path = *self.load_context.asset_path() == path; let handle = if self.load_context.should_load_dependencies { self.load_context.asset_server.load_with_meta_transform( path, @@ -318,8 +321,15 @@ impl NestedLoader<'_, '_, StaticTyped, Deferred> { }; // `load_with_meta_transform` and `get_or_create_path_handle` always returns a Strong // variant, so we are safe to unwrap. - let index = (&handle).try_into().unwrap(); - self.load_context.dependencies.insert(index); + if !is_self_path { + let index = (&handle).try_into().unwrap(); + self.load_context.dependencies.insert(index); + } else { + warn!( + "Asset from path `{}` loaded the same path as a dependent, ignoring.", + self.load_context.asset_path() + ); + } handle } } @@ -367,6 +377,7 @@ impl NestedLoader<'_, '_, UnknownTyped, Deferred> { /// This will infer the asset type from metadata. pub fn load<'p>(self, path: impl Into>) -> Handle { let path = path.into().to_owned(); + let is_self_path = *self.load_context.asset_path() == path; let handle = if self.load_context.should_load_dependencies { self.load_context .asset_server @@ -379,7 +390,12 @@ impl NestedLoader<'_, '_, UnknownTyped, Deferred> { // `load_unknown_type_with_meta_transform` and `get_or_create_path_handle` always returns a // Strong variant, so we are safe to unwrap. let index = (&handle).try_into().unwrap(); - self.load_context.dependencies.insert(index); + + if !is_self_path { + self.load_context.dependencies.insert(index); + } else { + warn!("Asset from path `{}` of unknown type loaded the same path as a dependent, ignoring.", self.load_context.asset_path()); + } handle } } @@ -402,6 +418,12 @@ impl<'builder, 'reader, T> NestedLoader<'_, '_, T, Immediate<'builder, 'reader>> if path.label().is_some() { return Err(LoadDirectError::RequestedSubasset(path.clone())); } + if self.load_context.asset_path() == path { + return Err(AssetDependentOnSelf { + asset_path: path.clone(), + } + .into()); + } self.load_context .asset_server .write_infos() diff --git a/crates/bevy_asset/src/server/mod.rs b/crates/bevy_asset/src/server/mod.rs index 3ae2ee1af6971..3576fdf365932 100644 --- a/crates/bevy_asset/src/server/mod.rs +++ b/crates/bevy_asset/src/server/mod.rs @@ -1782,6 +1782,13 @@ pub fn handle_internal_asset_events(world: &mut World) { ) { if let Some(dependents) = infos.loader_dependents.get(asset_path) { for dependent in dependents { + assert_ne!( + asset_path, dependent, + "The asset path `{}` contains itself as a dependent.", + &asset_path + ); + // If the above assertion fails, the following code would + // cause a stackoverflow. paths_to_reload.insert(dependent.to_owned()); queue_ancestors(dependent, infos, paths_to_reload); }