From cb4fe4ea9e13636b4194362808f2af0b99482fd9 Mon Sep 17 00:00:00 2001 From: Mikhail Novikov Date: Mon, 1 Jul 2024 16:05:16 +0200 Subject: [PATCH] Make gLTF node children Handle instead of objects (#13707) Part of #13681 # Objective gLTF Assets shouldn't be duplicated between Assets resource and node children. Also changed `asset_label` to be a method as [per previous PR comment](https://github.com/bevyengine/bevy/pull/13558). ## Solution - Made GltfNode children be Handles instead of asset copies. ## Testing - Added tests that actually test loading and hierarchy as previous ones unit tested only one function and that makes little sense. - Made circular nodes an actual loading failure instead of a warning no-op. You [_MUST NOT_ have cycles in gLTF](https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#nodes-and-hierarchy) according to the spec. - IMO this is a bugfix, not a breaking change. But in an extremely unlikely event in which you relied on invalid behavior for loading gLTF with cyclic children, you will not be able to do that anymore. You should fix your gLTF file as it's not valid according to gLTF spec. For it to for work someone, it had to be bevy with bevy_animation flag off. --- ## Changelog ### Changed - `GltfNode.children` are now `Vec>` instead of `Vec` - Having children cycles between gLTF nodes in a gLTF document is now an explicit asset loading failure. ## Migration Guide If accessing children, use `Assets` resource to get the actual child object. #### Before ```rs fn gltf_print_first_node_children_system(gltf_component_query: Query>, gltf_assets: Res>, gltf_nodes: Res>) { for gltf_handle in gltf_component_query.iter() { let gltf_root = gltf_assets.get(gltf_handle).unwrap(); let first_node_handle = gltf_root.nodes.get(0).unwrap(); let first_node = gltf_nodes.get(first_node_handle).unwrap(); let first_child = first_node.children.get(0).unwrap(); println!("First nodes child node name is {:?)", first_child.name); } } ``` #### After ```rs fn gltf_print_first_node_children_system(gltf_component_query: Query>, gltf_assets: Res>, gltf_nodes: Res>) { for gltf_handle in gltf_component_query.iter() { let gltf_root = gltf_assets.get(gltf_handle).unwrap(); let first_node_handle = gltf_root.nodes.get(0).unwrap(); let first_node = gltf_nodes.get(first_node_handle).unwrap(); let first_child_handle = first_node.children.get(0).unwrap(); let first_child = gltf_nodes.get(first_child_handle).unwrap(); println!("First nodes child node name is {:?)", first_child.name); } } ``` --- crates/bevy_gltf/Cargo.toml | 3 + crates/bevy_gltf/src/lib.rs | 39 +-- crates/bevy_gltf/src/loader.rs | 419 +++++++++++++++++++++++++-------- 3 files changed, 354 insertions(+), 107 deletions(-) diff --git a/crates/bevy_gltf/Cargo.toml b/crates/bevy_gltf/Cargo.toml index b74079b386..831543a801 100644 --- a/crates/bevy_gltf/Cargo.toml +++ b/crates/bevy_gltf/Cargo.toml @@ -57,6 +57,9 @@ serde = { version = "1.0", features = ["derive"] } serde_json = "1" smallvec = "1.11" +[dev-dependencies] +bevy_log = { path = "../bevy_log", version = "0.14.0-dev" } + [lints] workspace = true diff --git a/crates/bevy_gltf/src/lib.rs b/crates/bevy_gltf/src/lib.rs index faf25e58a7..1fa1966692 100644 --- a/crates/bevy_gltf/src/lib.rs +++ b/crates/bevy_gltf/src/lib.rs @@ -209,10 +209,8 @@ pub struct GltfNode { pub index: usize, /// Computed name for a node - either a user defined node name from gLTF or a generated name from index pub name: String, - /// Subasset label for this node within the gLTF parent asset. - pub asset_label: GltfAssetLabel, /// Direct children of the node. - pub children: Vec, + pub children: Vec>, /// Mesh of the node. pub mesh: Option>, /// Local transform. @@ -225,14 +223,13 @@ impl GltfNode { /// Create a node extracting name and index from glTF def pub fn new( node: &gltf::Node, - children: Vec, + children: Vec>, mesh: Option>, transform: bevy_transform::prelude::Transform, extras: Option, ) -> Self { Self { index: node.index(), - asset_label: GltfAssetLabel::Node(node.index()), name: if let Some(name) = node.name() { name.to_string() } else { @@ -244,6 +241,11 @@ impl GltfNode { extras, } } + + /// Subasset label for this node within the gLTF parent asset. + pub fn asset_label(&self) -> GltfAssetLabel { + GltfAssetLabel::Node(self.index) + } } /// A glTF mesh, which may consist of multiple [`GltfPrimitives`](GltfPrimitive) @@ -256,8 +258,6 @@ pub struct GltfMesh { pub index: usize, /// Computed name for a mesh - either a user defined mesh name from gLTF or a generated name from index pub name: String, - /// Subasset label for this mesh within the gLTF parent asset. - pub asset_label: GltfAssetLabel, /// Primitives of the glTF mesh. pub primitives: Vec, /// Additional data. @@ -273,7 +273,6 @@ impl GltfMesh { ) -> Self { Self { index: mesh.index(), - asset_label: GltfAssetLabel::Mesh(mesh.index()), name: if let Some(name) = mesh.name() { name.to_string() } else { @@ -283,6 +282,11 @@ impl GltfMesh { extras, } } + + /// Subasset label for this mesh within the gLTF parent asset. + pub fn asset_label(&self) -> GltfAssetLabel { + GltfAssetLabel::Mesh(self.index) + } } /// Part of a [`GltfMesh`] that consists of a [`Mesh`], an optional [`StandardMaterial`] and [`GltfExtras`]. @@ -292,10 +296,10 @@ impl GltfMesh { pub struct GltfPrimitive { /// Index of the primitive inside the mesh pub index: usize, + /// Index of the parent [`GltfMesh`] of this primitive + pub parent_mesh_index: usize, /// Computed name for a primitive - either a user defined primitive name from gLTF or a generated name from index pub name: String, - /// Subasset label for this mesh within the gLTF parent asset. - pub asset_label: GltfAssetLabel, /// Topology to be rendered. pub mesh: Handle, /// Material to apply to the `mesh`. @@ -318,6 +322,7 @@ impl GltfPrimitive { ) -> Self { GltfPrimitive { index: gltf_primitive.index(), + parent_mesh_index: gltf_mesh.index(), name: { let mesh_name = gltf_mesh.name().unwrap_or("Mesh"); if gltf_mesh.primitives().len() > 1 { @@ -326,16 +331,20 @@ impl GltfPrimitive { mesh_name.to_string() } }, - asset_label: GltfAssetLabel::Primitive { - mesh: gltf_mesh.index(), - primitive: gltf_primitive.index(), - }, mesh, material, extras, material_extras, } } + + /// Subasset label for this primitive within its parent [`GltfMesh`] within the gLTF parent asset. + pub fn asset_label(&self) -> GltfAssetLabel { + GltfAssetLabel::Primitive { + mesh: self.parent_mesh_index, + primitive: self.index, + } + } } /// Additional untyped data that can be present on most glTF types at the primitive level. @@ -405,7 +414,7 @@ pub struct GltfMaterialExtras { /// let gltf_scene: Handle = asset_server.load(format!("models/FlightHelmet/FlightHelmet.gltf#{}", GltfAssetLabel::Scene(0))); /// } /// ``` -#[derive(Debug, Clone, Copy)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum GltfAssetLabel { /// `Scene{}`: glTF Scene as a Bevy `Scene` Scene(usize), diff --git a/crates/bevy_gltf/src/loader.rs b/crates/bevy_gltf/src/loader.rs index c4426026c9..8ca6b63e26 100644 --- a/crates/bevy_gltf/src/loader.rs +++ b/crates/bevy_gltf/src/loader.rs @@ -103,6 +103,9 @@ pub enum GltfError { /// Failed to generate morph targets. #[error("failed to generate morph targets: {0}")] MorphTarget(#[from] bevy_render::mesh::morph::MorphBuildError), + /// Circular children in Nodes + #[error("GLTF model must be a tree, found cycle instead at node indices: {0:?}")] + CircularChildren(String), /// Failed to load a file. #[error("failed to load file: {0}")] Io(#[from] std::io::Error), @@ -252,7 +255,7 @@ async fn load_gltf<'a, 'b, 'c>( for scene in gltf.scenes() { for node in scene.nodes() { let root_index = node.index(); - paths_recur(node, &[], &mut paths, root_index); + paths_recur(node, &[], &mut paths, root_index, &mut HashSet::new()); } } paths @@ -576,7 +579,7 @@ async fn load_gltf<'a, 'b, 'c>( let mesh = super::GltfMesh::new(&gltf_mesh, primitives, get_gltf_extras(gltf_mesh.extras())); - let handle = load_context.add_labeled_asset(mesh.asset_label.to_string(), mesh); + let handle = load_context.add_labeled_asset(mesh.asset_label().to_string(), mesh); if let Some(name) = gltf_mesh.name() { named_meshes.insert(name.into(), handle.clone()); } @@ -597,16 +600,22 @@ async fn load_gltf<'a, 'b, 'c>( get_gltf_extras(node.extras()), ), node.children() - .map(|child| child.index()) + .map(|child| { + ( + child.index(), + load_context + .get_label_handle(format!("{}", GltfAssetLabel::Node(node.index()))), + ) + }) .collect::>(), )); if let Some(name) = node.name() { named_nodes_intermediate.insert(name, node.index()); } } - let nodes = resolve_node_hierarchy(nodes_intermediate, load_context.path()) + let nodes = resolve_node_hierarchy(nodes_intermediate)? .into_iter() - .map(|node| load_context.add_labeled_asset(node.asset_label.to_string(), node)) + .map(|node| load_context.add_labeled_asset(node.asset_label().to_string(), node)) .collect::>>(); let named_nodes = named_nodes_intermediate .into_iter() @@ -792,11 +801,15 @@ fn paths_recur( current_path: &[Name], paths: &mut HashMap)>, root_index: usize, + visited: &mut HashSet, ) { let mut path = current_path.to_owned(); path.push(node_name(&node)); + visited.insert(node.index()); for child in node.children() { - paths_recur(child, &path, paths, root_index); + if !visited.contains(&child.index()) { + paths_recur(child, &path, paths, root_index, visited); + } } paths.insert(node.index(), (root_index, path)); } @@ -1652,26 +1665,21 @@ async fn load_buffers( Ok(buffer_data) } +#[allow(clippy::result_large_err)] fn resolve_node_hierarchy( - nodes_intermediate: Vec<(GltfNode, Vec)>, - asset_path: &Path, -) -> Vec { - let mut has_errored = false; + nodes_intermediate: Vec<(GltfNode, Vec<(usize, Handle)>)>, +) -> Result, GltfError> { let mut empty_children = VecDeque::new(); let mut parents = vec![None; nodes_intermediate.len()]; let mut unprocessed_nodes = nodes_intermediate .into_iter() .enumerate() .map(|(i, (node, children))| { - for child in &children { - if let Some(parent) = parents.get_mut(*child) { - *parent = Some(i); - } else if !has_errored { - has_errored = true; - warn!("Unexpected child in GLTF Mesh {}", child); - } + for (child_index, _child_handle) in &children { + let parent = parents.get_mut(*child_index).unwrap(); + *parent = Some(i); } - let children = children.into_iter().collect::>(); + let children = children.into_iter().collect::>(); if children.is_empty() { empty_children.push_back(i); } @@ -1686,24 +1694,28 @@ fn resolve_node_hierarchy( if let Some(parent_index) = parents[index] { let (parent_node, parent_children) = unprocessed_nodes.get_mut(&parent_index).unwrap(); - assert!(parent_children.remove(&index)); - if let Some(child_node) = nodes.get(&index) { - parent_node.children.push(child_node.clone()); - } + let handle = parent_children.remove(&index).unwrap(); + parent_node.children.push(handle); if parent_children.is_empty() { empty_children.push_back(parent_index); } } } if !unprocessed_nodes.is_empty() { - warn!("GLTF model must be a tree: {:?}", asset_path); + return Err(GltfError::CircularChildren(format!( + "{:?}", + unprocessed_nodes + .iter() + .map(|(k, _v)| *k) + .collect::>(), + ))); } let mut nodes_to_sort = nodes.into_iter().collect::>(); nodes_to_sort.sort_by_key(|(i, _)| *i); - nodes_to_sort + Ok(nodes_to_sort .into_iter() .map(|(_, resolved)| resolved) - .collect() + .collect()) } enum ImageOrPath { @@ -1981,46 +1993,149 @@ fn material_needs_tangents(material: &Material) -> bool { #[cfg(test)] mod test { - use std::path::PathBuf; + use std::path::Path; - use super::resolve_node_hierarchy; - use crate::GltfNode; + use crate::{Gltf, GltfAssetLabel, GltfNode}; + use bevy_app::App; + use bevy_asset::{ + io::{ + memory::{Dir, MemoryAssetReader}, + AssetSource, AssetSourceId, + }, + AssetApp, AssetPlugin, AssetServer, Assets, Handle, LoadState, + }; + use bevy_core::TaskPoolPlugin; + use bevy_ecs::world::World; + use bevy_log::LogPlugin; + use bevy_scene::ScenePlugin; - impl GltfNode { - fn with_generated_name(index: usize) -> Self { - GltfNode { - index, - asset_label: crate::GltfAssetLabel::Node(index), - name: format!("l{}", index), - children: vec![], - mesh: None, - transform: bevy_transform::prelude::Transform::IDENTITY, - extras: None, + fn test_app(dir: Dir) -> App { + let mut app = App::new(); + let reader = MemoryAssetReader { root: dir }; + app.register_asset_source( + AssetSourceId::Default, + AssetSource::build().with_reader(move || Box::new(reader.clone())), + ) + .add_plugins(( + LogPlugin::default(), + TaskPoolPlugin::default(), + AssetPlugin::default(), + ScenePlugin, + crate::GltfPlugin::default(), + )); + + app.finish(); + app.cleanup(); + + app + } + + const LARGE_ITERATION_COUNT: usize = 10000; + + fn run_app_until(app: &mut App, mut predicate: impl FnMut(&mut World) -> Option<()>) { + for _ in 0..LARGE_ITERATION_COUNT { + app.update(); + if predicate(app.world_mut()).is_some() { + return; } } - } - #[test] - fn node_hierarchy_single_node() { - let result = resolve_node_hierarchy( - vec![(GltfNode::with_generated_name(1), vec![])], - PathBuf::new().as_path(), - ); - assert_eq!(result.len(), 1); - assert_eq!(result[0].name, "l1"); - assert_eq!(result[0].children.len(), 0); + panic!("Ran out of loops to return `Some` from `predicate`"); + } + + fn load_gltf_into_app(gltf_path: &str, gltf: &str) -> App { + let dir = Dir::default(); + dir.insert_asset_text(Path::new(gltf_path), gltf); + let mut app = test_app(dir); + app.update(); + let asset_server = app.world().resource::().clone(); + let handle: Handle = asset_server.load(gltf_path.to_string()); + let handle_id = handle.id(); + app.world_mut().spawn(handle.clone()); + app.update(); + run_app_until(&mut app, |_world| { + let load_state = asset_server.get_load_state(handle_id).unwrap(); + if load_state == LoadState::Loaded { + Some(()) + } else { + None + } + }); + app + } + + #[test] + fn single_node() { + let gltf_path = "test.gltf"; + let app = load_gltf_into_app( + gltf_path, + r#" +{ + "asset": { + "version": "2.0" + }, + "nodes": [ + { + "name": "TestSingleNode" + } + ], + "scene": 0, + "scenes": [{ "nodes": [0] }] +} +"#, + ); + let asset_server = app.world().resource::(); + let handle = asset_server.load(gltf_path); + let gltf_root_assets = app.world().resource::>(); + let gltf_node_assets = app.world().resource::>(); + let gltf_root = gltf_root_assets.get(&handle).unwrap(); + assert!(gltf_root.nodes.len() == 1, "Single node"); + assert!( + gltf_root.named_nodes.contains_key("TestSingleNode"), + "Named node is in named nodes" + ); + let gltf_node = gltf_node_assets + .get(gltf_root.named_nodes.get("TestSingleNode").unwrap()) + .unwrap(); + assert_eq!(gltf_node.name, "TestSingleNode", "Correct name"); + assert_eq!(gltf_node.index, 0, "Correct index"); + assert_eq!(gltf_node.children.len(), 0, "No children"); + assert_eq!(gltf_node.asset_label(), GltfAssetLabel::Node(0)); } #[test] fn node_hierarchy_no_hierarchy() { - let result = resolve_node_hierarchy( - vec![ - (GltfNode::with_generated_name(1), vec![]), - (GltfNode::with_generated_name(2), vec![]), - ], - PathBuf::new().as_path(), + let gltf_path = "test.gltf"; + let app = load_gltf_into_app( + gltf_path, + r#" +{ + "asset": { + "version": "2.0" + }, + "nodes": [ + { + "name": "l1" + }, + { + "name": "l2" + } + ], + "scene": 0, + "scenes": [{ "nodes": [0] }] +} +"#, ); - + let asset_server = app.world().resource::(); + let handle = asset_server.load(gltf_path); + let gltf_root_assets = app.world().resource::>(); + let gltf_node_assets = app.world().resource::>(); + let gltf_root = gltf_root_assets.get(&handle).unwrap(); + let result = gltf_root + .nodes + .iter() + .map(|h| gltf_node_assets.get(h).unwrap()) + .collect::>(); assert_eq!(result.len(), 2); assert_eq!(result[0].name, "l1"); assert_eq!(result[0].children.len(), 0); @@ -2030,14 +2145,38 @@ mod test { #[test] fn node_hierarchy_simple_hierarchy() { - let result = resolve_node_hierarchy( - vec![ - (GltfNode::with_generated_name(1), vec![1]), - (GltfNode::with_generated_name(2), vec![]), - ], - PathBuf::new().as_path(), + let gltf_path = "test.gltf"; + let app = load_gltf_into_app( + gltf_path, + r#" +{ + "asset": { + "version": "2.0" + }, + "nodes": [ + { + "name": "l1", + "children": [1] + }, + { + "name": "l2" + } + ], + "scene": 0, + "scenes": [{ "nodes": [0] }] +} +"#, ); - + let asset_server = app.world().resource::(); + let handle = asset_server.load(gltf_path); + let gltf_root_assets = app.world().resource::>(); + let gltf_node_assets = app.world().resource::>(); + let gltf_root = gltf_root_assets.get(&handle).unwrap(); + let result = gltf_root + .nodes + .iter() + .map(|h| gltf_node_assets.get(h).unwrap()) + .collect::>(); assert_eq!(result.len(), 2); assert_eq!(result[0].name, "l1"); assert_eq!(result[0].children.len(), 1); @@ -2047,19 +2186,56 @@ mod test { #[test] fn node_hierarchy_hierarchy() { - let result = resolve_node_hierarchy( - vec![ - (GltfNode::with_generated_name(1), vec![1]), - (GltfNode::with_generated_name(2), vec![2]), - (GltfNode::with_generated_name(3), vec![3, 4, 5]), - (GltfNode::with_generated_name(4), vec![6]), - (GltfNode::with_generated_name(5), vec![]), - (GltfNode::with_generated_name(6), vec![]), - (GltfNode::with_generated_name(7), vec![]), - ], - PathBuf::new().as_path(), + let gltf_path = "test.gltf"; + let app = load_gltf_into_app( + gltf_path, + r#" +{ + "asset": { + "version": "2.0" + }, + "nodes": [ + { + "name": "l1", + "children": [1] + }, + { + "name": "l2", + "children": [2] + }, + { + "name": "l3", + "children": [3, 4, 5] + }, + { + "name": "l4", + "children": [6] + }, + { + "name": "l5" + }, + { + "name": "l6" + }, + { + "name": "l7" + } + ], + "scene": 0, + "scenes": [{ "nodes": [0] }] +} +"#, ); - + let asset_server = app.world().resource::(); + let handle = asset_server.load(gltf_path); + let gltf_root_assets = app.world().resource::>(); + let gltf_node_assets = app.world().resource::>(); + let gltf_root = gltf_root_assets.get(&handle).unwrap(); + let result = gltf_root + .nodes + .iter() + .map(|h| gltf_node_assets.get(h).unwrap()) + .collect::>(); assert_eq!(result.len(), 7); assert_eq!(result[0].name, "l1"); assert_eq!(result[0].children.len(), 1); @@ -2079,29 +2255,88 @@ mod test { #[test] fn node_hierarchy_cyclic() { - let result = resolve_node_hierarchy( - vec![ - (GltfNode::with_generated_name(1), vec![1]), - (GltfNode::with_generated_name(2), vec![0]), - ], - PathBuf::new().as_path(), - ); + let gltf_path = "test.gltf"; + let gltf_str = r#" +{ + "asset": { + "version": "2.0" + }, + "nodes": [ + { + "name": "l1", + "children": [1] + }, + { + "name": "l2", + "children": [0] + } + ], + "scene": 0, + "scenes": [{ "nodes": [0] }] +} +"#; - assert_eq!(result.len(), 0); + let dir = Dir::default(); + dir.insert_asset_text(Path::new(gltf_path), gltf_str); + let mut app = test_app(dir); + app.update(); + let asset_server = app.world().resource::().clone(); + let handle: Handle = asset_server.load(gltf_path); + let handle_id = handle.id(); + app.world_mut().spawn(handle.clone()); + app.update(); + run_app_until(&mut app, |_world| { + let load_state = asset_server.get_load_state(handle_id).unwrap(); + if matches!(load_state, LoadState::Failed(_)) { + Some(()) + } else { + None + } + }); + let load_state = asset_server.get_load_state(handle_id).unwrap(); + assert!(matches!(load_state, LoadState::Failed(_))); } #[test] fn node_hierarchy_missing_node() { - let result = resolve_node_hierarchy( - vec![ - (GltfNode::with_generated_name(1), vec![2]), - (GltfNode::with_generated_name(2), vec![]), - ], - PathBuf::new().as_path(), - ); + let gltf_path = "test.gltf"; + let gltf_str = r#" +{ + "asset": { + "version": "2.0" + }, + "nodes": [ + { + "name": "l1", + "children": [2] + }, + { + "name": "l2" + } + ], + "scene": 0, + "scenes": [{ "nodes": [0] }] +} +"#; - assert_eq!(result.len(), 1); - assert_eq!(result[0].name, "l2"); - assert_eq!(result[0].children.len(), 0); + let dir = Dir::default(); + dir.insert_asset_text(Path::new(gltf_path), gltf_str); + let mut app = test_app(dir); + app.update(); + let asset_server = app.world().resource::().clone(); + let handle: Handle = asset_server.load(gltf_path); + let handle_id = handle.id(); + app.world_mut().spawn(handle.clone()); + app.update(); + run_app_until(&mut app, |_world| { + let load_state = asset_server.get_load_state(handle_id).unwrap(); + if matches!(load_state, LoadState::Failed(_)) { + Some(()) + } else { + None + } + }); + let load_state = asset_server.get_load_state(handle_id).unwrap(); + assert!(matches!(load_state, LoadState::Failed(_))); } }