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(_))); } }