Skip to content

Commit

Permalink
Fix untyped labeled asset loading (bevyengine#10514)
Browse files Browse the repository at this point in the history 
# Objective

Fixes bevyengine#10436
Alternative to bevyengine#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.
  • Loading branch information
@cart
cart authored and Ray Redondo committed Jan 9, 2024
1 parent c69d1be commit 0100130
Show file tree
Hide file tree
Showing 2 changed files with 94 additions and 42 deletions.
51 changes: 37 additions & 14 deletions crates/bevy_asset/src/server/info.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ impl AssetInfos {
),
type_name,
)
.unwrap()
}

#[allow(clippy::too_many_arguments)]
Expand All @@ -116,7 +117,7 @@ impl AssetInfos {
path: Option<AssetPath<'static>>,
meta_transform: Option<MetaTransform>,
loading: bool,
) -> Result<UntypedHandle, MissingHandleProviderError> {
) -> Result<UntypedHandle, GetOrCreateHandleInternalError> {
let provider = handle_providers
.get(&type_id)
.ok_or(MissingHandleProviderError(type_id))?;
Expand Down Expand Up @@ -151,11 +152,13 @@ impl AssetInfos {
) -> (Handle<A>, bool) {
let result = self.get_or_create_path_handle_internal(
path,
TypeId::of::<A>(),
Some(TypeId::of::<A>()),
loading_mode,
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)
}

Expand All @@ -167,20 +170,25 @@ impl AssetInfos {
loading_mode: HandleLoadingMode,
meta_transform: Option<MetaTransform>,
) -> (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<TypeId>,
loading_mode: HandleLoadingMode,
meta_transform: Option<MetaTransform>,
) -> Result<(UntypedHandle, bool), MissingHandleProviderError> {
) -> Result<(UntypedHandle, bool), GetOrCreateHandleInternalError> {
match self.path_to_id.entry(path.clone()) {
Entry::Occupied(entry) => {
let id = *entry.get();
Expand Down Expand Up @@ -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)
Expand All @@ -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,
Expand Down Expand Up @@ -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<T>(
result: Result<T, MissingHandleProviderError>,
/// 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<T>(
result: Result<T, GetOrCreateHandleInternalError>,
type_name: &'static str,
) -> T {
) -> Option<T> {
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}>()")
}
Expand Down
85 changes: 57 additions & 28 deletions crates/bevy_asset/src/server/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<AssetPath<'a>>,
) -> Result<UntypedHandle, AssetLoadError> {
Expand Down Expand Up @@ -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();
Expand All @@ -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() {
Expand All @@ -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 {
Expand Down

0 comments on commit 0100130

Please sign in to comment.