GLTF: MultiMeshInstance3D import

[2frac]

Allows to import MultiMeshInstance3D objects through EXT_mesh_gpu_instancing GLTF extension: https://github.com/KhronosGroup/glTF/blob/main/extensions/2.0/Vendor/EXT_mesh_gpu_instancing/README.md

Depends on #107672 for blender. Will work by itself for pure GLTF files(if exporter option enabled)

Bugsquad edit: Depends on #108853 and #109103.

Implementation notes:

• Adds 3 fields to GLTFNode - multimesh_translation, multimesh_rotation, multimesh_scale. All of this is referencing accessors indices in Gltf document.

• Adds ImporterMeshInstance3D::multimesh. It is created and filled in GLTFDocument::_generate_mesh_instance - nearby ImporterMeshInstance3D creation.
  (I am not sure about that decision - almost every resource parsed/created separately from nodes(and assigned later), but instances info stored in node description and not in json's root, so maybe its ok)

• Actual MultiMeshInstance3D creation happens in ResourceImporterScene::_pre_fix_node - nearby other nodes creation(and suffix parsing)
  Currenly there is no condition to create MultiMeshInstance3D - it just checks whether it has or not valid multimesh.
  I think ideally there should be suffix condition, OR at least code should check other conditions first - like col/colonly - whatever might be more important to have.
  (for that reason MultiMeshInstance3D is not created early in GLTFDocument)(there is also some mesh material manipulation which duplication we might want to avoid)
  Suffixes might be tricky, however - blender will set name of instance node based on name of main node - not on the name of children node in editor.

• to trigger multimesh creation, code need at least one of three accessors. Since blender creates all of them, i only tested that case.

• On import there is some warnings like "Instance count must be 0 to change the transform format". I think it is related more to copying MultimeshInstance3D and/or scene creation, than to creation code itself.

• There is also some work on GDScript properties and docs left(if rest is even desirable implementation)

[AThousandShips]

Please set up pre-commit hooks locally to ensure style correctness so you don't run CI needlessly to fix style

[lyuma]

Looks good overall. Just needs some changes to where the node conversion happens.

The conversion from ImporterMeshInstance3D to MultiMeshInstance3D should also be performed in GLTFDocumentExtensionConvertImporterMesh (modules/gltf/extensions/gltf_document_extension_convert_importer_mesh.cpp). That is where the translation from an ImporterMesh* node into the corresponding MeshInstance3D (or in this case, MultiMeshInstance3D) is happens in runtime import.

Finally, I'd love to see export code also added either here or in a future PR, so when a user exports a scene with a MultiMeshInstance3D, it creates the accessors and everything. I can't ask you to volunteer more of your time, but having the export logic would make round-trip testing much easier (import -> export -> import). For export, it would need a setting added to the export dialog whether to use the extension (required / optional / disabled), or use the current code which converts it to a normal gltf nodes.

[lyuma]

This is converting the node type too early. This code should be moved into ResourceImporterScene::_generate_meshes(). It should become an if/else statement that creates a MultiMeshInstance3D if mi->get_multimesh().is_valid() and a normal MeshInstance3D otherwise.

[2frac]

current code:

_generate_meshes:

Moving creation to _generate_meshes() makes is look like regular mesh in Advanced Import Settings. While creating inherited scene from same file will actually have MultiMeshes. Is this correct behavior?

[2frac]

If current is more correct, however, it ideally should display its mesh info.

edit: displayed this anyway, for multimeshes created by potential user extensions.

[aaronfranke]

This should be done with an extension, instead of adding properties to GLTFNode.

[2frac]

Does that mean i have to create new GLTFDocumentExtension subclass for parsing(and maybe processing extension's things), or just to use has/get/set_additional_data() in GLTFDocument instead of accessing those fields?

[aaronfranke]

A new GLTFDocumentExtension subclass, which uses the additional data functions. However, this isn't currently possible to do in a safe way with accessors. I've been working on refactoring these to support this, and opened PRs #108302 and #108320 so far, with more still to come... sorry to still ask you to wait longer.

[aaronfranke]

This isn't a safe usage of accessors, and will fail for many many cases. However, currently we don't expose APIs for using accessors safely. Coincidentally, I was planning on working on that as my next PR very soon. I know this is unfortunate to hear, but I'm going to ask you to please wait a few weeks.

[2frac]

Sure, i can wait. It is actually less blocker for my workflow than option to enable extension itself.

What exactly those unsafe cases? Can buffer in accessor be not aligned with its description? Or is it about pointer generally not being safe, for threading and similar things?

[aaronfranke]

For example, this will fail if the accessor primitive isn't float32, and will fail if Godot is compiled with doubles.

[2frac]

That is where the translation from an ImporterMesh* node into the corresponding MeshInstance3D (or in this case, MultiMeshInstance3D) is happens in runtime import.

Is GLTFDocumentExtensionConvertImporterMesh meant to do main conversion and code in _generate_meshes - whatever multimeshes been created by user extensions?

Finally, I'd love to see export code also added either here or in a future PR, so when a user exports a scene with a MultiMeshInstance3D, it creates the accessors and everything.

I might try to look export, since i've got some time for that(but probably after main fixes)
This is not expected to be in my pipeline, however, so testing will be rather simple.

[RichardJECooke]

Will this enhancement fix 109280? The file example there doesn't use EXT_mesh_gpu_instancing, but is just a few nodes that point to the same mesh in the gltf file. My example seems simpler/different, but I don't know enough about 3D files to know if this pull request will cover it or if I should look into another fix.

[2frac]

Will this enhancement fix 109280? The file example there doesn't use EXT_mesh_gpu_instancing

It should, actually. "GPU instances"+"GN instances" in blender option should create EXT_mesh_gpu_instancing entries in GLTF file - but for godot specifically this option added separately in #107672 is semi-prerequisite to that PR).

Not sure why is this not the case for example - it looks like it created without "GPU instances" option.

[passivestar]

There needs to be a way to disable this in case the user wants to do own import logic via an addon (i.e to implement auto proximity clustering for better culling). It's fine if it's enabled by default though

[aaronfranke]

@passivestar This extension would only exist in a glTF file if the asset author explicitly indicated that they want control over batching. I think it would be best to re-export such a file with different settings if this wasn't wanted.

However, even so, you'll be able to disable this (and any other GLTFDocumentExtension) implicitly by registering a new extension with higher priority, overriding the behavior.

[passivestar]

@passivestar This extension would only exist in a glTF file if the asset author explicitly indicated that they want control over batching. I think it would be best to re-export such a file with different settings if this wasn't wanted.

you'd want the extension enabled on export to make sure you write instances to .bin but have custom import logic to have control over how that binary data is converted into multimesh nodes

However, even so, you'll be able to disable this (and any other GLTFDocumentExtension) implicitly by registering a new extension with higher priority, overriding the behavior.

good

[2frac]

Closing PR for now, as there is better implementation now.