Open alice-i-cecile opened 12 months ago
Assuming "by path" is #Animation0
?
The issue is that animations (and every sub asset in a gltf file) is guaranteed to have a path, but labels are optional.
To keep it consistent, labeled assets are always reachable by path, and it's not possible to have the same asset with two labels (#Animation0
and #Animation-Idle
).
Assuming "by path" is #Animation0?
Correct, that was what I meant.
I'm working on a name-based GLTF workflow as breaking each individual asset I'm building into individual scenes is a headache. As a datapoint, with just two sets of wall tiles for a hex world, I'm already over 150 nodes. There's no easy way for me to figure out which index I want to load at this point.
I'm trying to figure out what are the key barriers to using a name-based asset label (tiles.glb#Node-cell_base_rock
) vs index-based (tiles.glb#Node178
).
To keep it consistent, labeled assets are always reachable by path, and it's not possible to have the same asset with two labels (#Animation0 and #Animation-Idle).
@mockersf is this a technical limitation of the AssetServer
? Or specific to the bevy_gltf crate?
If it's a technical limitation of AssetServer
, would there be opposition to a PR that adds a GltfLoaderSettings.by_name
field that causes assets to be registered with their name instead of index? With support for falling back to indexed name if there is no name for the asset.
It's a limitation of how assets work, and adding a setting to the gltf loader could be a good idea đź‘Ť
I asked in the discord about how difficult it would be to add LoadContext::add_asset_alias<A<(&mut self, label: String, handle: Handle<A>)
and @NthTensor pointed me at this PR #10153 (Extension-less Assets).
I've confirmed that with that PR it's possible to add a second label for an asset. This is the change I made to verify. It crashes on the main branch, succeeds on the PR branch.
diff --git a/crates/bevy_gltf/src/loader.rs b/crates/bevy_gltf/src/loader.rs
index dae9cb647..12530e094 100644
--- a/crates/bevy_gltf/src/loader.rs
+++ b/crates/bevy_gltf/src/loader.rs
@@ -500,11 +500,18 @@ async fn load_gltf<'a, 'b, 'c>(
let handle = load_context.add_labeled_asset(
mesh_label(&gltf_mesh),
super::GltfMesh {
- primitives,
+ primitives: primitives.clone(),
extras: get_gltf_extras(gltf_mesh.extras()),
},
);
if let Some(name) = gltf_mesh.name() {
+ load_context.add_labeled_asset(
+ format!("Mesh/{}", name),
+ super::GltfMesh {
+ primitives,
+ extras: get_gltf_extras(gltf_mesh.extras()),
+ },
+ );
named_meshes.insert(name.to_string(), handle.clone());
}
meshes.push(handle);
diff --git a/examples/3d/load_gltf.rs b/examples/3d/load_gltf.rs
index f6a412608..266f325ae 100644
--- a/examples/3d/load_gltf.rs
+++ b/examples/3d/load_gltf.rs
@@ -1,6 +1,7 @@
//! Loads and renders a glTF file as a scene.
use bevy::{
+ gltf::GltfNode,
pbr::{CascadeShadowConfigBuilder, DirectionalLightShadowMap},
prelude::*,
};
@@ -49,6 +50,10 @@ fn setup(mut commands: Commands, asset_server: Res<AssetServer>) {
scene: asset_server.load("models/FlightHelmet/FlightHelmet.gltf#Scene0"),
..default()
});
+
+ let handle: Handle<GltfNode> =
+ asset_server.load("models/FlightHelmet/FlightHelmet.gltf#Mesh/Hose_low");
+ commands.spawn(handle);
}
fn animate_light_direction(
I'm going to hold off on implementing the GltfLoaderSettings
option for now. As the extension-less PR moves forward, I'll open a PR to add labels for named assets.
(update 2024-01-09: Added changes to load_gltf
to patch above)
I've confirmed that with that PR it's possible to add a second label for an asset. This is the change I made to verify. It crashes on the main branch, succeeds on the PR branch.
It doesn't crash for me on main with that change. What error are you getting?
This change would also be more controversial than adding settings, as it would use a lot more memory as each sub-asset is duplicated
Exception was:
thread 'main' panicked at /Users/dmlary/src/bevy/vendor/bevy/crates/bevy_asset/src/id.rs:219:13:
The target AssetId<bevy_gltf::GltfMesh>'s TypeId does not match the TypeId of this UntypedAssetId
I did also modify the load_gltf example to actually load a Handle<GltfMesh>
by name. I can update the diff with that piece tomorrow.
I’ve started a thread in asset-dev talking about the possibility of adding label alias support as a mitigation for the duplication problem. Initial question: https://discord.com/channels/691052431525675048/749332104487108618/1193677168216125570
thread: https://discord.com/channels/691052431525675048/749332104487108618/1193754022721622122
If the alias approach isn’t terribly complicated that would probably be a better solution overall, but honestly I haven’t had much engagement about it in the channel.
If the concept falls into controversial territory, I’ll just fall back to the setting option. Better to have some mechanism to use names now.
Aliases would be good if they don't cost too much performances. I tried before (before asset v2, before pipelined rendering) and it wasn't good, but chances are now it won't be as much costly
Would you be up to talking architecture with me on discord sometime? I poked around and have some ideas on how to approach it in the LoadContext
but I don’t have a good view of the larger architecture of AssetServer
. Even just a pointer to where the LoadContext::finish()
results are ingested by AssetServer
would be helpful. I wasn’t able to figure that out quickly on Sunday.
Updated the patch in https://github.com/bevyengine/bevy/issues/10715#issuecomment-1880268103 to include the change made to examples/3d/load_gltf.rs
to trigger the crash on main.
How can Bevy's documentation be improved?
The animated_fox example shows how to load animations by path, but not by animation name.
By loading in the
Gltf
object directly and then debugging it, I was able to determine that I needed the path, not the animation name. However, no example of how I might use the animation name itself was found, despite it being substantially more human-readable.