Add labels to Gltf Node and Mesh assets #13558

freiksenet · 2024-05-28T15:26:44Z

Objective

Add labels to GltfNode and GltfMesh - they are missing from the assets even though they are need if one wants to write a custom Gltf spawning logic.

Eg AnimationPlayer relies on Name component of the node entities to control the animation. There is no way to actually get names of the gltf nodes, thus you can't manually spawn subtree from the scene and animate it.

Solution

Add label field and make use of existing label creation logic to store it there.

Testing

Ran all tests
Fixed tests for node_hierarchy to use lable now

github-actions · 2024-05-28T15:26:59Z

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

alice-i-cecile

This is a nice simple change, and the code quality benefits are a great side effect.

mockersf · 2024-05-28T17:08:23Z

doesn't do what it says, it always uses the auto generated label

freiksenet · 2024-05-28T17:38:13Z

Yeah, let's not merge it yet - we discussed it with @mockersf and I'll fix it tomorrow, then it should be gtg. It seems I was looking at different bevy version's code when using those helper functions.

freiksenet · 2024-05-29T09:42:47Z

Ok, fixed the logic now. I guess the following can be a possible follow-up:

Add label to other "GLTF" named types.
Consider wrapping stuff like StandardMaterial or Scene in a Gltf wrapper that has same label/name logic.
Add backlinks to parents to all nodes and possibly backlink to scene - this can be problematic because the code operates partially on gltf::Node and partially on GltfNode types (confusing).
It is maybe ergonomic, but currently GltfNodes in children of GltfNodes are not the same GltfNodes as the ones in Assets, because they are cloned. I think children should be changed to be Handle. That is a breaking change.
Potentially a way of loading GLTF that doesn't create a bevy scene for scenes would be good - I think loader shouldn't be the place scene is created, but rather a separate command should be added to spawn gltf that handles all the stuff there.

Could you take a look @mockersf?

freiksenet · 2024-05-29T11:27:27Z

I think I'll try to prototype a different kind of bevy gltf plugin for my usages, that doesn't do this work in loaders and let's see how it goes. I think this PR is still good tho.

mockersf · 2024-05-29T22:11:41Z

crates/bevy_gltf/src/lib.rs

+    }
+
+    /// Computed name for a node - either a user defined name or a generated name from index
+    pub fn name(&self) -> String {


I would prefer those to not return owned string, but not sur that's easy

I can make name/asset_label be precomputed on construction - but that mean it won't update if the object is mutated. That's probably okay tho.

Added a commit with that option, can revert it.

crates/bevy_gltf/src/lib.rs

Co-authored-by: François Mockers <[email protected]>

crates/bevy_gltf/src/lib.rs

Olle-Lukowski

Other than the concerns that were already mentioned, this looks good to me!

….com/freiksenet/bevy into freiksenet/add-labels-to-gltf-assets

alice-i-cecile · 2024-06-04T15:30:22Z

Can you resolve merge conflicts?

…-to-gltf-assets

freiksenet · 2024-06-04T15:35:35Z

@mockersf Addressed the issues, ported to new GltfAssetLabel system, added name/index/label for primitive. Ready for review.

crates/bevy_gltf/src/lib.rs

crates/bevy_gltf/src/loader.rs

freiksenet · 2024-06-05T08:39:25Z

@mockersf Apologies, one change didn't get added, I had those fixes in already. Welp.

freiksenet · 2024-06-05T09:01:10Z

@mockersf Added an issue with follow-up work, would appreciate your input. #13681

…-to-gltf-assets

mockersf

I would prefer if the asset_label fields were functions instead, as is they could have incoherent data with the index fields

alice-i-cecile · 2024-06-05T23:10:30Z

I'm going to merge this for now, but I think that Francois's comment above has merit.

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](#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); } } ```

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); } } ```

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](bevyengine/bevy#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); } } ```

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); } } ```

Add labels to Gltf Node and Mesh assets

28139fe

alice-i-cecile added C-Feature A new feature, making something new possible A-Assets Load files from disk to use for things like images, models, and sounds labels May 28, 2024

alice-i-cecile requested a review from IceSentry May 28, 2024 16:04

alice-i-cecile approved these changes May 28, 2024

View reviewed changes

alice-i-cecile added X-Uncontroversial This work is generally agreed upon D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels May 28, 2024

mockersf removed X-Uncontroversial This work is generally agreed upon D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels May 28, 2024

Fix labeling logic

98bf1e6

freiksenet requested a review from alice-i-cecile May 29, 2024 11:18

alice-i-cecile added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label May 29, 2024

mockersf reviewed May 29, 2024

View reviewed changes