bevy_gltf: Add support for using name-based labels #11279
Conversation
This is a first pass at supporting name-based labels for GLTF. This is implemented by adding a the settings field `GltfLoaderSettings.label_by_name`. When set to true, the majority of asset labels will be created using the name of the asset in the gltf file if present. If there is no name, it will fall back to the old naming scheme. There is a bit of a usability problem here as the load settings must be set every time an asset is loaded from the file. On top of that there's a race-condition in the label naming as the first asset AssetServer loads for the file determines how the labels were created. This is demonstrated in the animaled_fox example.
alice-i-cecile
added
C-Feature
This is #11111 |
examples/animation/animated_fox.rs
Outdated
| @@ -4,6 +4,7 @@ use std::f32::consts::PI; | |||
| use std::time::Duration; | |||
|
|
|||
| use bevy::{animation::RepeatAnimation, pbr::CascadeShadowConfigBuilder, prelude::*}; | |||
| use bevy_internal::gltf::GltfLoaderSettings; | |||
There was a problem hiding this comment.
no bevy_internal in examples even if your IDE really really want to import it 😄
Honestly, I agree with supporting both name and index-based labels. It looks like this will become possible with #10153, but we end up duplicating the assets for each label (#10715 (comment)). The most obvious step from there seems to be to exploring asset label aliasing, allowing multiple labels to point at the same asset. The challenge there is that the PR will likely fall into Controversial territory, causing delays. If there's sufficient interest in asset label aliases I'm happy to put together a PR building on the changes in #10153, but I think a more immediate solution is needed. Something that has a shorter path to adoption. |
|
This isn't my preferred approach here, but I felt like it was useful to at least complete the implementation so it's available. Also moving this out of draft. |
| |s: &mut GltfLoaderSettings| { | ||
| s.label_by_name = true; | ||
| }, | ||
| ), |
There was a problem hiding this comment.
Bah, forgot to remove this. Due to #11111 this does not work right now.
| @@ -1194,35 +1229,37 @@ fn primitive_name(mesh: &gltf::Mesh, primitive: &Primitive) -> String { | |||
| } | |||
There was a problem hiding this comment.
I didn't change primitive_name to match the changes to the asset labels because I wanted to limit scope to only the asset labels. This appears to be used when naming the entities within a Scene. This can be changed to match if anyone has a preference here.
alice-i-cecile
added
A-glTF
Objective
Allow the use of names while loading assets from a gltf file.
Solution
This is a first pass at supporting name-based labels for GLTF, and intended to spark discussion.
This is implemented by adding a the settings field
GltfLoaderSettings.label_by_name. When set to true, the majority of asset labels will be created using the name of the asset in the gltf file if present. If there is no name, it will fall back to the index-based naming scheme.When
label_by_name == true, Animation, Mesh, Node, and Primitive assets will use any available name when adding labeled assets. For example, the naming formodels/animated/Fox.glbbecomes:There is a usability problem here as the load settings must be set every time an asset is loaded from the file.
On top of that there's a race-condition in the label naming as the first asset AssetServer loads for the file determines how the labels were created. This is demonstrated in the modified animaled_fox example in this PR. When it runs, it's unable to find the name-based assets because the index-based assets have been loaded first. As a result we get the asset not found error:
Questions for Contributors
my_scenes.gltf#Scene:'Winter Scene'#, so we never have to escape anything that happens to be in the nameChangelog
GltfLoaderSettings.label_by_name; when set to true, will use gltf names in most asset labels (Animation, Mesh, Node, Primitive)