From 6d295036f2770a0c211cd18557afbfa1d769a8a5 Mon Sep 17 00:00:00 2001 From: Carter Anderson Date: Mon, 27 Nov 2023 14:42:28 -0800 Subject: [PATCH] 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 --- crates/bevy_asset/src/loader.rs | 7 +++++ crates/bevy_asset/src/server/info.rs | 4 +++ crates/bevy_asset/src/server/mod.rs | 40 +++++++++++++++++++--------- crates/bevy_gltf/src/loader.rs | 14 +++++++--- 4 files changed, 49 insertions(+), 16 deletions(-) diff --git a/crates/bevy_asset/src/loader.rs b/crates/bevy_asset/src/loader.rs index 4fc451991bbfb..2bd684106800b 100644 --- a/crates/bevy_asset/src/loader.rs +++ b/crates/bevy_asset/src/loader.rs @@ -353,6 +353,13 @@ impl<'a> LoadContext<'a> { /// This will add the given `asset` as a "labeled [`Asset`]" with the `label` label. /// + /// # Warning + /// + /// This will not assign dependencies to the given `asset`. If adding an asset + /// with dependencies generated from calls such as [`LoadContext::load`], use + /// [`LoadContext::labeled_asset_scope`] or [`LoadContext::begin_labeled_asset`] to generate a + /// new [`LoadContext`] to track the dependencies for the labeled asset. + /// /// See [`AssetPath`] for more on labeled assets. pub fn add_labeled_asset(&mut self, label: String, asset: A) -> Handle { self.labeled_asset_scope(label, |_| asset) diff --git a/crates/bevy_asset/src/server/info.rs b/crates/bevy_asset/src/server/info.rs index 2786a7d4b86b3..1297daec49ba8 100644 --- a/crates/bevy_asset/src/server/info.rs +++ b/crates/bevy_asset/src/server/info.rs @@ -260,6 +260,10 @@ impl AssetInfos { self.infos.get(&id) } + pub(crate) fn contains_key(&self, id: UntypedAssetId) -> bool { + self.infos.contains_key(&id) + } + pub(crate) fn get_mut(&mut self, id: UntypedAssetId) -> Option<&mut AssetInfo> { self.infos.get_mut(&id) } diff --git a/crates/bevy_asset/src/server/mod.rs b/crates/bevy_asset/src/server/mod.rs index b8bac7f7b2b0f..51d5cc2dc2604 100644 --- a/crates/bevy_asset/src/server/mod.rs +++ b/crates/bevy_asset/src/server/mod.rs @@ -482,7 +482,7 @@ impl AssetServer { .load_with_meta_loader_and_reader(&base_path, meta, &*loader, &mut *reader, true, false) .await { - Ok(mut loaded_asset) => { + Ok(loaded_asset) => { let final_handle = if let Some(label) = path.label_cow() { match loaded_asset.labeled_assets.get(&label) { Some(labeled_asset) => labeled_asset.handle.clone(), @@ -498,17 +498,7 @@ impl AssetServer { 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, - }); + self.send_loaded_asset(base_handle.id(), loaded_asset); Ok(final_handle) } Err(err) => { @@ -520,6 +510,16 @@ impl AssetServer { } } + /// Sends a load event for the given `loaded_asset` and does the same recursively for all + /// labeled assets. + fn send_loaded_asset(&self, id: UntypedAssetId, mut loaded_asset: ErasedLoadedAsset) { + for (_, labeled_asset) in loaded_asset.labeled_assets.drain() { + self.send_loaded_asset(labeled_asset.handle.id(), labeled_asset.asset); + } + + self.send_asset_event(InternalAssetEvent::Loaded { id, loaded_asset }); + } + /// Kicks off a reload of the asset stored at the given path. This will only reload the asset if it currently loaded. pub fn reload<'a>(&self, path: impl Into>) { let server = self.clone(); @@ -707,6 +707,9 @@ impl AssetServer { } /// Retrieves the main [`LoadState`] of a given asset `id`. + /// + /// Note that this is "just" the root asset load state. To check if an asset _and_ its recursive + /// dependencies have loaded, see [`AssetServer::is_loaded_with_dependencies`]. pub fn get_load_state(&self, id: impl Into) -> Option { self.data.infos.read().get(id.into()).map(|i| i.load_state) } @@ -737,6 +740,13 @@ impl AssetServer { .unwrap_or(RecursiveDependencyLoadState::NotLoaded) } + /// Returns true if the asset and all of its dependencies (recursive) have been loaded. + pub fn is_loaded_with_dependencies(&self, id: impl Into) -> bool { + let id = id.into(); + self.load_state(id) == LoadState::Loaded + && self.recursive_dependency_load_state(id) == RecursiveDependencyLoadState::Loaded + } + /// Returns an active handle for the given path, if the asset at the given path has already started loading, /// or is still "alive". pub fn get_handle<'a, A: Asset>(&self, path: impl Into>) -> Option> { @@ -752,6 +762,12 @@ impl AssetServer { self.data.infos.read().get_id_handle(id) } + /// Returns `true` if the given `id` corresponds to an asset that is managed by this [`AssetServer`]. + /// Otherwise, returns false. + pub fn is_managed(&self, id: impl Into) -> bool { + self.data.infos.read().contains_key(id.into()) + } + /// Returns an active untyped handle for the given path, if the asset at the given path has already started loading, /// or is still "alive". pub fn get_handle_untyped<'a>(&self, path: impl Into>) -> Option { diff --git a/crates/bevy_gltf/src/loader.rs b/crates/bevy_gltf/src/loader.rs index abdbc1a3cbe50..44b11b1e7c51c 100644 --- a/crates/bevy_gltf/src/loader.rs +++ b/crates/bevy_gltf/src/loader.rs @@ -545,7 +545,7 @@ async fn load_gltf<'a, 'b, 'c>( let mut world = World::default(); let mut node_index_to_entity_map = HashMap::new(); let mut entity_to_skin_index_map = HashMap::new(); - + let mut scene_load_context = load_context.begin_labeled_asset(); world .spawn(SpatialBundle::INHERITED_IDENTITY) .with_children(|parent| { @@ -554,6 +554,7 @@ async fn load_gltf<'a, 'b, 'c>( &node, parent, load_context, + &mut scene_load_context, &mut node_index_to_entity_map, &mut entity_to_skin_index_map, &mut active_camera_found, @@ -606,8 +607,8 @@ async fn load_gltf<'a, 'b, 'c>( joints: joint_entities, }); } - - let scene_handle = load_context.add_labeled_asset(scene_label(&scene), Scene::new(world)); + let loaded_scene = scene_load_context.finish(Scene::new(world), None); + let scene_handle = load_context.add_loaded_labeled_asset(scene_label(&scene), loaded_scene); if let Some(name) = scene.name() { named_scenes.insert(name.to_string(), scene_handle.clone()); @@ -853,9 +854,11 @@ fn load_material( } /// Loads a glTF node. +#[allow(clippy::too_many_arguments)] fn load_node( gltf_node: &gltf::Node, world_builder: &mut WorldChildBuilder, + root_load_context: &LoadContext, load_context: &mut LoadContext, node_index_to_entity_map: &mut HashMap, entity_to_skin_index_map: &mut HashMap, @@ -942,7 +945,9 @@ fn load_node( // added when iterating over all the gltf materials (since the default material is // not explicitly listed in the gltf). // It also ensures an inverted scale copy is instantiated if required. - if !load_context.has_labeled_asset(&material_label) { + if !root_load_context.has_labeled_asset(&material_label) + && !load_context.has_labeled_asset(&material_label) + { load_material(&material, load_context, is_scale_inverted); } @@ -1074,6 +1079,7 @@ fn load_node( if let Err(err) = load_node( &child, parent, + root_load_context, load_context, node_index_to_entity_map, entity_to_skin_index_map,