forestia/bevy
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix untyped labeled asset loading (#10514)

Browse Source 
# 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.
This commit is contained in:
Carter Anderson 2023-11-13 18:41:10 -08:00 committed by GitHub
parent b1aa74d511
commit 749f3d7430
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 94 additions and 42 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

51
crates/bevy_asset/src/server/info.rs
View File

@ -104,6 +104,7 @@ impl AssetInfos {
), ),
type_name, type_name,
) )
.unwrap()
} }
#[allow(clippy::too_many_arguments)] #[allow(clippy::too_many_arguments)]
@ -116,7 +117,7 @@ impl AssetInfos {
path: Option<AssetPath<'static>>, path: Option<AssetPath<'static>>,
meta_transform: Option<MetaTransform>, meta_transform: Option<MetaTransform>,
loading: bool, loading: bool,
) -> Result<UntypedHandle, MissingHandleProviderError> { ) -> Result<UntypedHandle, GetOrCreateHandleInternalError> {
let provider = handle_providers let provider = handle_providers
.get(&type_id) .get(&type_id)
.ok_or(MissingHandleProviderError(type_id))?; .ok_or(MissingHandleProviderError(type_id))?;
@ -151,11 +152,13 @@ impl AssetInfos {
) -> (Handle<A>, bool) { ) -> (Handle<A>, bool) {
let result = self.get_or_create_path_handle_internal( let result = self.get_or_create_path_handle_internal(
path, path,
TypeId::of::<A>(), Some(TypeId::of::<A>()),
loading_mode, loading_mode,
meta_transform, meta_transform,
); );
let (handle, should_load) = unwrap_with_context(result, std::any::type_name::<A>()); // it is ok to unwrap because TypeId was specified above
let (handle, should_load) =
unwrap_with_context(result, std::any::type_name::<A>()).unwrap();
(handle.typed_unchecked(), should_load) (handle.typed_unchecked(), should_load)
} }
@ -167,20 +170,25 @@ impl AssetInfos {
loading_mode: HandleLoadingMode, loading_mode: HandleLoadingMode,
meta_transform: Option<MetaTransform>, meta_transform: Option<MetaTransform>,
) -> (UntypedHandle, bool) { ) -> (UntypedHandle, bool) {
let result = let result = self.get_or_create_path_handle_internal(
self.get_or_create_path_handle_internal(path, type_id, loading_mode, meta_transform); path,
unwrap_with_context(result, type_name) 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. /// Retrieves asset tracking data, or creates it if it doesn't exist.
/// Returns true if an asset load should be kicked off /// 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, &mut self,
path: AssetPath<'static>, path: AssetPath<'static>,
type_id: TypeId, type_id: Option<TypeId>,
loading_mode: HandleLoadingMode, loading_mode: HandleLoadingMode,
meta_transform: Option<MetaTransform>, meta_transform: Option<MetaTransform>,
) -> Result<(UntypedHandle, bool), MissingHandleProviderError> { ) -> Result<(UntypedHandle, bool), GetOrCreateHandleInternalError> {
match self.path_to_id.entry(path.clone()) { match self.path_to_id.entry(path.clone()) {
Entry::Occupied(entry) => { Entry::Occupied(entry) => {
let id = *entry.get(); 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 // 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 // strong handle doesn't remove the asset from the Assets collection
info.handle_drops_to_skip += 1; info.handle_drops_to_skip += 1;
let type_id = type_id.ok_or(
GetOrCreateHandleInternalError::HandleMissingButTypeIdNotSpecified,
)?;
let provider = self let provider = self
.handle_providers .handle_providers
.get(&type_id) .get(&type_id)
@ -227,6 +238,8 @@ impl AssetInfos {
HandleLoadingMode::NotLoading => false, HandleLoadingMode::NotLoading => false,
HandleLoadingMode::Request | HandleLoadingMode::Force => true, HandleLoadingMode::Request | HandleLoadingMode::Force => true,
}; };
let type_id = type_id
.ok_or(GetOrCreateHandleInternalError::HandleMissingButTypeIdNotSpecified)?;
let handle = Self::create_handle_internal( let handle = Self::create_handle_internal(
&mut self.infos, &mut self.infos,
&self.handle_providers, &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:?}")] #[error("Cannot allocate a handle because no handle provider exists for asset type {0:?}")]
pub struct MissingHandleProviderError(TypeId); pub struct MissingHandleProviderError(TypeId);
fn unwrap_with_context<T>( /// An error encountered during [`AssetInfos::get_or_create_path_handle_internal`].
result: Result<T, MissingHandleProviderError>, #[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<T>(
result: Result<T, GetOrCreateHandleInternalError>,
type_name: &'static str, type_name: &'static str,
) -> T { ) -> Option<T> {
match result { match result {
Ok(value) => value, Ok(value) => Some(value),
Err(_) => { 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. \ 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}>()") Make sure you have called app.init_asset::<{type_name}>()")
} }

85
crates/bevy_asset/src/server/mod.rs
View File

@ -280,8 +280,11 @@ impl AssetServer {
handle 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"] #[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, &self,
path: impl Into<AssetPath<'a>>, path: impl Into<AssetPath<'a>>,
) -> Result<UntypedHandle, AssetLoadError> { ) -> Result<UntypedHandle, AssetLoadError> {
@ -379,35 +382,53 @@ impl AssetServer {
e 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) => { Some(handle) => {
// if a handle was passed in, the "should load" check was already done // if a handle was passed in, the "should load" check was already done
(handle, true) Some((handle, true))
} }
None => { None => {
let mut infos = self.data.infos.write(); 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(), path.clone(),
loader.asset_type_id(), path.label().is_none().then(|| loader.asset_type_id()),
loader.asset_type_name(),
HandleLoadingMode::Request, HandleLoadingMode::Request,
meta_transform, meta_transform,
) );
unwrap_with_context(result, loader.asset_type_name())
} }
}; };
if path.label().is_none() && handle.type_id() != loader.asset_type_id() { let handle = if let Some((handle, should_load)) = handle_result {
return Err(AssetLoadError::RequestedHandleTypeMismatch { if path.label().is_none() && handle.type_id() != loader.asset_type_id() {
path: path.into_owned(), return Err(AssetLoadError::RequestedHandleTypeMismatch {
requested: handle.type_id(), path: path.into_owned(),
actual_asset_name: loader.asset_type_name(), requested: handle.type_id(),
loader_name: loader.type_name(), actual_asset_name: loader.asset_type_name(),
}); loader_name: loader.type_name(),
} });
}
if !should_load && !force { if !should_load && !force {
return Ok(handle); 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 (base_handle, base_path) = if path.label().is_some() {
let mut infos = self.data.infos.write(); let mut infos = self.data.infos.write();
@ -421,7 +442,7 @@ impl AssetServer {
); );
(base_handle, base_path) (base_handle, base_path)
} else { } else {
(handle.clone(), path.clone()) (handle.clone().unwrap(), path.clone())
}; };
if let Some(meta_transform) = base_handle.meta_transform() { if let Some(meta_transform) = base_handle.meta_transform() {
@ -433,25 +454,33 @@ impl AssetServer {
.await .await
{ {
Ok(mut loaded_asset) => { Ok(mut loaded_asset) => {
if let Some(label) = path.label_cow() { let final_handle = if let Some(label) = path.label_cow() {
if !loaded_asset.labeled_assets.contains_key(&label) { match loaded_asset.labeled_assets.get(&label) {
return Err(AssetLoadError::MissingLabel { Some(labeled_asset) => labeled_asset.handle.clone(),
base_path, None => {
label: label.to_string(), 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() { for (_, labeled_asset) in loaded_asset.labeled_assets.drain() {
self.send_asset_event(InternalAssetEvent::Loaded { self.send_asset_event(InternalAssetEvent::Loaded {
id: labeled_asset.handle.id(), id: labeled_asset.handle.id(),
loaded_asset: labeled_asset.asset, loaded_asset: labeled_asset.asset,
}); });
} }
self.send_asset_event(InternalAssetEvent::Loaded { self.send_asset_event(InternalAssetEvent::Loaded {
id: base_handle.id(), id: base_handle.id(),
loaded_asset, loaded_asset,
}); });
Ok(handle) Ok(final_handle)
} }
Err(err) => { Err(err) => {
self.send_asset_event(InternalAssetEvent::Failed { self.send_asset_event(InternalAssetEvent::Failed {