Improved glTF extension support

[MalekiRe]

by adding the data directly to the nodes.

Objective

Fixes animation support compile bug + extension support

Solution

Adds gltf extension data to the nodes when importing as well as to the scene as a standalone entity.

Also allows you to access a weaker version of the gltf struct from within scenes.

Changelog

Added
GltfExtensions component
WeakGltf component

Migration Guide

[github-actions]

Welcome, new contributor!

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

[alice-i-cecile]

I like this solution much better! Should we revert #11138 as part of this? @CorneliusCornbread: is there anything that would be missing with the simpler design of this PR?

[CorneliusCornbread]

I like this solution much better! Should we revert #11138 as part of this? @CorneliusCornbread: is there anything that would be missing with the simpler design of this PR?

The only thing I could consider 'missing' is that this technically doesn't cover all the extensions a scene can have. But its not really feasible to do that, not without bevy creating its own full representation of gltf internally (or switching to using gltf-rs internally).

To summarize what this PR does, it essentially grabs extension data for just scene information, the catch is you have to deserialize the values again before you can use them but it makes the json data easily accessible via a system. Honestly, having bevy have a full-blown representation of gltf internally for its asset system would solve this problem entirely, but we're avoiding that, at least for now.

[mockersf]

could you change the PR title to be about gltf, not scenes?

[james7132]

Any reason why we are putting this into the component as a string and not as the already-parsed serde_json::Value? At least with the Value, you have a structure to work with, instead of needing to deserialize it again.

[MalekiRe]

Value doesn't implement reflect

[alice-i-cecile]

Github really needs a 🥲 reaction. Can we reflect ignore it? I think that's more useful on the balance: not having to deserialize it seems better than more functional reflection.

[MalekiRe]

Hmm we could, but I prefer to have it be a string over reflect ignoring the field, if you feel strongly about this I will change it.

[MalekiRe]

Actually, for some reason doing reflect ignore causes it to not get stored when you spawn the scene, so we just can't have it

[MalekiRe]

Uses a WeakGltf struct now, to store the gltf info other than scenes in the scene.

[tbillington]

Is this intended to be a temporary solution until there are better ways to author scenes?

It feels really weird to leak details of the original asset format into the ecs world like this.

I don't mean to drive-by after people have already reviewed this, it's just the first time I've seen it.

[MalekiRe]

@tbillington there is some very rough plans to make some sort of trait extension based gltf importer extension thing but they are not yet anywhere where they are gonna exist in an upstreamable state for at least the next version, and probably more. At some point having a complete rewrite of the gltf loader to handle this would be better, but that design should be well thought out, given it would be such a breaking change.

[alice-i-cecile]

I don't think this is a clear enough improvement over the existing solution to warrant the additional complexity at this point. I would really like to:

a) have exactly one way of doing this
b) store the parsed JSON rather than just a string

[mockersf]

very opposed to this: the scene is the wrong level to add everything about the global gltf. if you need informations about the gltf, load the gltf instead of just the scene, then you could add an handle to the parent gltf to the scene however you want, but it's your responsibility to do.

I could understand adding a weak handle to the parent gltf by default, but opposed to anything more than that

[MalekiRe]

I understand the distress here with that. But this means the standard bevy way of doing

commands.spawn(SceneBundle {
   scene: assets.load("my_gltf.gltf#Scene0");
});

Will become a footgun when interacting with any third party gltf extension handling that requires external data.

The above code is fine, until the point that you want to, for example load physics scenes, in which case, with the weak handle you have to do

commands.spawn((SceneBundle {
   scene: assets.load("my_gltf.gltf#Scene0");
}, assets.load("my_gltf.gltf")));

This is a tradeoff I'm willing to make here, and then have a weak handle to the gLTF in the scene, however it def makes spawning scenes have different behavior.

Before with spawning a scene from a gLTF it was assumed that everything you needed for the scene was in the

struct Scene {
    world: World
}

Now this is no longer the case, whenever spawning a scene bundle, if you want extension support, you have to remember to spawn the gltf along side it.

[MalekiRe]

If my other PR #12077 get's merged then I will revert this one back to it's simple state of just putting extension data in nodes, and one for top level.

[mockersf]

Will become a footgun when interacting with any third party gltf extension handling that requires external data.

This seems to be a badly designed gltf extension then? Why do they put data needed for the scene outside of the scene? gltfs can have multiple scenes, how that would work? It is possible to keep the extension data at the scene level in the gltf spec.

[MalekiRe]

The issue here is not the design of the gltf extension, the issue here is that in order to access nodes, materials, or anything else by index ( which is the way you need to access them in gltf, accessing them by name is fraught and not recommended, and extensions thus do not do that ) you need to actually have the data. This is what the original weak gltf solved by re-cloning the vecs of nodes and materials and such into each scene.

Having the functionality to reference the parent gltf is more important than resolving any footguns though, so in order to push this PR forward I have reverted to the old weak handle method, which means you must spawn in a gltf along side the scene, but it works.

[alice-i-cecile]

Before I can approve this, I need:

1. A clear explanation of how this relates to GLTF extension support #11138: what are the differences, why might users use one vs the other and why should we keep both?
2. An example demonstrating how end users can take advantage of this, that answers the questions above and can be quickly validated to ensure we didn't break anything.
3. A better PR description, clearly laying out the key facts and usage patterns.