Use material name for mesh entity's Name when available

[rendaoer]

Objective

Fixes #19286

Solution

Use material name for mesh entity's Name when available

Testing

Test code, modified from examples/load_gltf.rs

//! Loads and renders a glTF file as a scene.

use bevy::{gltf::GltfMaterialName, prelude::*, scene::SceneInstanceReady};

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_systems(Startup, setup)
        .add_observer(on_scene_load)
        .run();
}

fn setup(mut commands: Commands, asset_server: Res<AssetServer>) {
    commands.spawn((
        Camera3d::default(),
        Transform::from_xyz(0.7, 0.7, 1.0).looking_at(Vec3::new(0.0, 0.3, 0.0), Vec3::Y),
    ));

    commands.spawn((DirectionalLight {
        shadows_enabled: true,
        ..default()
    },));

    commands.spawn(SceneRoot(asset_server.load(
        GltfAssetLabel::Scene(0).from_asset("models/FlightHelmet/FlightHelmet.gltf"),
    )));
}

fn on_scene_load(
    trigger: Trigger<SceneInstanceReady>,
    children: Query<&Children>,
    names: Query<&Name>,
    material_names: Query<&GltfMaterialName>,
) {
    let target = trigger.target();

    for child in children.iter_descendants(target) {
        let name = if let Ok(name) = names.get(child) {
            Some(name.to_string())
        } else {
            None
        };
        let material_name = if let Ok(name) = material_names.get(child) {
            Some(name.0.clone())
        } else {
            None
        };

        info!("Entity name:{:?} | material name:{:?}", name, material_name);
    }
}

---

Showcase

Run log:

[github-actions]

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes.

[greeble-dev]

I've tagged this as M-Needs-Migration-Guide in case a user might have been relying on the old naming behaviour to find the entity. Which seems like a really niche case, but not impossible.

[alice-i-cecile]

TIL that we're spawning the materials as their own entities 🤔 Given that, this is reasonable behavior. Migration guide needs a bit of an editing pass; I'll do my best to fix it in suggestions.

[alice-i-cecile]

I would also prefer if we had some sort of marker like GltfMaterial or something, and removed GltfMaterialName entirely, in favor of a joint query. That's a larger change though: this is a fine start.

[greeble-dev]

TIL that we're spawning the materials as their own entities 🤔

This might be a misunderstanding? At least there's no entity that contains a material on its own. The entity named in this PR corresponds to a gltTF primitive, so it contains Mesh3d and MeshMaterial3d components. glTF primitives don't have names, but assigning them the material name does seem fairly intuitive for debugging.

This situation is always going to be a bit confusing because the Bevy notion of "mesh" doesn't match glTF's. I suspect a longer term improvement is to have a distinct GltfMeshName component, which could be more controversial. I might look into that as a potential follow up.

[rendaoer]

When generating gltf in bevy, each material will have its own entity, and different mesh parts only belong to this material entity. According to the current situation, I think it is reasonable to display the material name. I think this might improve once we have our own editor, maybe in the bevy editor gltf will have its own display?

[greeble-dev]

I'd agree that displaying the material name in inspectors is reasonable, particularly when a mesh has multiple materials. But after looking at this some more I think it's likely to break users who are using the name to find entities in code. But maybe that can be fixed by adding some more data.

Say there's a glTF that has a node named "fox", which has a mesh named "fox_mesh", which is a single primitive with a material named "fox_material". That gets turned into two entities:

• (Name: "fox", Transform) <-- glTF node.
  • (Name: "fox_mesh", Transform, Mesh3d, MeshMaterial3d) <-- glTF primitive.

Before this PR the primitive entity would have been named "fox_mesh". After this PR, it will be named "fox_material". There won't be anything named "fox_mesh".

I don't think there's a simple and intuitive solution to this as Bevy's entity structure just doesn't match the glTF hierarchy.

The least worst solution might be to add a new component to the primitive entity:

struct GltfPrimitiveComponent {
    mesh_name: String,
    index: usize
}

That way this PR can stay the same - the Name component can still change to be the material name. But the migration guide is "if you were searching for the mesh by Name::name, instead search for GltfPrimitiveComponent::mesh_name".

If this PR lands as-is then I can do a follow up PR to pitch adding GltfPrimitiveComponent.

[greeble-dev]

After digging a bit more I think struct GltfMeshName(String) is better than GltfPrimitiveComponent. Is a little unintuitive that the GltfMeshName will be duplicated across multiple primitives, but might be the least worst solution.

Related: #13473, which has some more background and originally pitched GltfMeshName.

[rendaoer]

I think the name can be changed to MeshName.MaterialName

[rendaoer]

I also need to test the results in different situations

[greeble-dev]

MeshName.MaterialName makes sense to me as an inspection friendly name.

[greeble-dev]

Could also consider MeshName.MaterialName if a mesh has multiple primitives, and MeshName if it only has a single primitive. That keeps some backwards compatibility.

[rendaoer]

If there is no material, it will keep MeshName. If there is a material, it will still use MeshName.MaterialName

[rendaoer]

Thanks for your review, showing MeshName.MaterialName is definitely a better choice than just showing MaterialName

[greeble-dev]

Looks good to me once the markdown passes CI.

I'll look into following up with a PR that adds GltfMeshName. This will affect the migration guide as users should probably prefer that component - Name would be just for debugging/inspection.

[rendaoer]

I think the usage rate of GltfMeshName should not be very high. Users usually don't modify Mesh, but the probability of modifying Material is still quite high. Usually there is no need to check the name of Mesh, because there is only one Mesh name in Gltf. Even if you want to modify Mesh, you have to look for it according to the name of GltfMaterialName.

[greeble-dev]

My guess is that some users with single primitive meshes will have been using the mesh name to find the material. While they could switch to using GltfMaterialName, adding a distinct mesh name would make upgrading easier for them.

[rendaoer]

My guess is that some users with single primitive meshes will have been using the mesh name to find the material. While they could switch to using GltfMaterialName, adding a distinct mesh name would make upgrading easier for them.

Side effects appeared haha, now I think what you said makes a lot of sense, I decided to add GltfMeshNam