From 260a7ae219afde06b24d4cf2f15317d230020ee0 Mon Sep 17 00:00:00 2001 From: Cole Varner Date: Sun, 15 Sep 2024 11:33:04 -0700 Subject: [PATCH 1/2] - resovles #15098 - implements consistently named AssertServer methods for asset, dependency, and recursive dependency load states - adds error to DependencyLoadState::Failed and RecursiveDependencyLoadState::Failed - fixes bug where successful dependency load overwrites previous dependency failed load state --- crates/bevy_asset/src/lib.rs | 134 ++++++++++++++++++++++++++- crates/bevy_asset/src/server/info.rs | 47 ++++++---- crates/bevy_asset/src/server/mod.rs | 104 +++++++++++++++++---- 3 files changed, 244 insertions(+), 41 deletions(-) diff --git a/crates/bevy_asset/src/lib.rs b/crates/bevy_asset/src/lib.rs index b42b4436f989b..eec01bef5baaf 100644 --- a/crates/bevy_asset/src/lib.rs +++ b/crates/bevy_asset/src/lib.rs @@ -1095,13 +1095,19 @@ mod tests { assert!(d_text.is_none()); assert!(matches!(d_load, LoadState::Failed(_))); - assert_eq!(d_deps, DependencyLoadState::Failed); - assert_eq!(d_rec_deps, RecursiveDependencyLoadState::Failed); + assert!(matches!(d_deps, DependencyLoadState::Failed(_))); + assert!(matches!( + d_rec_deps, + RecursiveDependencyLoadState::Failed(_) + )); assert_eq!(a_text.text, "a"); assert_eq!(a_load, LoadState::Loaded); assert_eq!(a_deps, DependencyLoadState::Loaded); - assert_eq!(a_rec_deps, RecursiveDependencyLoadState::Failed); + assert!(matches!( + a_rec_deps, + RecursiveDependencyLoadState::Failed(_) + )); assert_eq!(b_text.text, "b"); assert_eq!(b_load, LoadState::Loaded); @@ -1110,13 +1116,131 @@ mod tests { assert_eq!(c_text.text, "c"); assert_eq!(c_load, LoadState::Loaded); - assert_eq!(c_deps, DependencyLoadState::Failed); - assert_eq!(c_rec_deps, RecursiveDependencyLoadState::Failed); + assert!(matches!(c_deps, DependencyLoadState::Failed(_))); + assert!(matches!( + c_rec_deps, + RecursiveDependencyLoadState::Failed(_) + )); + + assert_eq!(asset_server.load_state(a_id), LoadState::Loaded); + assert_eq!( + asset_server.dependency_load_state(a_id), + DependencyLoadState::Loaded + ); + assert!(matches!( + asset_server.recursive_dependency_load_state(a_id), + RecursiveDependencyLoadState::Failed(_) + )); + + assert!(asset_server.is_loaded(a_id)); + assert!(asset_server.is_loaded_with_dependencies(a_id)); + assert!(!asset_server.is_loaded_with_recursive_dependencies(a_id)); Some(()) }); } + #[test] + fn dependency_load_states() { + // The particular usage of GatedReader in this test will cause deadlocking if running single-threaded + #[cfg(not(feature = "multi_threaded"))] + panic!("This test requires the \"multi_threaded\" feature, otherwise it will deadlock.\ncargo test --package bevy_asset --features multi_threaded"); + + let a_path = "a.cool.ron"; + let a_ron = r#" +( + text: "a", + dependencies: [ + "b.cool.ron", + "c.cool.ron", + ], + embedded_dependencies: [], + sub_texts: [] +)"#; + let b_path = "b.cool.ron"; + let b_ron = r#" +( + text: "b", + dependencies: [], + MALFORMED + embedded_dependencies: [], + sub_texts: [] +)"#; + + let c_path = "c.cool.ron"; + let c_ron = r#" +( + text: "c", + dependencies: [], + embedded_dependencies: [], + sub_texts: [] +)"#; + + let dir = Dir::default(); + dir.insert_asset_text(Path::new(a_path), a_ron); + dir.insert_asset_text(Path::new(b_path), b_ron); + dir.insert_asset_text(Path::new(c_path), c_ron); + + let (mut app, gate_opener) = test_app(dir); + app.init_asset::() + .register_asset_loader(CoolTextLoader); + let asset_server = app.world().resource::().clone(); + let handle: Handle = asset_server.load(a_path); + let a_id = handle.id(); + app.world_mut().spawn(handle); + + gate_opener.open(a_path); + run_app_until(&mut app, |world| { + let _a_text = get::(world, a_id)?; + let (a_load, a_deps, a_rec_deps) = asset_server.get_load_states(a_id).unwrap(); + assert_eq!(a_load, LoadState::Loaded); + assert_eq!(a_deps, DependencyLoadState::Loading); + assert_eq!(a_rec_deps, RecursiveDependencyLoadState::Loading); + Some(()) + }); + + gate_opener.open(b_path); + run_app_until(&mut app, |world| { + let a_text = get::(world, a_id)?; + let b_id = a_text.dependencies[0].id(); + + let (b_load, _b_deps, _b_rec_deps) = asset_server.get_load_states(b_id).unwrap(); + if !matches!(b_load, LoadState::Failed(_)) { + // wait until b fails + return None; + } + + let (a_load, a_deps, a_rec_deps) = asset_server.get_load_states(a_id).unwrap(); + assert_eq!(a_load, LoadState::Loaded); + assert!(matches!(a_deps, DependencyLoadState::Failed(_))); + assert!(matches!( + a_rec_deps, + RecursiveDependencyLoadState::Failed(_) + )); + Some(()) + }); + + gate_opener.open(c_path); + run_app_until(&mut app, |world| { + let a_text = get::(world, a_id)?; + let c_id = a_text.dependencies[1].id(); + // wait until c loads + let _c_text = get::(world, c_id)?; + + let (a_load, a_deps, a_rec_deps) = asset_server.get_load_states(a_id).unwrap(); + assert_eq!(a_load, LoadState::Loaded); + assert!( + matches!(a_deps, DependencyLoadState::Failed(_)), + "Successful dependency load should not overwrite a previous failure" + ); + assert!( + matches!(a_rec_deps, RecursiveDependencyLoadState::Failed(_)), + "Successful dependency load should not overwrite a previous failure" + ); + Some(()) + }); + } + const SIMPLE_TEXT: &str = r#" ( text: "dep", diff --git a/crates/bevy_asset/src/server/info.rs b/crates/bevy_asset/src/server/info.rs index b63c139f326a2..82bc285f21edb 100644 --- a/crates/bevy_asset/src/server/info.rs +++ b/crates/bevy_asset/src/server/info.rs @@ -388,8 +388,10 @@ impl AssetInfos { loaded_asset.value.insert(loaded_asset_id, world); let mut loading_deps = loaded_asset.dependencies; let mut failed_deps = HashSet::new(); + let mut dep_error = None; let mut loading_rec_deps = loading_deps.clone(); let mut failed_rec_deps = HashSet::new(); + let mut rec_dep_error = None; loading_deps.retain(|dep_id| { if let Some(dep_info) = self.get_mut(*dep_id) { match dep_info.rec_dep_load_state { @@ -404,7 +406,10 @@ impl AssetInfos { // If dependency is loaded, reduce our count by one loading_rec_deps.remove(dep_id); } - RecursiveDependencyLoadState::Failed => { + RecursiveDependencyLoadState::Failed(ref error) => { + if rec_dep_error.is_none() { + rec_dep_error = Some(error.clone()); + } failed_rec_deps.insert(*dep_id); loading_rec_deps.remove(dep_id); } @@ -419,7 +424,10 @@ impl AssetInfos { // If dependency is loaded, reduce our count by one false } - LoadState::Failed(_) => { + LoadState::Failed(ref error) => { + if dep_error.is_none() { + dep_error = Some(error.clone()); + } failed_deps.insert(*dep_id); false } @@ -437,7 +445,7 @@ impl AssetInfos { let dep_load_state = match (loading_deps.len(), failed_deps.len()) { (0, 0) => DependencyLoadState::Loaded, (_loading, 0) => DependencyLoadState::Loading, - (_loading, _failed) => DependencyLoadState::Failed, + (_loading, _failed) => DependencyLoadState::Failed(dep_error.unwrap()), }; let rec_dep_load_state = match (loading_rec_deps.len(), failed_rec_deps.len()) { @@ -450,7 +458,7 @@ impl AssetInfos { RecursiveDependencyLoadState::Loaded } (_loading, 0) => RecursiveDependencyLoadState::Loading, - (_loading, _failed) => RecursiveDependencyLoadState::Failed, + (_loading, _failed) => RecursiveDependencyLoadState::Failed(rec_dep_error.unwrap()), }; let (dependants_waiting_on_load, dependants_waiting_on_rec_load) = { @@ -480,14 +488,14 @@ impl AssetInfos { info.failed_rec_dependencies = failed_rec_deps; info.load_state = LoadState::Loaded; info.dep_load_state = dep_load_state; - info.rec_dep_load_state = rec_dep_load_state; + info.rec_dep_load_state = rec_dep_load_state.clone(); if watching_for_changes { info.loader_dependencies = loaded_asset.loader_dependencies; } let dependants_waiting_on_rec_load = if matches!( rec_dep_load_state, - RecursiveDependencyLoadState::Loaded | RecursiveDependencyLoadState::Failed + RecursiveDependencyLoadState::Loaded | RecursiveDependencyLoadState::Failed(_) ) { Some(std::mem::take( &mut info.dependants_waiting_on_recursive_dep_load, @@ -505,7 +513,9 @@ impl AssetInfos { for id in dependants_waiting_on_load { if let Some(info) = self.get_mut(id) { info.loading_dependencies.remove(&loaded_asset_id); - if info.loading_dependencies.is_empty() { + if info.loading_dependencies.is_empty() + && !matches!(info.dep_load_state, DependencyLoadState::Failed(_)) + { // send dependencies loaded event info.dep_load_state = DependencyLoadState::Loaded; } @@ -519,9 +529,9 @@ impl AssetInfos { Self::propagate_loaded_state(self, loaded_asset_id, dep_id, sender); } } - RecursiveDependencyLoadState::Failed => { + RecursiveDependencyLoadState::Failed(ref error) => { for dep_id in dependants_waiting_on_rec_load { - Self::propagate_failed_state(self, loaded_asset_id, dep_id); + Self::propagate_failed_state(self, loaded_asset_id, dep_id, error); } } RecursiveDependencyLoadState::Loading | RecursiveDependencyLoadState::NotLoaded => { @@ -570,11 +580,12 @@ impl AssetInfos { infos: &mut AssetInfos, failed_id: UntypedAssetId, waiting_id: UntypedAssetId, + error: &Arc, ) { let dependants_waiting_on_rec_load = if let Some(info) = infos.get_mut(waiting_id) { info.loading_rec_dependencies.remove(&failed_id); info.failed_rec_dependencies.insert(failed_id); - info.rec_dep_load_state = RecursiveDependencyLoadState::Failed; + info.rec_dep_load_state = RecursiveDependencyLoadState::Failed(error.clone()); Some(std::mem::take( &mut info.dependants_waiting_on_recursive_dep_load, )) @@ -584,7 +595,7 @@ impl AssetInfos { if let Some(dependants_waiting_on_rec_load) = dependants_waiting_on_rec_load { for dep_id in dependants_waiting_on_rec_load { - Self::propagate_failed_state(infos, waiting_id, dep_id); + Self::propagate_failed_state(infos, waiting_id, dep_id, error); } } } @@ -595,14 +606,15 @@ impl AssetInfos { return; } + let error = Arc::new(error); let (dependants_waiting_on_load, dependants_waiting_on_rec_load) = { let Some(info) = self.get_mut(failed_id) else { // The asset was already dropped. return; }; - info.load_state = LoadState::Failed(Box::new(error)); - info.dep_load_state = DependencyLoadState::Failed; - info.rec_dep_load_state = RecursiveDependencyLoadState::Failed; + info.load_state = LoadState::Failed(error.clone()); + info.dep_load_state = DependencyLoadState::Failed(error.clone()); + info.rec_dep_load_state = RecursiveDependencyLoadState::Failed(error.clone()); ( std::mem::take(&mut info.dependants_waiting_on_load), std::mem::take(&mut info.dependants_waiting_on_recursive_dep_load), @@ -613,12 +625,15 @@ impl AssetInfos { if let Some(info) = self.get_mut(waiting_id) { info.loading_dependencies.remove(&failed_id); info.failed_dependencies.insert(failed_id); - info.dep_load_state = DependencyLoadState::Failed; + // don't overwrite DependencyLoadState if already failed to preserve first error + if !(matches!(info.dep_load_state, DependencyLoadState::Failed(_))) { + info.dep_load_state = DependencyLoadState::Failed(error.clone()); + } } } for waiting_id in dependants_waiting_on_rec_load { - Self::propagate_failed_state(self, failed_id, waiting_id); + Self::propagate_failed_state(self, failed_id, waiting_id, &error); } } diff --git a/crates/bevy_asset/src/server/mod.rs b/crates/bevy_asset/src/server/mod.rs index a9e5fcddbf31c..61920e68db399 100644 --- a/crates/bevy_asset/src/server/mod.rs +++ b/crates/bevy_asset/src/server/mod.rs @@ -895,26 +895,49 @@ impl AssetServer { &self, id: impl Into, ) -> Option<(LoadState, DependencyLoadState, RecursiveDependencyLoadState)> { + self.data.infos.read().get(id.into()).map(|i| { + ( + i.load_state.clone(), + i.dep_load_state.clone(), + i.rec_dep_load_state.clone(), + ) + }) + } + + /// Retrieves the main [`LoadState`] of a given asset `id`. + /// + /// Note that this is "just" the root asset load state. To get the load state of + /// its dependencies or recursive dependencies, see [`AssetServer::get_dependency_load_state`] + /// and [`AssetServer::get_recursive_dependency_load_state`] respectively. + pub fn get_load_state(&self, id: impl Into) -> Option { self.data .infos .read() .get(id.into()) - .map(|i| (i.load_state.clone(), i.dep_load_state, i.rec_dep_load_state)) + .map(|i| i.load_state.clone()) } - /// Retrieves the main [`LoadState`] of a given asset `id`. + /// Retrieves the [`DependencyLoadState`] of a given asset `id`'s dependencies. /// - /// Note that this is "just" the root asset load state. To check if an asset _and_ its recursive - /// dependencies have loaded, see [`AssetServer::is_loaded_with_dependencies`]. - pub fn get_load_state(&self, id: impl Into) -> Option { + /// Note that this is only the load state of direct dependencies of the root asset. To get + /// the load state of the root asset itself or its recursive dependencies, see + /// [`AssetServer::get_load_state`] and [`AssetServer::get_recursive_dependency_load_state`] respectively. + pub fn get_dependency_load_state( + &self, + id: impl Into, + ) -> Option { self.data .infos .read() .get(id.into()) - .map(|i| i.load_state.clone()) + .map(|i| i.dep_load_state.clone()) } - /// Retrieves the [`RecursiveDependencyLoadState`] of a given asset `id`. + /// Retrieves the main [`RecursiveDependencyLoadState`] of a given asset `id`'s recursive dependencies. + /// + /// Note that this is only the load state of recursive dependencies of the root asset. To get + /// the load state of the root asset itself or its direct dependencies only, see + /// [`AssetServer::get_load_state`] and [`AssetServer::get_dependency_load_state`] respectively. pub fn get_recursive_dependency_load_state( &self, id: impl Into, @@ -923,15 +946,30 @@ impl AssetServer { .infos .read() .get(id.into()) - .map(|i| i.rec_dep_load_state) + .map(|i| i.rec_dep_load_state.clone()) } /// Retrieves the main [`LoadState`] of a given asset `id`. + /// + /// This is the same as [`AssetServer::get_load_state`] except the result is unwrapped. If + /// the result is None, [`LoadState::NotLoaded`] is returned. pub fn load_state(&self, id: impl Into) -> LoadState { self.get_load_state(id).unwrap_or(LoadState::NotLoaded) } + /// Retrieves the [`DependencyLoadState`] of a given asset `id`. + /// + /// This is the same as [`AssetServer::get_dependency_load_state`] except the result is unwrapped. If + /// the result is None, [`DependencyLoadState::NotLoaded`] is returned. + pub fn dependency_load_state(&self, id: impl Into) -> DependencyLoadState { + self.get_dependency_load_state(id) + .unwrap_or(DependencyLoadState::NotLoaded) + } + /// Retrieves the [`RecursiveDependencyLoadState`] of a given asset `id`. + /// + /// This is the same as [`AssetServer::get_recursive_dependency_load_state`] except the result is unwrapped. If + /// the result is None, [`RecursiveDependencyLoadState::NotLoaded`] is returned. pub fn recursive_dependency_load_state( &self, id: impl Into, @@ -940,11 +978,30 @@ impl AssetServer { .unwrap_or(RecursiveDependencyLoadState::NotLoaded) } - /// Returns true if the asset and all of its dependencies (recursive) have been loaded. + /// Convenience method that returns true if the asset has been loaded. + pub fn is_loaded(&self, id: impl Into) -> bool { + matches!(self.load_state(id), LoadState::Loaded) + } + + /// Convenience method that returns true if the asset and all of its direct dependencies have been loaded. pub fn is_loaded_with_dependencies(&self, id: impl Into) -> bool { - let id = id.into(); - self.load_state(id) == LoadState::Loaded - && self.recursive_dependency_load_state(id) == RecursiveDependencyLoadState::Loaded + matches!( + self.get_load_states(id), + Some((LoadState::Loaded, DependencyLoadState::Loaded, _)) + ) + } + + /// Convenience method that returns true if the asset, all of its dependencies, and all of its recursive + /// dependencies have been loaded. + pub fn is_loaded_with_recursive_dependencies(&self, id: impl Into) -> bool { + matches!( + self.get_load_states(id), + Some(( + LoadState::Loaded, + DependencyLoadState::Loaded, + RecursiveDependencyLoadState::Loaded + )) + ) } /// Returns an active handle for the given path, if the asset at the given path has already started loading, @@ -1360,12 +1417,14 @@ pub enum LoadState { Loading, /// The asset has been loaded and has been added to the [`World`] Loaded, - /// The asset failed to load. - Failed(Box), + /// The asset failed to load. The underlying [`AssetLoadError`] is + /// referenced by [`Arc`] clones in all related [`DependencyLoadState`]s + /// and [`RecursiveDependencyLoadState`]s in the asset's dependency tree. + Failed(Arc), } /// The load state of an asset's dependencies. -#[derive(Component, Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd)] +#[derive(Component, Clone, Debug, Eq, PartialEq)] pub enum DependencyLoadState { /// The asset has not started loading yet NotLoaded, @@ -1373,12 +1432,14 @@ pub enum DependencyLoadState { Loading, /// Dependencies have all loaded Loaded, - /// One or more dependencies have failed to load - Failed, + /// One or more dependencies have failed to load. The underlying [`AssetLoadError`] + /// is referenced by [`Arc`] clones in all related [`LoadState`] and + /// [`RecursiveDependencyLoadState`]s in the asset's dependency tree. + Failed(Arc), } /// The recursive load state of an asset's dependencies. -#[derive(Component, Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd)] +#[derive(Component, Clone, Debug, Eq, PartialEq)] pub enum RecursiveDependencyLoadState { /// The asset has not started loading yet NotLoaded, @@ -1386,8 +1447,11 @@ pub enum RecursiveDependencyLoadState { Loading, /// Dependencies in this asset's dependency tree have all loaded Loaded, - /// One or more dependencies have failed to load in this asset's dependency tree - Failed, + /// One or more dependencies have failed to load in this asset's dependency + /// tree. The underlying [`AssetLoadError`] is referenced by [`Arc`] clones + /// in all related [`LoadState`]s and [`DependencyLoadState`]s in the asset's + /// dependency tree. + Failed(Arc), } /// An error that occurs during an [`Asset`] load. From b5ca4002cf6a0b007dfcc8561bd4fa2dffe81575 Mon Sep 17 00:00:00 2001 From: Cole Varner Date: Thu, 19 Sep 2024 12:04:00 -0700 Subject: [PATCH 2/2] renames to preserve original semantics of is_loaded_with_dependencies --- crates/bevy_asset/src/lib.rs | 4 ++-- crates/bevy_asset/src/server/mod.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/bevy_asset/src/lib.rs b/crates/bevy_asset/src/lib.rs index eec01bef5baaf..58a49ddcee84c 100644 --- a/crates/bevy_asset/src/lib.rs +++ b/crates/bevy_asset/src/lib.rs @@ -1133,8 +1133,8 @@ mod tests { )); assert!(asset_server.is_loaded(a_id)); - assert!(asset_server.is_loaded_with_dependencies(a_id)); - assert!(!asset_server.is_loaded_with_recursive_dependencies(a_id)); + assert!(asset_server.is_loaded_with_direct_dependencies(a_id)); + assert!(!asset_server.is_loaded_with_dependencies(a_id)); Some(()) }); diff --git a/crates/bevy_asset/src/server/mod.rs b/crates/bevy_asset/src/server/mod.rs index 61920e68db399..c8c7ca81348c2 100644 --- a/crates/bevy_asset/src/server/mod.rs +++ b/crates/bevy_asset/src/server/mod.rs @@ -984,7 +984,7 @@ impl AssetServer { } /// Convenience method that returns true if the asset and all of its direct dependencies have been loaded. - pub fn is_loaded_with_dependencies(&self, id: impl Into) -> bool { + pub fn is_loaded_with_direct_dependencies(&self, id: impl Into) -> bool { matches!( self.get_load_states(id), Some((LoadState::Loaded, DependencyLoadState::Loaded, _)) @@ -993,7 +993,7 @@ impl AssetServer { /// Convenience method that returns true if the asset, all of its dependencies, and all of its recursive /// dependencies have been loaded. - pub fn is_loaded_with_recursive_dependencies(&self, id: impl Into) -> bool { + pub fn is_loaded_with_dependencies(&self, id: impl Into) -> bool { matches!( self.get_load_states(id), Some((