Skip to content

Commit 3805896

Browse files
authored
Restructure the logic in AssetServer::load_internal to avoid using weak handles. (#19634)
# Objective - A step towards #19024. - The logic here was kinda complex before. ## Solution - I've restructured the logic here while preserving the behavior (as far as I can tell). - We no longer return the handle if it was passed in. The caller should already have access to it, and the returned handle will be a weak handle, not a strong handle (which can cause issues). This prevents us from needing weak handles at all here. - I verified the callers do not need the return value. The only callsite that needs the returned handle does not pass in the input_handle argument. ## Testing - CI
1 parent 2ea8f77 commit 3805896

File tree

1 file changed

+86
-67
lines changed
  • crates/bevy_asset/src/server

1 file changed

+86
-67
lines changed

crates/bevy_asset/src/server/mod.rs

Lines changed: 86 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -553,7 +553,9 @@ impl AssetServer {
553553
path: impl Into<AssetPath<'a>>,
554554
) -> Result<UntypedHandle, AssetLoadError> {
555555
let path: AssetPath = path.into();
556-
self.load_internal(None, path, false, None).await
556+
self.load_internal(None, path, false, None)
557+
.await
558+
.map(|h| h.expect("handle must be returned, since we didn't pass in an input handle"))
557559
}
558560

559561
pub(crate) fn load_unknown_type_with_meta_transform<'a>(
@@ -643,21 +645,25 @@ impl AssetServer {
643645

644646
/// Performs an async asset load.
645647
///
646-
/// `input_handle` must only be [`Some`] if `should_load` was true when retrieving `input_handle`. This is an optimization to
647-
/// avoid looking up `should_load` twice, but it means you _must_ be sure a load is necessary when calling this function with [`Some`].
648+
/// `input_handle` must only be [`Some`] if `should_load` was true when retrieving
649+
/// `input_handle`. This is an optimization to avoid looking up `should_load` twice, but it
650+
/// means you _must_ be sure a load is necessary when calling this function with [`Some`].
651+
///
652+
/// Returns the handle of the asset if one was retrieved by this function. Otherwise, may return
653+
/// [`None`].
648654
async fn load_internal<'a>(
649655
&self,
650-
mut input_handle: Option<UntypedHandle>,
656+
input_handle: Option<UntypedHandle>,
651657
path: AssetPath<'a>,
652658
force: bool,
653659
meta_transform: Option<MetaTransform>,
654-
) -> Result<UntypedHandle, AssetLoadError> {
655-
let asset_type_id = input_handle.as_ref().map(UntypedHandle::type_id);
660+
) -> Result<Option<UntypedHandle>, AssetLoadError> {
661+
let input_handle_type_id = input_handle.as_ref().map(UntypedHandle::type_id);
656662

657663
let path = path.into_owned();
658664
let path_clone = path.clone();
659665
let (mut meta, loader, mut reader) = self
660-
.get_meta_loader_and_reader(&path_clone, asset_type_id)
666+
.get_meta_loader_and_reader(&path_clone, input_handle_type_id)
661667
.await
662668
.inspect_err(|e| {
663669
// if there was an input handle, a "load" operation has already started, so we must produce a "failure" event, if
@@ -674,76 +680,90 @@ impl AssetServer {
674680
if let Some(meta_transform) = input_handle.as_ref().and_then(|h| h.meta_transform()) {
675681
(*meta_transform)(&mut *meta);
676682
}
677-
// downgrade the input handle so we don't keep the asset alive just because we're loading it
678-
// note we can't just pass a weak handle in, as only strong handles contain the asset meta transform
679-
input_handle = input_handle.map(|h| h.clone_weak());
680-
681-
// This contains Some(UntypedHandle), if it was retrievable
682-
// If it is None, that is because it was _not_ retrievable, due to
683-
// 1. The handle was not already passed in for this path, meaning we can't just use that
684-
// 2. The asset has not been loaded yet, meaning there is no existing Handle for it
685-
// 3. The path has a label, meaning the AssetLoader's root asset type is not the path's asset type
686-
//
687-
// In the None case, the only course of action is to wait for the asset to load so we can allocate the
688-
// handle for that type.
689-
//
690-
// TODO: Note that in the None case, multiple asset loads for the same path can happen at the same time
691-
// (rather than "early out-ing" in the "normal" case)
692-
// This would be resolved by a universal asset id, as we would not need to resolve the asset type
693-
// to generate the ID. See this issue: https://github.com/bevyengine/bevy/issues/10549
694-
let handle_result = match input_handle {
695-
Some(handle) => {
696-
// if a handle was passed in, the "should load" check was already done
697-
Some((handle, true))
698-
}
699-
None => {
700-
let mut infos = self.data.infos.write();
701-
let result = infos.get_or_create_path_handle_internal(
702-
path.clone(),
703-
path.label().is_none().then(|| loader.asset_type_id()),
704-
HandleLoadingMode::Request,
705-
meta_transform,
706-
);
707-
unwrap_with_context(result, Either::Left(loader.asset_type_name()))
708-
}
709-
};
710683

711-
let handle = if let Some((handle, should_load)) = handle_result {
712-
if path.label().is_none() && handle.type_id() != loader.asset_type_id() {
684+
let asset_id; // The asset ID of the asset we are trying to load.
685+
let fetched_handle; // The handle if one was looked up/created.
686+
let should_load; // Whether we need to load the asset.
687+
if let Some(input_handle) = input_handle {
688+
asset_id = Some(input_handle.id());
689+
// In this case, we intentionally drop the input handle so we can cancel loading the
690+
// asset if the handle gets dropped (externally) before it finishes loading.
691+
fetched_handle = None;
692+
// The handle was passed in, so the "should_load" check was already done.
693+
should_load = true;
694+
} else {
695+
// TODO: multiple asset loads for the same path can happen at the same time (rather than
696+
// "early out-ing" in the "normal" case). This would be resolved by a universal asset
697+
// id, as we would not need to resolve the asset type to generate the ID. See this
698+
// issue: https://github.com/bevyengine/bevy/issues/10549
699+
700+
let mut infos = self.data.infos.write();
701+
let result = infos.get_or_create_path_handle_internal(
702+
path.clone(),
703+
path.label().is_none().then(|| loader.asset_type_id()),
704+
HandleLoadingMode::Request,
705+
meta_transform,
706+
);
707+
match unwrap_with_context(result, Either::Left(loader.asset_type_name())) {
708+
// We couldn't figure out the correct handle without its type ID (which can only
709+
// happen if we are loading a subasset).
710+
None => {
711+
// We don't know the expected type since the subasset may have a different type
712+
// than the "root" asset (which is the type the loader will load).
713+
asset_id = None;
714+
fetched_handle = None;
715+
// If we couldn't find an appropriate handle, then the asset certainly needs to
716+
// be loaded.
717+
should_load = true;
718+
}
719+
Some((handle, result_should_load)) => {
720+
asset_id = Some(handle.id());
721+
fetched_handle = Some(handle);
722+
should_load = result_should_load;
723+
}
724+
}
725+
}
726+
// Verify that the expected type matches the loader's type.
727+
if let Some(asset_type_id) = asset_id.map(|id| id.type_id()) {
728+
// If we are loading a subasset, then the subasset's type almost certainly doesn't match
729+
// the loader's type - and that's ok.
730+
if path.label().is_none() && asset_type_id != loader.asset_type_id() {
713731
error!(
714732
"Expected {:?}, got {:?}",
715-
handle.type_id(),
733+
asset_type_id,
716734
loader.asset_type_id()
717735
);
718736
return Err(AssetLoadError::RequestedHandleTypeMismatch {
719737
path: path.into_owned(),
720-
requested: handle.type_id(),
738+
requested: asset_type_id,
721739
actual_asset_name: loader.asset_type_name(),
722740
loader_name: loader.type_name(),
723741
});
724742
}
725-
if !should_load && !force {
726-
return Ok(handle);
727-
}
728-
Some(handle)
729-
} else {
730-
None
731-
};
732-
// if the handle result is None, we definitely need to load the asset
743+
}
744+
// Bail out earlier if we don't need to load the asset.
745+
if !should_load && !force {
746+
return Ok(fetched_handle);
747+
}
733748

734-
let (base_handle, base_path) = if path.label().is_some() {
749+
// We don't actually need to use _base_handle, but we do need to keep the handle alive.
750+
// Dropping it would cancel the load of the base asset, which would make the load of this
751+
// subasset never complete.
752+
let (base_asset_id, _base_handle, base_path) = if path.label().is_some() {
735753
let mut infos = self.data.infos.write();
736754
let base_path = path.without_label().into_owned();
737-
let (base_handle, _) = infos.get_or_create_path_handle_erased(
738-
base_path.clone(),
739-
loader.asset_type_id(),
740-
Some(loader.asset_type_name()),
741-
HandleLoadingMode::Force,
742-
None,
743-
);
744-
(base_handle, base_path)
755+
let base_handle = infos
756+
.get_or_create_path_handle_erased(
757+
base_path.clone(),
758+
loader.asset_type_id(),
759+
Some(loader.asset_type_name()),
760+
HandleLoadingMode::Force,
761+
None,
762+
)
763+
.0;
764+
(base_handle.id(), Some(base_handle), base_path)
745765
} else {
746-
(handle.clone().unwrap(), path.clone())
766+
(asset_id.unwrap(), None, path.clone())
747767
};
748768

749769
match self
@@ -760,7 +780,7 @@ impl AssetServer {
760780
Ok(loaded_asset) => {
761781
let final_handle = if let Some(label) = path.label_cow() {
762782
match loaded_asset.labeled_assets.get(&label) {
763-
Some(labeled_asset) => labeled_asset.handle.clone(),
783+
Some(labeled_asset) => Some(labeled_asset.handle.clone()),
764784
None => {
765785
let mut all_labels: Vec<String> = loaded_asset
766786
.labeled_assets
@@ -776,16 +796,15 @@ impl AssetServer {
776796
}
777797
}
778798
} else {
779-
// if the path does not have a label, the handle must exist at this point
780-
handle.unwrap()
799+
fetched_handle
781800
};
782801

783-
self.send_loaded_asset(base_handle.id(), loaded_asset);
802+
self.send_loaded_asset(base_asset_id, loaded_asset);
784803
Ok(final_handle)
785804
}
786805
Err(err) => {
787806
self.send_asset_event(InternalAssetEvent::Failed {
788-
id: base_handle.id(),
807+
id: base_asset_id,
789808
error: err.clone(),
790809
path: path.into_owned(),
791810
});

0 commit comments

Comments
 (0)