Skip to content

Commit cc6c4d6

Browse files
cartalice-i-cecile
andauthored
Fix GLTF scene dependencies and make full scene renders predictable (#10745)
# Objective Fixes #10688 There were a number of issues at play: 1. The GLTF loader was not registering Scene dependencies properly. They were being registered at the root instead of on the scene assets. This made `LoadedWithDependencies` fire immediately on load. 2. Recursive labeled assets _inside_ of labeled assets were not being loaded. This only became relevant for scenes after fixing (1) because we now add labeled assets to the nested scene `LoadContext` instead of the root load context. I'm surprised nobody has hit this yet. I'm glad I caught it before somebody hit it. 3. Accessing "loaded with dependencies" state on the Asset Server is boilerplatey + error prone (because you need to manually query two states). ## Solution 1. In GltfLoader, use a nested LoadContext for scenes and load dependencies through that context. 2. In the `AssetServer`, load labeled assets recursively. 3. Added a simple `asset_server.is_loaded_with_dependencies(id)` I also added some docs to `LoadContext` to help prevent this problem in the future. --- ## Changelog - Added `AssetServer::is_loaded_with_dependencies` - Fixed GLTF Scene dependencies - Fixed nested labeled assets not being loaded --------- Co-authored-by: Alice Cecile <[email protected]>
1 parent 6e871ab commit cc6c4d6

File tree

4 files changed

+49
-16
lines changed

4 files changed

+49
-16
lines changed

crates/bevy_asset/src/loader.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -353,6 +353,13 @@ impl<'a> LoadContext<'a> {
353353

354354
/// This will add the given `asset` as a "labeled [`Asset`]" with the `label` label.
355355
///
356+
/// # Warning
357+
///
358+
/// This will not assign dependencies to the given `asset`. If adding an asset
359+
/// with dependencies generated from calls such as [`LoadContext::load`], use
360+
/// [`LoadContext::labeled_asset_scope`] or [`LoadContext::begin_labeled_asset`] to generate a
361+
/// new [`LoadContext`] to track the dependencies for the labeled asset.
362+
///
356363
/// See [`AssetPath`] for more on labeled assets.
357364
pub fn add_labeled_asset<A: Asset>(&mut self, label: String, asset: A) -> Handle<A> {
358365
self.labeled_asset_scope(label, |_| asset)

crates/bevy_asset/src/server/info.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -260,6 +260,10 @@ impl AssetInfos {
260260
self.infos.get(&id)
261261
}
262262

263+
pub(crate) fn contains_key(&self, id: UntypedAssetId) -> bool {
264+
self.infos.contains_key(&id)
265+
}
266+
263267
pub(crate) fn get_mut(&mut self, id: UntypedAssetId) -> Option<&mut AssetInfo> {
264268
self.infos.get_mut(&id)
265269
}

crates/bevy_asset/src/server/mod.rs

Lines changed: 28 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -482,7 +482,7 @@ impl AssetServer {
482482
.load_with_meta_loader_and_reader(&base_path, meta, &*loader, &mut *reader, true, false)
483483
.await
484484
{
485-
Ok(mut loaded_asset) => {
485+
Ok(loaded_asset) => {
486486
let final_handle = if let Some(label) = path.label_cow() {
487487
match loaded_asset.labeled_assets.get(&label) {
488488
Some(labeled_asset) => labeled_asset.handle.clone(),
@@ -498,17 +498,7 @@ impl AssetServer {
498498
handle.unwrap()
499499
};
500500

501-
for (_, labeled_asset) in loaded_asset.labeled_assets.drain() {
502-
self.send_asset_event(InternalAssetEvent::Loaded {
503-
id: labeled_asset.handle.id(),
504-
loaded_asset: labeled_asset.asset,
505-
});
506-
}
507-
508-
self.send_asset_event(InternalAssetEvent::Loaded {
509-
id: base_handle.id(),
510-
loaded_asset,
511-
});
501+
self.send_loaded_asset(base_handle.id(), loaded_asset);
512502
Ok(final_handle)
513503
}
514504
Err(err) => {
@@ -520,6 +510,16 @@ impl AssetServer {
520510
}
521511
}
522512

513+
/// Sends a load event for the given `loaded_asset` and does the same recursively for all
514+
/// labeled assets.
515+
fn send_loaded_asset(&self, id: UntypedAssetId, mut loaded_asset: ErasedLoadedAsset) {
516+
for (_, labeled_asset) in loaded_asset.labeled_assets.drain() {
517+
self.send_loaded_asset(labeled_asset.handle.id(), labeled_asset.asset);
518+
}
519+
520+
self.send_asset_event(InternalAssetEvent::Loaded { id, loaded_asset });
521+
}
522+
523523
/// Kicks off a reload of the asset stored at the given path. This will only reload the asset if it currently loaded.
524524
pub fn reload<'a>(&self, path: impl Into<AssetPath<'a>>) {
525525
let server = self.clone();
@@ -707,6 +707,9 @@ impl AssetServer {
707707
}
708708

709709
/// Retrieves the main [`LoadState`] of a given asset `id`.
710+
///
711+
/// Note that this is "just" the root asset load state. To check if an asset _and_ its recursive
712+
/// dependencies have loaded, see [`AssetServer::is_loaded_with_dependencies`].
710713
pub fn get_load_state(&self, id: impl Into<UntypedAssetId>) -> Option<LoadState> {
711714
self.data.infos.read().get(id.into()).map(|i| i.load_state)
712715
}
@@ -737,6 +740,13 @@ impl AssetServer {
737740
.unwrap_or(RecursiveDependencyLoadState::NotLoaded)
738741
}
739742

743+
/// Returns true if the asset and all of its dependencies (recursive) have been loaded.
744+
pub fn is_loaded_with_dependencies(&self, id: impl Into<UntypedAssetId>) -> bool {
745+
let id = id.into();
746+
self.load_state(id) == LoadState::Loaded
747+
&& self.recursive_dependency_load_state(id) == RecursiveDependencyLoadState::Loaded
748+
}
749+
740750
/// Returns an active handle for the given path, if the asset at the given path has already started loading,
741751
/// or is still "alive".
742752
pub fn get_handle<'a, A: Asset>(&self, path: impl Into<AssetPath<'a>>) -> Option<Handle<A>> {
@@ -752,6 +762,12 @@ impl AssetServer {
752762
self.data.infos.read().get_id_handle(id)
753763
}
754764

765+
/// Returns `true` if the given `id` corresponds to an asset that is managed by this [`AssetServer`].
766+
/// Otherwise, returns false.
767+
pub fn is_managed(&self, id: impl Into<UntypedAssetId>) -> bool {
768+
self.data.infos.read().contains_key(id.into())
769+
}
770+
755771
/// Returns an active untyped handle for the given path, if the asset at the given path has already started loading,
756772
/// or is still "alive".
757773
pub fn get_handle_untyped<'a>(&self, path: impl Into<AssetPath<'a>>) -> Option<UntypedHandle> {

crates/bevy_gltf/src/loader.rs

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -545,7 +545,7 @@ async fn load_gltf<'a, 'b, 'c>(
545545
let mut world = World::default();
546546
let mut node_index_to_entity_map = HashMap::new();
547547
let mut entity_to_skin_index_map = HashMap::new();
548-
548+
let mut scene_load_context = load_context.begin_labeled_asset();
549549
world
550550
.spawn(SpatialBundle::INHERITED_IDENTITY)
551551
.with_children(|parent| {
@@ -554,6 +554,7 @@ async fn load_gltf<'a, 'b, 'c>(
554554
&node,
555555
parent,
556556
load_context,
557+
&mut scene_load_context,
557558
&mut node_index_to_entity_map,
558559
&mut entity_to_skin_index_map,
559560
&mut active_camera_found,
@@ -606,8 +607,8 @@ async fn load_gltf<'a, 'b, 'c>(
606607
joints: joint_entities,
607608
});
608609
}
609-
610-
let scene_handle = load_context.add_labeled_asset(scene_label(&scene), Scene::new(world));
610+
let loaded_scene = scene_load_context.finish(Scene::new(world), None);
611+
let scene_handle = load_context.add_loaded_labeled_asset(scene_label(&scene), loaded_scene);
611612

612613
if let Some(name) = scene.name() {
613614
named_scenes.insert(name.to_string(), scene_handle.clone());
@@ -853,9 +854,11 @@ fn load_material(
853854
}
854855

855856
/// Loads a glTF node.
857+
#[allow(clippy::too_many_arguments)]
856858
fn load_node(
857859
gltf_node: &gltf::Node,
858860
world_builder: &mut WorldChildBuilder,
861+
root_load_context: &LoadContext,
859862
load_context: &mut LoadContext,
860863
node_index_to_entity_map: &mut HashMap<usize, Entity>,
861864
entity_to_skin_index_map: &mut HashMap<Entity, usize>,
@@ -942,7 +945,9 @@ fn load_node(
942945
// added when iterating over all the gltf materials (since the default material is
943946
// not explicitly listed in the gltf).
944947
// It also ensures an inverted scale copy is instantiated if required.
945-
if !load_context.has_labeled_asset(&material_label) {
948+
if !root_load_context.has_labeled_asset(&material_label)
949+
&& !load_context.has_labeled_asset(&material_label)
950+
{
946951
load_material(&material, load_context, is_scale_inverted);
947952
}
948953

@@ -1074,6 +1079,7 @@ fn load_node(
10741079
if let Err(err) = load_node(
10751080
&child,
10761081
parent,
1082+
root_load_context,
10771083
load_context,
10781084
node_index_to_entity_map,
10791085
entity_to_skin_index_map,

0 commit comments

Comments
 (0)