Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix untyped labeled asset loading #10514

Merged
merged 6 commits into from
Nov 14, 2023
Merged

Fix untyped labeled asset loading #10514

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d6e6ae3
Fix untyped labeled asset loading
cart Nov 11, 2023
0db8153
Add details about duplicate loads and issue link
cart Nov 14, 2023
43453ff
Use TODO: syntax
cart Nov 14, 2023
6218f28
pub(crate)
cart Nov 14, 2023
a70c7ca
Add doc string and make pub(crate)
cart Nov 14, 2023
3a1275b
Add docs for load_untyped_async
cart Nov 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 37 additions & 14 deletions crates/bevy_asset/src/server/info.rs
View file
Open in desktop
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>,
cart marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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>(
cart marked this conversation as resolved.
Show resolved Hide resolved
&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 {
cart marked this conversation as resolved.
Show resolved Hide resolved
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