Skip to content

Commit

Permalink
Make gLTF node children Handle instead of objects (bevyengine#13707)
Browse files Browse the repository at this point in the history
Part of bevyengine#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](bevyengine#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<Handle<GltfNode>>` instead of
`Vec<GltfNode>`
- Having children cycles between gLTF nodes in a gLTF document is now an
explicit asset loading failure.

## Migration Guide

If accessing children, use `Assets<GltfNode>` resource to get the actual
child object.

#### Before

```rs
fn gltf_print_first_node_children_system(gltf_component_query: Query<Handle<Gltf>>, gltf_assets: Res<Assets<Gltf>>, gltf_nodes: Res<Assets<GltfNode>>) {
    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<Handle<Gltf>>, gltf_assets: Res<Assets<Gltf>>, gltf_nodes: Res<Assets<GltfNode>>) {
    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);
    }
}
```
  • Loading branch information
freiksenet authored and barsoosayque committed Jul 15, 2024
1 parent b231ebb commit e419537
Show file tree
Hide file tree
Showing 3 changed files with 356 additions and 109 deletions.
3 changes: 3 additions & 0 deletions crates/bevy_gltf/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ serde = { version = "1.0", features = ["derive"] }
serde_json = "1"
smallvec = "1.11"

[dev-dependencies]
bevy_log = { path = "../bevy_log", version = "0.14.0" }

[lints]
workspace = true

Expand Down
39 changes: 24 additions & 15 deletions crates/bevy_gltf/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<GltfNode>,
pub children: Vec<Handle<GltfNode>>,
/// Mesh of the node.
pub mesh: Option<Handle<GltfMesh>>,
/// Local transform.
Expand All @@ -225,14 +223,13 @@ impl GltfNode {
/// Create a node extracting name and index from glTF def
pub fn new(
node: &gltf::Node,
children: Vec<GltfNode>,
children: Vec<Handle<GltfNode>>,
mesh: Option<Handle<GltfMesh>>,
transform: bevy_transform::prelude::Transform,
extras: Option<GltfExtras>,
) -> Self {
Self {
index: node.index(),
asset_label: GltfAssetLabel::Node(node.index()),
name: if let Some(name) = node.name() {
name.to_string()
} else {
Expand All @@ -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)
Expand All @@ -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<GltfPrimitive>,
/// Additional data.
Expand All @@ -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 {
Expand All @@ -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`].
Expand All @@ -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<Mesh>,
/// Material to apply to the `mesh`.
Expand All @@ -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 {
Expand All @@ -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.
Expand Down Expand Up @@ -405,7 +414,7 @@ pub struct GltfMaterialExtras {
/// let gltf_scene: Handle<Scene> = 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),
Expand Down
Loading

0 comments on commit e419537

Please sign in to comment.