From 02943737b25add58262f95346969ea068d491601 Mon Sep 17 00:00:00 2001 From: Carter Anderson Date: Sun, 15 Oct 2023 11:36:51 -0700 Subject: [PATCH] Return an error when loading non-existent labels (#9751) # Objective Calling `asset_server.load("scene.gltf#SomeLabel")` will silently fail if `SomeLabel` does not exist. Referenced in #9714 ## Solution We now detect this case and return an error. I also slightly refactored `load_internal` to make the logic / dataflow much clearer. --------- Co-authored-by: Pascal Hertleif --- crates/bevy_asset/src/path.rs | 6 +++ crates/bevy_asset/src/server/mod.rs | 72 +++++++++++++++++------------ 2 files changed, 48 insertions(+), 30 deletions(-) diff --git a/crates/bevy_asset/src/path.rs b/crates/bevy_asset/src/path.rs index 11168ca245..43b7d2cab5 100644 --- a/crates/bevy_asset/src/path.rs +++ b/crates/bevy_asset/src/path.rs @@ -203,6 +203,12 @@ impl<'a> AssetPath<'a> { self.label.as_deref() } + /// Gets the "sub-asset label". + #[inline] + pub fn label_cow(&self) -> Option> { + self.label.clone() + } + /// Gets the path to the asset in the "virtual filesystem". #[inline] pub fn path(&self) -> &Path { diff --git a/crates/bevy_asset/src/server/mod.rs b/crates/bevy_asset/src/server/mod.rs index 03cf286349..160a3e4d32 100644 --- a/crates/bevy_asset/src/server/mod.rs +++ b/crates/bevy_asset/src/server/mod.rs @@ -257,7 +257,7 @@ impl AssetServer { path: impl Into>, meta_transform: Option, ) -> Handle { - let mut path = path.into().into_owned(); + let path = path.into().into_owned(); let (handle, should_load) = self.data.infos.write().get_or_create_path_handle::( path.clone(), HandleLoadingMode::Request, @@ -265,13 +265,10 @@ impl AssetServer { ); if should_load { - let mut owned_handle = Some(handle.clone().untyped()); + let owned_handle = Some(handle.clone().untyped()); let server = self.clone(); IoTaskPool::get() .spawn(async move { - if path.take_label().is_some() { - owned_handle = None; - } if let Err(err) = server.load_internal(owned_handle, path, false, None).await { error!("{}", err); } @@ -291,6 +288,10 @@ impl AssetServer { self.load_internal(None, path, false, None).await } + /// Performs an async asset load. + /// + /// `input_handle` must only be [`Some`] if `should_load` was true when retrieving `input_handle`. This is an optimization to + /// avoid looking up `should_load` twice, but it means you _must_ be sure a load is necessary when calling this function with [`Some`]. async fn load_internal<'a>( &self, input_handle: Option, @@ -298,7 +299,7 @@ impl AssetServer { force: bool, meta_transform: Option, ) -> Result { - let mut path = path.into_owned(); + let path = path.into_owned(); let path_clone = path.clone(); let (mut meta, loader, mut reader) = self .get_meta_loader_and_reader(&path_clone) @@ -312,18 +313,8 @@ impl AssetServer { e })?; - let has_label = path.label().is_some(); - let (handle, should_load) = match input_handle { Some(handle) => { - if !has_label && handle.type_id() != loader.asset_type_id() { - return Err(AssetLoadError::RequestedHandleTypeMismatch { - path: path.into_owned(), - requested: handle.type_id(), - actual_asset_name: loader.asset_type_name(), - loader_name: loader.type_name(), - }); - } // if a handle was passed in, the "should load" check was already done (handle, true) } @@ -339,37 +330,51 @@ impl AssetServer { } }; + if path.label().is_none() && handle.type_id() != loader.asset_type_id() { + return Err(AssetLoadError::RequestedHandleTypeMismatch { + path: path.into_owned(), + requested: handle.type_id(), + actual_asset_name: loader.asset_type_name(), + loader_name: loader.type_name(), + }); + } + if !should_load && !force { return Ok(handle); } - let base_asset_id = if has_label { - path.remove_label(); - // If the path has a label, the current id does not match the asset root type. - // We need to get the actual asset id + + let (base_handle, base_path) = if path.label().is_some() { let mut infos = self.data.infos.write(); - let (actual_handle, _) = infos.get_or_create_path_handle_untyped( - path.clone(), + let base_path = path.without_label().into_owned(); + let (base_handle, _) = infos.get_or_create_path_handle_untyped( + base_path.clone(), loader.asset_type_id(), loader.asset_type_name(), - // ignore current load state ... we kicked off this sub asset load because it needed to be loaded but - // does not currently exist HandleLoadingMode::Force, None, ); - actual_handle.id() + (base_handle, base_path) } else { - handle.id() + (handle.clone(), path.clone()) }; - if let Some(meta_transform) = handle.meta_transform() { + if let Some(meta_transform) = base_handle.meta_transform() { (*meta_transform)(&mut *meta); } match self - .load_with_meta_loader_and_reader(&path, meta, &*loader, &mut *reader, true, false) + .load_with_meta_loader_and_reader(&base_path, meta, &*loader, &mut *reader, true, false) .await { Ok(mut loaded_asset) => { + if let Some(label) = path.label_cow() { + if !loaded_asset.labeled_assets.contains_key(&label) { + return Err(AssetLoadError::MissingLabel { + base_path, + label: label.to_string(), + }); + } + } for (_, labeled_asset) in loaded_asset.labeled_assets.drain() { self.send_asset_event(InternalAssetEvent::Loaded { id: labeled_asset.handle.id(), @@ -377,13 +382,15 @@ impl AssetServer { }); } self.send_asset_event(InternalAssetEvent::Loaded { - id: base_asset_id, + id: base_handle.id(), loaded_asset, }); Ok(handle) } Err(err) => { - self.send_asset_event(InternalAssetEvent::Failed { id: base_asset_id }); + self.send_asset_event(InternalAssetEvent::Failed { + id: base_handle.id(), + }); Err(err) } } @@ -935,6 +942,11 @@ pub enum AssetLoadError { loader_name: &'static str, error: Box, }, + #[error("The file at '{base_path}' does not contain the labeled asset '{label}'.")] + MissingLabel { + base_path: AssetPath<'static>, + label: String, + }, } /// An error that occurs when an [`AssetLoader`] is not registered for a given extension.