Import/export GLTF extras to node->meta and back

[demolke]

This is useful for custom tagging of objects with properties (for example in Blender) and having this available in the editor for scripting.

• Adds import logic to propagate the parsed GLTF extras all the way to the resulting Node->meta where it creates extras dictionary
• Adds export logic to save Godot Object meta/extras dictionary into GLTF extras
• Support nodes, meshes and materials (in GLTF sense of the words)

Closes: godotengine/godot-proposals#8271

[AThousandShips]

Please open a proposal to discuss the need and use cases for this feature

[demolke]

Thanks, looks like there is one in godotengine/godot-proposals#8271, so I'll try to reuse that one

[noidexe]

I tested it with a sample scene and overall it works well but I noticed a few things:

• materials don't get metadata
• blender mesh objects with -colonly and -convcolonly lose the metadata (in my workaround I assign it to the resulting StaticBody3D)
• metadata in mesh datablocks doesn't end in the corresponding ArrayMesh resource. It seems to be added to every MeshInstance3D using the mesh. This could create collisions if a meshinstance and its assigned mesh happen to have a custom property with the same name

[fire]

We also worked on a similar pull request with meta here #39024 #40474

[demolke]

@noidexe I've uploaded new version

• GLTF material extras gets set in Godot Material
• collision node extras get set on StaticBody3D (does not support collider mesh extra CollisionShape3D)
• MeshInstance3D gets the node extras and Mesh gets the mesh extra properties

[fire]

Part of my goal is that meta import and export is symmetrical for gltf. Let me know if thats too much or you agree.

[demolke]

I was thinking the same thing. I wanted to add a test that creates a scene, serializes it to gltf, deserializes it and checks it has the right meta, but the fact the mesh re-generation is happening in ResourceImporterScene makes testing it hairy.

I also don't understand github too much, so thought I was not including it in this PR, but somehow my in-progress stuff got attached - ignore it for now, I'll try to figure out.

[demolke]

I feel bad for having you review this, apologies. I meant to say in my last comment that it needs more work - especially the test change, which I did not want to attach at all.

I guess I should mark it as draft?

[demolke]

I've added both import and export as discussed in #86183 (comment) and added a test it works as expected.

I haven't found any import/export testing to take inspiration from - it might be my unfamiliarity with Godot, but the test has to instantiate way more of Godot that I would like - the whole import process is extremely intertwined with Editor/Engine (and their subsequent dependencies).

It's also not possible to do this in-memory, so a number of files (.gltf, .bin, .scn) get written and subsequently read from disk during the test run, which I'm not happy about.

Please take a look, cheers

[aaronfranke]

@demolke Please review the CI failure and resolve all issues until the CI turns green. Only then is it ready for human review.

[aaronfranke]

@akien-mga Does the way the test case hooks into the editor look fine to you? Another option is to have the tests only use GLTFDocument/GLTFState, the same as the runtime import/export pipeline. However it seems like a good idea to allow test cases to test the editor's full import pipeline.

[aaronfranke]

Thank you for this! This looks good from the GLTF and import pipeline side of things.

[lyuma]

Reviewed on a call with fire. Looks good.

The only thing we need to fix is one case which looks like it may be use-after-free. we should reorganize that function to ensure the memdelete is after we copy meta from p_original_node.

[aaronfranke]

@demolke: Thanks for taking a look, I've updated it except I did not call it gltf_extras, but just extras.

After thinking about it more, I agree that this is a good idea. If someone uses the .blend importer and that generates extras, the fact that the .blend file importer uses GLTF under the hood is an implementation detail, and the user ideally shouldn't know. So yeah, I like this PR as-is.

[lyuma]

Hmm, I am worried extras is not something I think most people will understand. (It is a technical term in the gltf spec...)
Is there a better term to indicate it is imported? Or is extras ok

[demolke]

Hmm, I am worried extras is not something I think most people will understand. (It is a technical term in the gltf spec...) Is there a better term to indicate it is imported? Or is extras ok

In absence of other suggestions, I think extras is okay. We can't just call it imported_extras since they also get exported. ported_extras :).

The only other one I can see working is Imported/exported metadata, but it's quite long and since the main usage of this is from code it should be typo proof, so I would stick with extras.

[fire]

I do like that extras is symmetrical on import and export.