Refactor GltfLoader::load_gltf

[hukasu]

Objective

Refactor the load_gltf method in bevy_gltf/loader.rs, the criteria for the split is kind of arbitrary but at least it is not a 2.6k line file.

Solution

Move methods and structs found in bevy_gltf/loader.rs into multiple new modules.

Testing

cargo run -p ci

[github-actions]

Welcome, new contributor!

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

[suprohub]

any updates?

[hukasu]

The team is focused on 0.15.1, i'm in wait until 0.16 becames the target, then i will merge and do the comments

[hukasu]

there were too many changes on bevy_gltf since my last commit, will restart from main, will reopen when i commit something

[hukasu]

@alice-i-cecile can we get this moving now? who would be SME on this topic to do the reviews?

[thepackett]

Overall this seems like a good PR. Having read through the GltfLoader code before, this is a significant improvement. I ran CI locally successfully and was able to confirm that examples using Gltf assets were working as expected. The only question I have is whether or not the extension traits should be public or not.

[andriyDev]

I am not a fan of how many extension traits there are. This doesn't seem necessary, and results in a bunch of tiny files which hurts readability IMO. For example crates/bevy_gltf/src/ext/scene.rs is a whopping 14 lines. Jumping to this file just to see "oh this is just an extension trait" is quite underwhelming.

I would prefer just free functions for most of these (like the original implementation), just organized into modules more. For example, rather than node.node_name(), we just have node_name(&node) and node_transform(&node) and so on. But with the important part being that node_name and node_transform should be in a mod node.

One case where "extension traits" is likely desirable is labels - these could all be a single trait ToLabel which Scene, Skin, Texture, etc can implement. Now all these disparate label stuffs can be brought under one roof and we can see how all of these are really the same thing over and over again. Of course whether this really is the way we want to go I'm not sure (for example Skin already breaks this pattern since it wants a inverse_bind_matrics_label version - though this could just sit next to the trait implementations as inverse_bind_matrics_label_for_skin to again keep the similar implementations together.

[hukasu]

@andriyDev see if this solution is satisfactory

[rparrett]

I added some specific suggestions about the ext module, though I think I generally agree that this is feeling excessively "enterprise-grade."

I don't really agree with the way the changeset is presented. The current arrangement of "the loader stuff is in loader.rs" is logical and not necessarily worse than an "arbitrary" split.