User defined extensions

[jjcasmar]

This patch allows to create user defined extensions. This works by loading dynamically python files in the extensions directory. This way, a user can define his own extensions without the need to fork the exporter

[jjcasmar]

@julienduroure

[julienduroure]

@donmccurdy @emackey @scurest This PR is quite interesting, and will need lots of testing / code review. Any help is welcome :)

[scurest]

Weird whitespace.

[scurest]

Why does __gather_extras get changed only here?

[scurest]

Should be return extensions or None (not that I think it matters since empty dicts get erased at some point anyway, but for consistency...)

[jjcasmar]

This is an error. gether_extras should be unchanged.

[scurest]

Grammatical number seems to be inconsistent here. Some are singular (GLTF2ExtensionMesh, GLTF2ExtensionNode, etc.) and some are plural (GLTF2ExtensionCameras, GLTF2ExtensionMaterials, etc.)

[jjcasmar]

I using the same notation as the gltf2_blender_gather_.py files. There are some that are cameras and others that are mesh.

[scurest]

Node is the only difference then (GLTF2ExtensionNode but gltf2_blender_gather_nodes.py). I still think they should still have a consistent number though.

[scurest]

How does this work? What action/object is it getting? Because generally an animation will correspond to a collection of Blender actions.

[jjcasmar]

It works exactly the same way as in other exported element. The __gather_extensions in gltf2_blender_gather_animations receives a blender_action and a blender_object. I simply redirect those to the extension exporters.

[scurest]

Yeah, I see that you just use whatever the __gather_extensions function takes, but is that okay? Most of these are just returning None right now anyway so there's no guarantee their arguments are actually useful or sensible enough to become a public API.

For example, I just tested this and this gets called once for each (action,blender_object) pair in the animation, but only the value returned by the first call for each animation actually makes it into the extensions dict; the rest get dropped on the floor. I don't think that's how it should work.

[jjcasmar]

Well, if the __gather_extensions should receive another thing, the it's obvious it will be incorrect, but I'm not sure if its on the scope of this PR to fix what the __gather_extensions function should receive.
If you think its on the scope, we can try to fix it, but probably we will have to review all the other __gather_extensions methods.

[scurest]

The big thing I guess is backwards compatibility. This creates a public API the exporter would presumably be beholden to maintain, which seems like a big commitment.

[jjcasmar]

What can we do about the backward compatibility? Maybe use the Blender version number or the exporter version as some kind of extension API version? Not sure about this, but if there are no commitments at all about API stability, is hard (imposible) to expose this to the public.

[scurest]

Well, what's the use case for this? It's already pretty limited because you can only add data to the extensions object without changing the rest of the glTF.

I'm wondering if we could design something more like how extras already works. Ziflin showed something in #730 where they used a regular Blender add-on to "extend" the exporter with new custom properties. Could we do something like that?

[jjcasmar]

I dont understand what you mean when you say you can only add data to the extensions object. I have been able to reimplement KHR_materials_unlit and KHR_punctual_lights with this mechanism. You can add new extensions to the root extensions object and refer to them from other places in the glTF file, as proved by the KHR_punctual_lights reimplementation. Ill push it to this PR so you can see how it works

[scurest]

For example, you wouldn't be able to set the fallback behavior on a KHR_materials_unlit material with this would you? And doesn't knowledge about KHR_lights_punctual need to be integrated into the exporter anyway so it can emit the correction node? There'd be no way to do that from a user extension.

That's what I mean about only being able to add data to extensions dicts.

[jjcasmar]

The default fallback material is already exported. The current code exports the PBR information if the shader graph has a Principled BSDF node, no matter what the extension does. Then the extension add the extension data if the shader graph has a Background node.

For the lights you are right, its not possible to add the correction node from the extension exporter, as we can only add data to the extensions dictionaries.
How do you suggest to improve this? Im not sure if an extension should have impact outside of the extension dictionaries anyway. That means that an importer that doesn't support the extension may be importing something that is not useful and could potentially break the scene

A bit offopic, I'm looking at the current code for the exporter and wouldn't be better to add a global correction node at the beggining of the hierarchy instead of per camera/light?

[scurest]

The current code exports the PBR information if the shader graph has a Principled BSDF node, no matter what the extension does

But the PBR information depends on whether it should be unlit. For example, unlit materials should have a metallicFactor of 0 for fallback purposes, but the default metallicFactor is 1.

Right now if I import a KHR_materials_unlit material (in 2.8 where an Emission node is used for this because there is no Background node in 2.8) and export it, I get a material like this

{
    "emissiveFactor": [1, 1, 1],
    "emissiveTexture": { "index": xxx },
    "pbrMetallicRoughness": {}
}

A correct KHR_materials_unlit implementation should give something like this

{
    "pbrMetallicRoughness": {
        "baseColorTexture": { "index": xxx },
        "roughnessFactor": 0.9,
        "metallicFactor": 0.0
    },
    "extensions": { "KHR_materials_unlit": {} }
}

That requires more that just adding extensions data.

---

(The correction nodes are there to rotate the camera/light when you have a situation where glTF says it should point along one axis but in Blender it points along another axis. You can't do that with one rotation at the root.)

[jjcasmar]

I understand what you mean. Maybe we can do a two step mechanism? From, what I have seen, the gltf2 object is created using something like this:

    node = gltf2_io.Node(
        camera=__gather_camera(blender_object, export_settings),
        children=__gather_children(blender_object, blender_scene, export_settings),
        extensions=__gather_extensions(blender_object, export_settings),
        extras=__gather_extras(blender_object, export_settings),
        matrix=__gather_matrix(blender_object, export_settings),
        mesh=__gather_mesh(blender_object, export_settings),
        name=__gather_name(blender_object, export_settings),
        rotation=None,
        scale=None,
        skin=__gather_skin(blender_object, export_settings),
        translation=None,
        weights=__gather_weights(blender_object, export_settings)
    )

Maybe we can keep the first step as it is right now, returning an Extension on the __gather_extensions and then do a second step in which we pass the glTF2 object to the extension plugin to modify something if its necessary.

What do you think?

[scurest]

The first step seems unnecessary. If you're going to be given the glTF object to manipulate as you please you can just add anything you want to the extensions property then. At that point this would no longer be limited to glTF extensions since you could also do manipulations that don't touch the extensions prop. You'd basically have a bunch of hooks to modify the output of the __gather_XXX functions just before they finish?

It's difficult to see how well this would work in practice. I think any user extension would have to track changes to the exporter pretty heavily so that in practice they would end up highly coupled anyway. The two examples we just had show that it's not so easy to write a correct user extension.

Do you have an example of a user extension you actually want to use this for?

[jjcasmar]

Probably the user extension developers would need to keep track on exporter changes but I think there are some benefits:
First of all, if the developer needs to patch the exporter to add extensions, he is going to need to keep track on the changes anyway.
Second, patching the exporter and deploying a new exporter forces the users to disable/uninstall the official exporter and install the patched exporter. Not very user friendly in my opinion.

You are right that passing around the glTF object to the extension exporter opens the door to modifying the whole node. Patching the exporter doesn't fix this, a developer can add whatever he wants. Also, if to properly implement an extension you need to modify other properties than the extensions property, then its clear the developer needs access to the whole glTF object.

As use cases, at KDAB we have developed an internal extension to add tags to nodes. Basically its a new glTF array layers and a layer property on the node. I'm algo trying to draft something for supporting animating any floating point value property using fcurves. Of course its possible to implement these extensions, or any extension, just patching the exporter, but I think this has some inconvenients as already mentioned at the beginning.

[scurest]

Second, patching the exporter and deploying a new exporter forces the users to disable/uninstall the official exporter and install the patched exporter. Not very user friendly in my opinion.

Yes, and it would also let the user mix and match the user-extensions they want (if there are n user-extensions, you'd need 2^n - 1 patched exporters to represent every combination of installed user-extensions). Like I said above, I think it would be even better if we could find a way to use regular Blender addons for this. Then Blender would handle managing their lifecycles and their installation would be even more user friendly.

You are right that passing around the glTF object to the extension exporter opens the door to modifying the whole node

I wasn't saying this was bad, just that user-extensions aren't about glTF extensions anymore; they become general plugins for modifying bits of the glTF.

add tags to nodes

Tagging sounds like something you could basically use custom properties for though.

[jjcasmar]

How do you propose to let Blender managed this? If we add the user extensions as a new Blender addon, we wont be able to call them as hooks from the exporter, no?
Something that was in my roadmap is to let the user specify the folder where the user extensions python files are, but they will be still managed by the exporter itself, as they are with the current approach.

Tagging sounds like something you could basically use custom properties for though.

They are not just tags. The layers have some properties and can be shared between different nodes. They represent the layers in Qt3D.

[scurest]

I dunno, does something like this work?

# At beginning of each export
import sys
user_exts = {}
for addon_name in bpy.context.preferences.addons.keys():
    try:
        # Every user-extension would have a top-level class called Gltf2UserExt
        user_exts[addon_name] = sys.modules[addon_name].Gltf2UserExt()
    except Exception:
        pass

# Example hook
for user_ext in user_exts.values():
    try:
        user_ext.gather_node_hook(gltf_node, blender_object, export_settings)
    except Exception:
        pass

The user-extensions would need to make sure they wait until the Gltf2UserExt class is called into to import io_scene_gltf2 I think.

[jjcasmar]

Right now, if the user wants to enable/disable one extension, he just need to check a checkbox in the Extensions section of the exporter panel. Probably easy to do anyway with the approach you suggested.
You approach will allow to simplify code also, as I dont need all the code to load for the py files.

If I understand correctly, at each place where a gltf2 object is created, I will need to call the user hooks, similar to what Im doing now, right?

Thanks for the suggestion

[jjcasmar]

I have tried what you suggested, but there is an issue. The gltf2 addon is imported before the user extensions addons, so in sys.modules[addon_name], the module for the user_extension doesn't exist yet. Do you know how can I force the extensions to be imported before the gltf2 addon?

[jjcasmar]

I have added the call to the hooks for the rest of gltf objects that have a __gather_extension function

[julienduroure]

So ... Being quite far from this development. Can you please confirm that this dev is stable and can be merged?

[emackey]

@jjcasmar Thanks so much for this contribution. I've wanted this feature for some time, but I haven't really been able to sit and review this thoroughly until this morning.

My initial impression is this appears very stable and works well. I do have a small concern on the UI. When there are no custom extensions enabled, Blender still puts an extra Extensions dropdown on the glTF export file dialog. Most users will see this as empty, so expanding the dropdown doesn't do anything but spin a triangle. Is there any way to remove that? (I'm testing with Blender 2.81a. No need for compatibility with 2.80).

I also tried going into user preferences and toggling the glTF addon itself off, and after a moment, back on. This ensured that the glTF addon "woke up" with a custom extension already previously active. When I did this, the custom settings appeared below (outside of) the Extensions dropdown on the UI. This also resulted in an expandable dropdown being empty.

I wonder if it would be better to just get rid of the Extensions dropdown on the UI, and always have custom extension options simply appear underneath, separately. It seems like that could save some concern over which addon is activated first. Just a suggestion.

Great work on this.

[jjcasmar]

@emackey thanks for your comments.

I will review the UI stuff and submit any needed changes.

[jjcasmar]

@emackey The latest version should fix the issues you have found. Now I only show the extension panel if there is some extension to be shown and I unregister the extension panels when the exporter is disabled. That way, when the exporter is enabled again, there are no panels preregistered.

CI is failing though. Apparently, is not being able to download blender?

[emackey]

Should the extension_panel_unregister_functors list be cleared after this? Or Blender always makes a fresh copy of the whole addon when re-registering?

[jjcasmar]

I can clear the list, but anyway, since the function runs in a try/catch scope, there is no problem is there is a dangling function trying to remove an already removed panel.

[emackey]

I tried to manually re-run the CI but I think it only ran 1 of the 4 containers. Can you push a new commit to trigger the whole thing? I think it should be OK now...

[emackey]

This feels solid, and I like the UI fixes. Let's merge this once the tests are green.

@donmccurdy Did you want us to wait for your review?

[julienduroure]

These is in #829 a discussion about required / used extensions: how this PR manage it? (Sorry, don't have the time to test for now)

[emackey]

These is in #829 a discussion about required / used extensions: how this PR manage it? (Sorry, don't have the time to test for now)

This PR doesn't need to do anything about #829. That one is a bugfix for a flag that's not being respected.

[emackey]

FloatProperty

[julienduroure]

My question was (sorry, was not really clear):
With this new way of extension creation at export, is there a way to define these new extensions as required or used?
Can't see any info about that in misc/ExampleExtension.py file

[emackey]

@julienduroure Yes, good question, I just tested this and it works.

in misc/ExampleExtension.py around line 101, set the required parameter:

            gltf2_object.extensions[bl_info['name']] = self.Extension(
                name= bl_info['name'], 
                extension={"float": self.properties.float_property},
                required=False
            )

That might be a good thing to add to this example, now that the bug is fixed in master.

The default is True (required), and the flag is only respected if #829 or master is merged into this branch.

[julienduroure]

Thanks for your explanation.
Agree that this should be added to the example. This will not replace the documentation (that still need to be written ?), but it can lead to understand/find this parameter easier.

[jjcasmar]

I will add that flag in the ExampleExtension.

@julienduroure where/how should I write the documentation?

[julienduroure]

@jjcasmar The documentation is here: docs/blender_docs/io_scene_gltf2.rst
This file is committed in official blender documentation here : https://docs.blender.org/manual/en/latest/

[donmccurdy]

It would be a good to see an example or two of real vendor extensions implemented on this API before we consider the API stable — I expect that if we merge it now, there could be breaking changes before we get to that point. If that's OK and matches everyone's expectations, I'm fine with merging the current PR any time.

The documentation is here: docs/blender_docs/io_scene_gltf2.rst
This file is committed in official blender documentation here : https://docs.blender.org/manual/en/latest/ ...

Perhaps just put a brief section mentioning the existence of this API in the public Blender documentation, and link to a more detailed markdown API doc for the feature in this repository? Both can be a separate PR. I don't think we'd want to spend a lot of words on Python APIs in the otherwise UI-focused official addon documentation.

[jjcasmar]

I can provide a KDAB extension if necesary. I will do this last changes ASAP

[julienduroure]

Hello,
I am going to merge it, because bcon2 will start on Monday, so this is the last days to merge new big feature into Blender repository. (see #801 for more info about that).
Polishing can be done on another PR

[emackey]

Thanks again @jjcasmar, this is an important one.

[jjcasmar]

Thanks @emackey . This patch is very useful for KDAB.