Allow intercepting GLTFDocument on scene import

[RedMser]

Closes godotengine/godot-proposals#4968

• Add virtual method _initialize_gltf to all glTF-based importers (gltf/glb, fbx, blend), which can intercept the step between creation and usage of GLTFDocument and GLTFState.
  • User would create a plugin which uses add_scene_format_importer_plugin to register a EditorSceneFormatImporter. It can inherit from e.g. the GLTF importer and merely override that specific virtual method to conditionally modify the document/state.
• With this, the user can e.g. create scripts that inherit from GLTFDocumentExtension and let those run for all imported scene assets that go through the glTF pipeline (filtering by path is possible). Among other things, this allows for direct access to the "extensions" and "extras" dictionaries of the glTF nodes.
• MRP for testing: gltf_document_extensions.zip
  • Re-importing the .gltf file should log a bunch of information, including the "extensions" section. Change the extends inside of plugin.gd to try out .fbx and .blend (don't forget to reload the plugin!).
  • The .blend file in specific has objects which get scaled based on a custom property variable.
  • The godot-props-test.py script can be installed and enabled as a Blender add-on to try out the user extension functionality, which will cause a glTF extension to be printed on import.

Example code:

# Create a plugin in project settings, then replace plugin.gd with this:
@tool
extends EditorPlugin

var importer

func _enter_tree():
	importer = MyImporter.new()
	add_scene_format_importer_plugin(importer, true)

func _exit_tree():
	remove_scene_format_importer_plugin(importer)

# Base class can be changed to FBX, Blend, etc.
class MyImporter extends EditorSceneFormatImporterGLTF:
	func _initialize_gltf(original_path, gltf_path, document, state):
		prints("orig path:", original_path, "// gltf path:", gltf_path)
		document.extensions.append(MyDocumentExtension.new())

class MyDocumentExtension extends GLTFDocumentExtension:
	func _import_node(state, gltf_node, json, node):
		prints("parent:", node.get_parent().name)
		prints("scene_name:", state.scene_name)
		prints("json:", json)
		return 0

[fire]

Is it possible to avoid the use of class name. I think the similar system of an array .gd files may work.

An array of gdscript is not so good now that I think about it, because gdextentions may extend godot.

Will need to ponder more.

[fire]

I don't think #63072 was an improvement. Testing showed it was a dead end.

[fire]

I have made a gltf extensions sandbox which was using the older approach.

https://github.com/V-Sekai/godot-gltf-sandbox I can port it to the newer approach.

[RedMser]

Is it possible to avoid the use of class name.

RichTextEffect is another system that does it the same way (by creating scripts that use class_name). Don't know what alternatives there really are. I can't think of other systems in Godot that use the inspector to assign custom functionality (besides Import Script I guess?)

[RedMser]

@fire Did some bigger changes, which I think makes the code a bit clearer:

• Create new base class EditorSceneFormatImporterGLTFBase from which GLTF/FBX/Blend inherit. It contains all shared functionality between the glTF-dependent formats:
  • New import options that all formats use.
  • The creation and conversion of the GLTFDocument, including extensions.
• Moved the extension check from EditorSceneFormatImporter::get_import_options up, so the caller has to filter for it now (in this case ResourceImporterScene::get_import_options).

These changes could be split off from adding GLTFDocumentExtension, if you want it to be two PRs (or two commits in one PR)?

[fire]

I haven't checked precisely but if EditorSceneFormatImporterGLTFBase is a base class, can you make it GDREGISTER_VIRTUAL_CLASS?

[RedMser]

I haven't checked precisely but if EditorSceneFormatImporterGLTFBase is a base class, can you make it GDREGISTER_VIRTUAL_CLASS?

Done. Should I also add bind_method for generate_gltf?

[fire]

Did you see the style check failures?

[fire]

add bind_method for generate_gltf?

I think so. We can also do the thing where we wait for people to complain. Since if it was essential the compiler and tests would have failed.

[RedMser]

Hmm on second thought, I think since import_scene and similar methods aren't bound either, I think it's okay to keep it like this. If people need it in GDScript, it needs a bigger change anyway, since the raw pointers are causing template errors when combined with bind_method...

[fire]

I realized there's an alternative away to do the array.

Let me know how well it works. It's a bit complicated... D:

ADD_PROPERTY(PropertyInfo(Variant::INT, "modification_count", PROPERTY_HINT_RANGE, "0, 100, 1", PROPERTY_USAGE_DEFAULT | PROPERTY_USAGE_ARRAY, "Modifications,modifications/"), "set_modification_count", "get_modification_count");

Becomes:

ADD_PROPERTY(PropertyInfo(Variant::INT, "extension_count", PROPERTY_HINT_RANGE, "0, 100, 1", PROPERTY_USAGE_DEFAULT | PROPERTY_USAGE_ARRAY, "Extensions,import_scene/extensions/"set_extension_count", "get_extension_count");

https://github.com/godotengine/godot/blob/master/scene/resources/skeleton_modification_stack_3d.cpp

[fire]

Can revise for a future pr.

[RedMser]

Why not register this globally?

Because GLTFDocumentExtension does not have a way to know what original file it's coming from. Best we have is state.scene_name which does not include the path, and may include a md5 hash for generated files.

I am not entirely sure what is the benefit of having it per scene.

For glTF extensions I agree, having those global would make a lot more sense. But the "extras" object is likely only relevant for specific assets only.

If you want some extra configuration per scene, you can add an importer plugin.

Do you mean EditorImportPlugin or EditorSceneFormatImporter (both are registered as "plugins" in the API)?

How should the user get access to GLTFDocument there before the import is done, without re-implementing e.g. the .blend import procedure by hand? I could imagine adding a virtual method to the new EditorSceneFormatImporterGLTFBase class for example, which allows hijacking the glTF initialization.

[fire]

GLTFDocumentExtension does not have a way to know what original file it's coming from.

The file can be a runtime glb loaded from a in-memory buffer. So there is no file!

[RedMser]

I have updated the approach with reduz's feedback. I also updated the MRP for testing above.

• Instead of an import option, create a virtual method EditorSceneFormatImporterGLTFBase::_initialize_gltf which does nothing by default.
• User can override it in a custom importer to (conditionally) add document extensions or do other things.

This allows for both global import behavior changes, or to check file path beforehand.

Sample code:

@tool
extends EditorSceneFormatImporterGLTF

func _initialize_gltf(gltf_path, document, state):
	document.extensions.append(load(MyDocumentExtension.new())

A simple EditorPlugin that uses add_scene_format_importer_plugin(MyPlugin.new(), true) is needed to register this (the true is necessary, otherwise the default gltf importer is executed first!).

[RedMser]

Does not seem possible to make a method (_import_scene) overridable in gdscript, while also calling the C++ method...

So I added an argument to retrieve the original file path (e.g. .blend path) to _initialize_gltf so you can do conditional checks on it.

Means that for now you can't really do any post-processing logic after the scene import is done (besides using import scripts like before).

[fire]

A simple EditorPlugin that uses add_scene_format_importer_plugin(MyPlugin.new(), true) is needed to register this (the true is necessary, otherwise the default gltf importer is executed first!).

Does this mean we have to make a plug per type of gltf and we'll can only make one of this per extension?

[RedMser]

@fire You have to create one editor plugin (in the addons folder) that looks basically like this:

@tool
extends EditorPlugin

const MyImporter = preload("res://addons/...")
var importer

func _enter_tree():
	importer = MyImporter.new()
	add_scene_format_importer_plugin(importer, true)

func _exit_tree():
	remove_scene_format_importer_plugin(importer)

but you can of course register multiple importer plugins in one editor plugin. The MRP zip has a simple example of how it is done.

Each importer plugin you add can change its logic freely based on scene file path (so it can add none, one, or multiple GLTFDocumentExtensions).
But you'll need to create a new importer plugin per scene file format (so extends EditorSceneFormatImporterGLTF, Blend, FBX etc. have to be separate plugins as far as I understand it - maybe you can re-use them idk).
Think it's ok since one project usually has one asset pipeline anyway.

[fire]

I am not sure what is blocking this. There's a rebase problem though.

[RedMser]

From my perspective it should be ready.

Fixed the conflict that came with 805ffdf , works now.

[RedMser]

@fire Is anything still needed to get this merged?

I updated the example project so the addon is a single .gd file now - see PR description for the code. It's a bit simpler than before, might be good for putting into a distant-future wiki page or something.

[fire]

Sorry, we was focusing on getting #62499 to be part of Godot 4 beta, and it is in a limboed reviewing state by @reduz , I think both touch the same areas, and I want both.

The maintainers have been working hard, https://github.com/godotengine/godot/pulse/monthly and I've been trying to get the two reviews required.

1. The first review is another maintainer.
2. The second review from @akien-mga or others for style and architecture.

[akien-mga]

Style wise it seems fine to me, and the refactoring seems good. Still need an OK from @reduz.

[fire]

@aaronfranke Please see if this is usable

[aaronfranke]

The design of this seems quite limited, because it requires you to create a class that extends the importer in order to add extensions. This means that you are limited to using one importer at a time that adds all of the extensions. This will not work well if you have multiple third-party extensions, you would need to build a master class importer that includes the document extensions. It would make more sense to me to allow independently registering document extensions:

# Create a plugin in project settings, then replace plugin.gd with this:
@tool
extends EditorPlugin

func _enter_tree():
	# Extensions can be provided anywhere without having to know about each other or
	# a master scene importer, so multiple extensions will not conflict with each other.
	GLTFDocument.register_gltf_document_extension(MyDocumentExtension.new())

func _exit_tree():
	remove_scene_format_importer_plugin(MyDocumentExtension)

class MyDocumentExtension extends GLTFDocumentExtension:
	func _import_node(state, gltf_node, json, node):
		prints("parent:", node.get_parent().name)
		prints("scene_name:", state.scene_name)
		prints("json:", json)
		return 0

# Create a plugin in project settings, then replace plugin.gd with this:
@tool
extends EditorPlugin

func _enter_tree():
	# Extensions can be provided anywhere without having to know about each other or
	# a master scene importer, so multiple extensions will not conflict with each other.
	GLTFDocument.register_gltf_document_extension(OtherDocumentExtension.new())

func _exit_tree():
	remove_scene_format_importer_plugin(OtherDocumentExtension)

class OtherDocumentExtension extends GLTFDocumentExtension:
	func _import_node(state, gltf_node, json, node):
		prints("parent:", node.get_parent().name)
		prints("scene_name:", state.scene_name)
		prints("json:", json)
		return 0

[RedMser]

@aaronfranke But then the use case for handling cases file-by-file is lost, since GLTFDocumentExtension does not have a file path association (iirc GLTFState only has the "generated" GLTF path, not the original file's). This is very important for handling the "extras" section (e.g. .blend file Custom Properties), which unlike "extensions" usually differs between the different assets.

[aaronfranke]

@RedMser That's a good point, although that's not my use case. I think maybe we should have both systems in place, a way to register GLTFDocumentExtensions for GLTF importers to use, and a way to specify custom importers per-file. In that case, I don't really know what to think of with your PR, because I simply don't need it, the use case I have is to specify an extension that is used for importing all GLTF files.

EDIT: After discussing with fire, I made a PR that will fit with our use cases: #66026

[fire]

#66026 fixes I and @aaronfranke' use cases.

[akien-mga]

Superseded by #66026.

[fire]

Let us know if you're able to solve your usecases as an mod to #66026. I am still interested.

[RedMser]

@fire Thanks, I checked the PR and it seems to be a good alternative. Some things I have noticed:

• I tried using it from an EditorPlugin and noticed you can't unregister gltfdocumentextensions. This is a problem when disabling and re-enabling the plugin. I added an unregister method in Add unregister for GLTFDocumentExtension #69022
• GLTFState.base_path only includes the folder path of the .gltf file, not the actual file name (so for example res:// is all I get when importing res://test.gltf). Is this a bug or an intentional change? I don't remember this happening before, but I can open an issue if wanted.

I will close my proposal since this seems to be a good alternative.

[fire]

Base path is used for the engine importer to find .bin and .png/.jpg. Like in the blender importer the gltf base path is in the .godot/importer/ folder for a res://example.gltf.

Path is “res://example.gltf”. Path doesn’t exist for buffers.

I’ll poke @aaronfranke about unregistering gltf document extensions.