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 1 commit
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
48 changes: 35 additions & 13 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(
&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,22 @@ 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>,
#[derive(Error, Debug)]
pub enum GetOrCreateHandleInternalError {
cart marked this conversation as resolved.
Show resolved Hide resolved
#[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
77 changes: 49 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 @@ -281,7 +281,7 @@ impl AssetServer {
}

#[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 +379,48 @@ 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.
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 +434,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 +446,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