From 0100130a7c0bcd9660a587b87a34f977ef0cde99 Mon Sep 17 00:00:00 2001 From: Carter Anderson Date: Mon, 13 Nov 2023 18:41:10 -0800 Subject: [PATCH] Fix untyped labeled asset loading (#10514) # Objective Fixes #10436 Alternative to #10465 ## Solution `load_untyped_async` / `load_internal` currently has a bug. In `load_untyped_async`, we pass None into `load_internal` for the `UntypedHandle` of the labeled asset path. This results in a call to `get_or_create_path_handle_untyped` with `loader.asset_type_id()` This is a new code path that wasn't hit prior to the newly added `load_untyped` because `load_untyped_async` was a private method only used in the context of the `load_folder` impl (which doesn't have labels) The fix required some refactoring to catch that case and defer handle retrieval. I have also made `load_untyped_async` public as it is now "ready for public use" and unlocks new scenarios. --- crates/bevy_asset/src/server/info.rs | 51 ++++++++++++----- crates/bevy_asset/src/server/mod.rs | 85 +++++++++++++++++++--------- 2 files changed, 94 insertions(+), 42 deletions(-) diff --git a/crates/bevy_asset/src/server/info.rs b/crates/bevy_asset/src/server/info.rs index b81c3feb07f70..2786a7d4b86b3 100644 --- a/crates/bevy_asset/src/server/info.rs +++ b/crates/bevy_asset/src/server/info.rs @@ -104,6 +104,7 @@ impl AssetInfos { ), type_name, ) + .unwrap() } #[allow(clippy::too_many_arguments)] @@ -116,7 +117,7 @@ impl AssetInfos { path: Option>, meta_transform: Option, loading: bool, - ) -> Result { + ) -> Result { let provider = handle_providers .get(&type_id) .ok_or(MissingHandleProviderError(type_id))?; @@ -151,11 +152,13 @@ impl AssetInfos { ) -> (Handle, bool) { let result = self.get_or_create_path_handle_internal( path, - TypeId::of::(), + Some(TypeId::of::()), loading_mode, meta_transform, ); - let (handle, should_load) = unwrap_with_context(result, std::any::type_name::()); + // it is ok to unwrap because TypeId was specified above + let (handle, should_load) = + unwrap_with_context(result, std::any::type_name::()).unwrap(); (handle.typed_unchecked(), should_load) } @@ -167,20 +170,25 @@ impl AssetInfos { loading_mode: HandleLoadingMode, meta_transform: Option, ) -> (UntypedHandle, bool) { - let result = - self.get_or_create_path_handle_internal(path, type_id, loading_mode, meta_transform); - unwrap_with_context(result, type_name) + let result = self.get_or_create_path_handle_internal( + path, + Some(type_id), + loading_mode, + meta_transform, + ); + // it is ok to unwrap because TypeId was specified above + unwrap_with_context(result, type_name).unwrap() } /// Retrieves asset tracking data, or creates it if it doesn't exist. /// Returns true if an asset load should be kicked off - pub fn get_or_create_path_handle_internal( + pub(crate) fn get_or_create_path_handle_internal( &mut self, path: AssetPath<'static>, - type_id: TypeId, + type_id: Option, loading_mode: HandleLoadingMode, meta_transform: Option, - ) -> Result<(UntypedHandle, bool), MissingHandleProviderError> { + ) -> Result<(UntypedHandle, bool), GetOrCreateHandleInternalError> { match self.path_to_id.entry(path.clone()) { Entry::Occupied(entry) => { let id = *entry.get(); @@ -211,6 +219,9 @@ impl AssetInfos { // We must create a new strong handle for the existing id and ensure that the drop of the old // strong handle doesn't remove the asset from the Assets collection info.handle_drops_to_skip += 1; + let type_id = type_id.ok_or( + GetOrCreateHandleInternalError::HandleMissingButTypeIdNotSpecified, + )?; let provider = self .handle_providers .get(&type_id) @@ -227,6 +238,8 @@ impl AssetInfos { HandleLoadingMode::NotLoading => false, HandleLoadingMode::Request | HandleLoadingMode::Force => true, }; + let type_id = type_id + .ok_or(GetOrCreateHandleInternalError::HandleMissingButTypeIdNotSpecified)?; let handle = Self::create_handle_internal( &mut self.infos, &self.handle_providers, @@ -624,13 +637,23 @@ pub(crate) enum HandleLoadingMode { #[error("Cannot allocate a handle because no handle provider exists for asset type {0:?}")] pub struct MissingHandleProviderError(TypeId); -fn unwrap_with_context( - result: Result, +/// An error encountered during [`AssetInfos::get_or_create_path_handle_internal`]. +#[derive(Error, Debug)] +pub(crate) enum GetOrCreateHandleInternalError { + #[error(transparent)] + MissingHandleProviderError(#[from] MissingHandleProviderError), + #[error("Handle does not exist but TypeId was not specified.")] + HandleMissingButTypeIdNotSpecified, +} + +pub(crate) fn unwrap_with_context( + result: Result, type_name: &'static str, -) -> T { +) -> Option { match result { - Ok(value) => value, - Err(_) => { + Ok(value) => Some(value), + Err(GetOrCreateHandleInternalError::HandleMissingButTypeIdNotSpecified) => None, + Err(GetOrCreateHandleInternalError::MissingHandleProviderError(_)) => { panic!("Cannot allocate an Asset Handle of type '{type_name}' because the asset type has not been initialized. \ Make sure you have called app.init_asset::<{type_name}>()") } diff --git a/crates/bevy_asset/src/server/mod.rs b/crates/bevy_asset/src/server/mod.rs index 864faf70fc71b..fb9ff73a5796f 100644 --- a/crates/bevy_asset/src/server/mod.rs +++ b/crates/bevy_asset/src/server/mod.rs @@ -280,8 +280,11 @@ impl AssetServer { handle } + /// Asynchronously load an asset that you do not know the type of statically. If you _do_ know the type of the asset, + /// you should use [`AssetServer::load`]. If you don't know the type of the asset, but you can't use an async method, + /// consider using [`AssetServer::load_untyped`]. #[must_use = "not using the returned strong handle may result in the unexpected release of the asset"] - pub(crate) async fn load_untyped_async<'a>( + pub async fn load_untyped_async<'a>( &self, path: impl Into>, ) -> Result { @@ -379,35 +382,53 @@ impl AssetServer { e })?; - let (handle, should_load) = match input_handle { + // This contains Some(UntypedHandle), if it was retrievable + // If it is None, that is because it was _not_ retrievable, due to + // 1. The handle was not already passed in for this path, meaning we can't just use that + // 2. The asset has not been loaded yet, meaning there is no existing Handle for it + // 3. The path has a label, meaning the AssetLoader's root asset type is not the path's asset type + // + // In the None case, the only course of action is to wait for the asset to load so we can allocate the + // handle for that type. + // + // TODO: Note that in the None case, multiple asset loads for the same path can happen at the same time + // (rather than "early out-ing" in the "normal" case) + // This would be resolved by a universal asset id, as we would not need to resolve the asset type + // to generate the ID. See this issue: https://github.com/bevyengine/bevy/issues/10549 + let handle_result = match input_handle { Some(handle) => { // if a handle was passed in, the "should load" check was already done - (handle, true) + Some((handle, true)) } None => { let mut infos = self.data.infos.write(); - infos.get_or_create_path_handle_untyped( + let result = infos.get_or_create_path_handle_internal( path.clone(), - loader.asset_type_id(), - loader.asset_type_name(), + path.label().is_none().then(|| loader.asset_type_id()), HandleLoadingMode::Request, meta_transform, - ) + ); + unwrap_with_context(result, loader.asset_type_name()) } }; - 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 handle = if let Some((handle, should_load)) = handle_result { + 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); + } + Some(handle) + } else { + None + }; + // if the handle result is None, we definitely need to load the asset let (base_handle, base_path) = if path.label().is_some() { let mut infos = self.data.infos.write(); @@ -421,7 +442,7 @@ impl AssetServer { ); (base_handle, base_path) } else { - (handle.clone(), path.clone()) + (handle.clone().unwrap(), path.clone()) }; if let Some(meta_transform) = base_handle.meta_transform() { @@ -433,25 +454,33 @@ impl AssetServer { .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(), - }); + let final_handle = if let Some(label) = path.label_cow() { + match loaded_asset.labeled_assets.get(&label) { + Some(labeled_asset) => labeled_asset.handle.clone(), + None => { + return Err(AssetLoadError::MissingLabel { + base_path, + label: label.to_string(), + }); + } } - } + } else { + // if the path does not have a label, the handle must exist at this point + handle.unwrap() + }; + for (_, labeled_asset) in loaded_asset.labeled_assets.drain() { self.send_asset_event(InternalAssetEvent::Loaded { id: labeled_asset.handle.id(), loaded_asset: labeled_asset.asset, }); } + self.send_asset_event(InternalAssetEvent::Loaded { id: base_handle.id(), loaded_asset, }); - Ok(handle) + Ok(final_handle) } Err(err) => { self.send_asset_event(InternalAssetEvent::Failed {