make more information available from loaded GLTF model by mockersf · Pull Request #1020 · bevyengine/bevy

mockersf · 2020-12-07T02:14:20Z

makes node information available as "path/to/file.gltf#Node0"
this helps the user select correct meshes and their transform between each other when picking nodes out of a gltf file

makes list of primitives per mesh and their material available as "path/to/file.gltf#Mesh0"
this lets the user get all the primitives for a mesh with the correct material

I am not sure about the asset type names, Gltf*, but didn't want confusion with the existing Node and Mesh types

mockersf · 2020-12-07T02:44:32Z

CI failures are due to formatting error in bevy_render, seems like macro matches! now is formatted differently

ambeeeeee

The code changes look good, but I'm not familiar enough with GLTF to comment on much more.

cart · 2020-12-09T03:52:44Z

I like the idea of exposing more information, but I think we could make this a bit cleaner by addressing an issue with how I implemented the loader: right now it assumes that the scene is the top-level asset.

This is problematic for two reasons:

GLTF files can have more than one scene
GLTF files encode useful information other than "raw assets".

I propose something like the following:

#[derive(Debug, TypeUuid)]
#[uuid = "5c7d5f8a-f7b0-4e45-a09e-406c0372fea2"]
struct Gltf {
    scenes: Vec<Handle<Scene>>
    meshes: Vec<Handle<Mesh>>
    materials: Vec<Handle<Material>>
    nodes: Vec<GltfNode>,
}

This would be the "default" asset returned, instead of a single Scene.

It would have a number of benefits:

Able to include whatever metadata you want without needing new asset types for each "thing"
Able to navigate gltf sub-assets in a type-safe way
Supports multiple scenes

The downside is that we could no longer do this:

commands.spawn_scene(server.load("my.gltf"))

But this is almost as good, and is more "correct":

commands.spawn_scene(server.load("my.gltf#Scene0"))

Thoughts?

cart · 2020-12-09T03:55:06Z

The meshes: Vec<Handle<Mesh>> would probably actually be meshes: Vec<GltfMesh>

Then we could add the Handle<Mesh> to GltfMesh

mockersf · 2020-12-09T10:59:24Z

didn't want to break everything as I was discovering GLTF, but that look's better to me!

mockersf · 2020-12-10T02:53:13Z

I changed the object to be returned when loading a gltf to:

pub struct Gltf {
    pub scenes: Vec<Handle<Scene>>,
    pub meshes: Vec<Handle<GltfMesh>>,
    pub materials: Vec<Handle<StandardMaterial>>,
    pub nodes: Vec<Handle<GltfNode>>,
    pub default_scene: Option<Handle<Scene>>,
}

in a separate commit I also added the names feature of gltf and the following fields:

    pub named_scenes: HashMap<String, Handle<Scene>>,
    pub named_meshes: HashMap<String, Handle<GltfMesh>>,
    pub named_materials: HashMap<String, Handle<StandardMaterial>>,
    pub named_nodes: HashMap<String, Handle<GltfNode>>,

on a gltf file I have, this gives me extra information which looks useful:

    named_nodes: {
        "body": StrongHandle<GltfNode>(AssetPathId(AssetPathId(SourcePathId(3806774963703447708), LabelId(11310862584416107483)))),
        "armLeft": StrongHandle<GltfNode>(AssetPathId(AssetPathId(SourcePathId(3806774963703447708), LabelId(15957055608553047399)))),
        "cape": StrongHandle<GltfNode>(AssetPathId(AssetPathId(SourcePathId(3806774963703447708), LabelId(16543170105607056499)))),
        "legRight": StrongHandle<GltfNode>(AssetPathId(AssetPathId(SourcePathId(3806774963703447708), LabelId(3945245733129666297)))),
        "head": StrongHandle<GltfNode>(AssetPathId(AssetPathId(SourcePathId(3806774963703447708), LabelId(12706021548412882015)))),
        "tmpParent": StrongHandle<GltfNode>(AssetPathId(AssetPathId(SourcePathId(3806774963703447708), LabelId(9458706519151557857)))),
        "vampire": StrongHandle<GltfNode>(AssetPathId(AssetPathId(SourcePathId(3806774963703447708), LabelId(13863501689609897062)))),
        "legLeft": StrongHandle<GltfNode>(AssetPathId(AssetPathId(SourcePathId(3806774963703447708), LabelId(11246812150243246633)))),
        "armRight": StrongHandle<GltfNode>(AssetPathId(AssetPathId(SourcePathId(3806774963703447708), LabelId(15717067171356699421)))),
    },

I have sometimes a crash when I launch example load_gltf, maybe one time in ten.
On a valid execution, I have the following AssetEvent<Texture>:

AssetEvent<bevy_render::texture::texture::Texture>::Created { handle: AssetPathId(AssetPathId(SourcePathId(14915453948569874894), LabelId(6298619649789039366))) }
AssetEvent<bevy_render::texture::texture::Texture>::Created { handle: AssetPathId(AssetPathId(SourcePathId(12698207005980422666), LabelId(6298619649789039366))) }
AssetEvent<bevy_render::texture::texture::Texture>::Created { handle: AssetPathId(AssetPathId(SourcePathId(15334466250598859188), LabelId(6298619649789039366))) }
AssetEvent<bevy_render::texture::texture::Texture>::Created { handle: AssetPathId(AssetPathId(SourcePathId(1984338858159794673), LabelId(6298619649789039366))) }
AssetEvent<bevy_render::texture::texture::Texture>::Created { handle: AssetPathId(AssetPathId(SourcePathId(10130169047868106944), LabelId(6298619649789039366))) }

on a crash:

AssetEvent<bevy_render::texture::texture::Texture>::Created { handle: AssetPathId(AssetPathId(SourcePathId(15334466250598859188), LabelId(6298619649789039366))) }
AssetEvent<bevy_render::texture::texture::Texture>::Created { handle: AssetPathId(AssetPathId(SourcePathId(10130169047868106944), LabelId(6298619649789039366))) }
AssetEvent<bevy_render::texture::texture::Texture>::Modified { handle: AssetPathId(AssetPathId(SourcePathId(15334466250598859188), LabelId(6298619649789039366))) }
AssetEvent<bevy_render::texture::texture::Texture>::Created { handle: AssetPathId(AssetPathId(SourcePathId(12698207005980422666), LabelId(6298619649789039366))) }
thread 'main' panicked at 'TextureId(2d05c484-52b7-4a0d-9f8d-e0a2c0d1495c)', crates/bevy_wgpu/src/renderer/wgpu_render_resource_context.rs:481:52
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

also there is a conflict with #1016 that I'll fix tomorrow but I would appreciate some feedback as the PR is getting bigger

cart · 2020-12-10T19:55:47Z

Yup so far these changes are looking great!

mockersf · 2020-12-14T00:45:35Z

fixed the conflict.

for GltfNode:

#[derive(Debug, Clone, TypeUuid)]
 #[uuid = "dad74750-1fd6-460f-ac51-0a7937563865"]
 pub struct GltfNode {
     pub children: Vec<GltfNode>,
     pub mesh: Option<Handle<GltfMesh>>,
     pub transform: bevy_transform::prelude::Transform,
 }

I would have liked children to be a Vec<Handle<GltfNode>>, but that doesn't compile because

error[E0275]: overflow evaluating the requirement `GltfNode: Send`
  --> crates/bevy_gltf/src/lib.rs:36:16
   |
36 |     pub nodes: Vec<Handle<GltfNode>>,
   |                ^^^^^^^^^^^^^^^^^^^^^
   |
  ::: /Users/francois/Dev/vleue/bevy_friends/bevy/crates/bevy_asset/src/handle.rs:63:8
   |
63 |     T: Asset,
   |        ----- required by this bound in `Handle`
   |
   = note: required because of the requirements on the impl of `AssetDynamic` for `GltfNode`
   = note: required because of the requirements on the impl of `bevy_asset::Asset` for `GltfNode`

The other way around this would be to have a Vec<usize> with just the index of the children nodes. What do you think?

mockersf · 2020-12-24T03:21:22Z

should not be merged before #1142 due to Labels usage

mockersf · 2020-12-28T03:31:43Z

works with #1152, would be better with #1109 when it gets merged

cart · 2020-12-31T20:55:30Z

Cool these changes look good to me. I suspect that all of this sorting / hierarchy navigation / allocation might be a bit expensive when loading large gltf files, but this is such a clear usability improvement that I'm down to have that conversation if/when it becomes an issue.

cart · 2020-12-31T20:56:40Z

I just merged #1109. Feel free to follow up with Name changes if you feel so inclined 😄

smokku · 2021-01-27T17:13:48Z

crates/bevy_asset/src/loader.rs

    }

-    pub fn set_labeled_asset(&mut self, label: &str, asset: LoadedAsset) {
+    pub fn set_labeled_asset<T: Asset>(&mut self, label: &str, asset: LoadedAsset) -> Handle<T> {


This change makes this kind of code fail compilation:

load_context.set_labeled_asset("cube", LoadedAsset::new(mesh));

Is there a way to not be bothered with specifying the T parameter?

Hmm yeah that is suboptimal. Its both less ergonomic and creates a class of bug where the user specifies the wrong asset type.

We could make it LoadedAsset<T>, then created a BoxedLoadedAsset that we coerce it into within set_labeled_asset

mockersf marked this pull request as draft December 7, 2020 02:25

mockersf changed the title ~~make gltf nodes available as assets~~ make more information available from loaded GLTF model Dec 7, 2020

mockersf marked this pull request as ready for review December 7, 2020 02:43

mockersf force-pushed the gltf_node branch from eeb583e to 230ada7 Compare December 7, 2020 20:29

ambeeeeee reviewed Dec 8, 2020

View reviewed changes

Moxinilian added A-Assets Load files from disk to use for things like images, models, and sounds C-Feature A new feature, making something new possible labels Dec 9, 2020

mockersf force-pushed the gltf_node branch from ad928c8 to 09917ea Compare December 13, 2020 15:06

mockersf force-pushed the gltf_node branch from 2924c0a to 710f6d9 Compare December 24, 2020 02:56

mockersf added 7 commits December 28, 2020 04:29

make gltf nodes available as assets

791d907

add list of primitive per mesh, and their associated material

0867828

complete gltf structure

a470964

get names of gltf assets

cc30ad2

only load materials once

6da061c

format

df4ef7e

add labels with node names

9a915c0

mockersf force-pushed the gltf_node branch from 5fec782 to 9a915c0 Compare December 28, 2020 03:30

cart merged commit 21794fe into bevyengine:master Dec 31, 2020

mockersf mentioned this pull request Jan 1, 2021

use Name on node when loading a gltf file #1183

Merged

This was referenced Jan 3, 2021

let user run a function on a scene entities #1058

Merged

fix example update_gltf_scene: add label to load asset #1204

Merged

smokku reviewed Jan 27, 2021

View reviewed changes

mockersf mentioned this pull request Jan 31, 2021

bind the labeled asset type to the actual loaded asset #1363

Merged

mockersf deleted the gltf_node branch April 27, 2021 23:51

Uh oh!

Conversation

mockersf commented Dec 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mockersf commented Dec 7, 2020

Uh oh!

ambeeeeee left a comment

Choose a reason for hiding this comment

Uh oh!

cart commented Dec 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cart commented Dec 9, 2020

Uh oh!

mockersf commented Dec 9, 2020

Uh oh!

mockersf commented Dec 10, 2020

Uh oh!

cart commented Dec 10, 2020

Uh oh!

mockersf commented Dec 14, 2020

Uh oh!

mockersf commented Dec 24, 2020

Uh oh!

mockersf commented Dec 28, 2020

Uh oh!

cart commented Dec 31, 2020

Uh oh!

cart commented Dec 31, 2020

Uh oh!

smokku Jan 27, 2021

Choose a reason for hiding this comment

Uh oh!

cart Jan 27, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mockersf commented Dec 7, 2020 •

edited

Loading

cart commented Dec 9, 2020 •

edited

Loading