USD export: prototype invoking Python chasers #108823

Merged
Michael Kowalski merged 35 commits from makowalski/blender:test-usd-export-chaser into main 2023-08-07 23:02:54 +02:00

This branch implements an experimental prototype for invoking Python
chaser or hook functions to extend the USD export functionality.
This code is intended to be a limited proof of concept, as part of a
design discussion for implementing Python hooks for USD IO in
#108825.

Added support for registering subclasses of a new bpy.types.USDHook
type which may implement the hooks as member functions. Supported
hook functions are on_export() and on_material_export(). Also added
definitions and Python registration for USDSceneExportContext and
USDMaterialExportContext structs that encapsulate arguments
to these functions.

The hook functions should return True on success or False if the
operation was bypasssed or otherwise failed to complete. Exceptions raised
by these functions will be reported in Blender, with the exception details printed
to the console.

On Scene Export

Immediately before saving the USD stage, the C++ export code invokes the
on_export() Python function defined on all registered USDHook instances.
This function is called with an export_context argument of type USDSceneExportContext
which provides the following accessors to the scene data:

  • export_context.get_stage(): returns the USD stage to be saved.
  • export_context.get_depsgraph(): returns the Blender scene dependency graph.

On Material Export

The on_material_export() hook function is called for each material that is exported,
allowing updates or modifications to the USD material (e.g., shader generation).
The hook is passed three arguments:

  • export_context: An instance of the internally defined type USDMaterialExportContext,
    providing the function export_context.get_stage() which returns the stage to be saved.
  • bl_material: The source Blender material.
  • usd_material: The target USD material to be exported.

Note that the target USD material might already have connected shaders created by the USD exporter or
by other material export hooks.

Python Bindings

This implementation relies on the boost::python bindings for USD types
provided by the USD library. Therefore, the boost::python API is used
to invoke the Python hooks instead of the function call mechanism provided
by RNA.

Example Add-On

To test, install the attached usd_hook_test add-on. This add-on registers
a USDHookTest class which implements an on_export() function to add
custom data to the stage's root layer and an on_material_export() function
to create a simple MaterialX shader on the USD material.

class USDHookTest(bpy.types.USDHook):
    bl_idname = "usd_hook_test"
    bl_label = "Test"
    bl_description = "Test implementation of USD IO hooks"

    @staticmethod
    def on_export(export_context):
        """ Include the Blender filepath in the root layer custom data.
        Arguments:
            export_context: Provides a get_stage() function which returns the UsdStage to
                            be exported.
        """

        stage = export_context.get_stage()

        if stage is None:
            return False
        data = bpy.data
        if data is None:
            return False

        # Set the custom data.
        rootLayer = stage.GetRootLayer()
        customData = rootLayer.customLayerData
        customData["blenderFilepath"] = data.filepath
        rootLayer.customLayerData = customData

        return True

    @staticmethod
    def on_material_export(export_context, bl_material, usd_material):
        """ Create a simple MaterialX shader on the exported material.
        Arguments:
            export_context: Provides a get_stage() function which returns the UsdStage to
                            be exported.
            bl_material: The source Blender material.
            usd_material: The target USD material to be exported.  Note that this material
                          might already have connected shaders created by the USD exporter
                          or by other material export hooks.
        """

        stage = export_context.get_stage()

        # Create a MaterialX standard surface shader
        mtl_path = usd_material.GetPrim().GetPath()
        shader = UsdShade.Shader.Define(stage, mtl_path.AppendPath("mtlxstandard_surface"))
        shader.CreateIdAttr("ND_standard_surface_surfaceshader")

        # Connect the shader.  MaterialX materials use "mtlx" renderContext
        usd_material.CreateSurfaceOutput("mtlx").ConnectToSource(shader.ConnectableAPI(), "out")

        # Set the color to the Blender material's viewport display color.
        col = bl_material.diffuse_color
        shader.CreateInput("base_color", Sdf.ValueTypeNames.Color3f).Set(Gf.Vec3f(col[0], col[1], col[2]))

        return True


def register():
  bpy.utils.register_class(USDHookTest)

def unregister():
  bpy.utils.unregister_class(USDHookTest)

When exporting to USDA, the root layer custom data will be visible at the top of the USDA file:

#usda 1.0
(
    customLayerData = {
        string blenderFilepath = "C:\\test\\test.blend"
    }
    defaultPrim = "Cube"
    doc = "Blender v4.0.0 Alpha"
    metersPerUnit = 1
    upAxis = "Z"
)

Note that the blenderFilepath value will be empty if the Blender file was not previously saved.

The MaterialX shader added by the hook will be visible under the Material, with the color set to
the material's viewport color, similar to the following:

        def Shader "mtlxstandard_surface"
        {
            uniform token info:id = "ND_standard_surface_surfaceshader"
            color3f inputs:base_color = (0.8, 0.5, 0.1)
            token outputs:out
        }
This branch implements an experimental prototype for invoking Python `chaser` or `hook` functions to extend the USD export functionality. This code is intended to be a limited proof of concept, as part of a design discussion for implementing Python hooks for USD IO in https://projects.blender.org/blender/blender/issues/108825. Added support for registering subclasses of a new `bpy.types.USDHook` type which may implement the hooks as member functions. Supported hook functions are `on_export()` and `on_material_export()`. Also added definitions and Python registration for `USDSceneExportContext` and `USDMaterialExportContext` structs that encapsulate arguments to these functions. The hook functions should return ``True`` on success or ``False`` if the operation was bypasssed or otherwise failed to complete. Exceptions raised by these functions will be reported in Blender, with the exception details printed to the console. ### On Scene Export Immediately before saving the USD stage, the C++ export code invokes the `on_export()` Python function defined on all registered `USDHook` instances. This function is called with an `export_context` argument of type `USDSceneExportContext` which provides the following accessors to the scene data: - `export_context.get_stage()`: returns the USD stage to be saved. - `export_context.get_depsgraph()`: returns the Blender scene dependency graph. ### On Material Export The `on_material_export()` hook function is called for each material that is exported, allowing updates or modifications to the USD material (e.g., shader generation). The hook is passed three arguments: - `export_context`: An instance of the internally defined type `USDMaterialExportContext`, providing the function `export_context.get_stage()` which returns the stage to be saved. - `bl_material`: The source Blender material. - `usd_material`: The target USD material to be exported. Note that the target USD material might already have connected shaders created by the USD exporter or by other material export hooks. ### Python Bindings This implementation relies on the `boost::python` bindings for USD types provided by the USD library. Therefore, the `boost::python` API is used to invoke the Python hooks instead of the function call mechanism provided by RNA. ### Example Add-On To test, install the attached `usd_hook_test` add-on. This add-on registers a `USDHookTest` class which implements an `on_export()` function to add custom data to the stage's root layer and an `on_material_export()` function to create a simple ``MaterialX`` shader on the USD material. ``` class USDHookTest(bpy.types.USDHook): bl_idname = "usd_hook_test" bl_label = "Test" bl_description = "Test implementation of USD IO hooks" @staticmethod def on_export(export_context): """ Include the Blender filepath in the root layer custom data. Arguments: export_context: Provides a get_stage() function which returns the UsdStage to be exported. """ stage = export_context.get_stage() if stage is None: return False data = bpy.data if data is None: return False # Set the custom data. rootLayer = stage.GetRootLayer() customData = rootLayer.customLayerData customData["blenderFilepath"] = data.filepath rootLayer.customLayerData = customData return True @staticmethod def on_material_export(export_context, bl_material, usd_material): """ Create a simple MaterialX shader on the exported material. Arguments: export_context: Provides a get_stage() function which returns the UsdStage to be exported. bl_material: The source Blender material. usd_material: The target USD material to be exported. Note that this material might already have connected shaders created by the USD exporter or by other material export hooks. """ stage = export_context.get_stage() # Create a MaterialX standard surface shader mtl_path = usd_material.GetPrim().GetPath() shader = UsdShade.Shader.Define(stage, mtl_path.AppendPath("mtlxstandard_surface")) shader.CreateIdAttr("ND_standard_surface_surfaceshader") # Connect the shader. MaterialX materials use "mtlx" renderContext usd_material.CreateSurfaceOutput("mtlx").ConnectToSource(shader.ConnectableAPI(), "out") # Set the color to the Blender material's viewport display color. col = bl_material.diffuse_color shader.CreateInput("base_color", Sdf.ValueTypeNames.Color3f).Set(Gf.Vec3f(col[0], col[1], col[2])) return True def register(): bpy.utils.register_class(USDHookTest) def unregister(): bpy.utils.unregister_class(USDHookTest) ``` When exporting to USDA, the root layer custom data will be visible at the top of the USDA file: ``` #usda 1.0 ( customLayerData = { string blenderFilepath = "C:\\test\\test.blend" } defaultPrim = "Cube" doc = "Blender v4.0.0 Alpha" metersPerUnit = 1 upAxis = "Z" ) ``` Note that the `blenderFilepath` value will be empty if the Blender file was not previously saved. The `MaterialX` shader added by the hook will be visible under the Material, with the color set to the material's viewport color, similar to the following: ``` def Shader "mtlxstandard_surface" { uniform token info:id = "ND_standard_surface_surfaceshader" color3f inputs:base_color = (0.8, 0.5, 0.1) token outputs:out } ```
Michael Kowalski added the
Interest
Pipeline, Assets & IO
Interest
USD
labels 2023-06-09 20:45:50 +02:00
Michael Kowalski added 1 commit 2023-06-09 20:46:02 +02:00
Implemented a simple protoype for invoking Python
"chaser" functions to extend the USD export functionality.
This is intended as a limited proof of concept as part of
the design discussion for implementing Python hooks for USD IO.

Added a new "chasers" USD export parameter for providing a
comma-delimited list of Python modules that implement the chaser
functions.  Immediately before saving the USD stage, the export
code invokes a usd_on_export() Python function defined in the chaser
modules, providing the stage as an argument.
Michael Kowalski added 1 commit 2023-06-09 22:08:34 +02:00
Merge branch 'main' into test-usd-export-chaser
All checks were successful
buildbot/vexp-code-patch-coordinator Build done.
58b679584e
Author
Member

@blender-bot build

@blender-bot build
Author
Member

One important improvement to this example would be the ability to provide the Blender context or other Blender data as arguments to the chaser, in addition to the stage. E.g., it would be nice to support a function definition like

def usd_on_export(stage, context)

Of course, users can currently work around this by accessing the context directly through the bpy module, E.g.:

import bpy
scene = bpy.context.scene

However, it would be much cleaner to provide the Blender data as arguments directly.

I'm currently looking into how to invoke Blender's Python bindings in C++, for example, to get the PyObject representation of the context.

@ideasman42 Is there any example code I could look at for doing something like this, specifically to convert a bContext* to a PyObject*? I see there is an interesting CTX_py_dict_get() which suggests it might do something like this, but this appears to return null by default.

One important improvement to this example would be the ability to provide the Blender context or other Blender data as arguments to the chaser, in addition to the stage. E.g., it would be nice to support a function definition like `def usd_on_export(stage, context)` Of course, users can currently work around this by accessing the context directly through the `bpy` module, E.g.: ``` import bpy scene = bpy.context.scene ``` However, it would be much cleaner to provide the Blender data as arguments directly. I'm currently looking into how to invoke Blender's Python bindings in C++, for example, to get the `PyObject` representation of the context. @ideasman42 Is there any example code I could look at for doing something like this, specifically to convert a `bContext*` to a `PyObject*`? I see there is an interesting `CTX_py_dict_get()` which suggests it might do something like this, but this appears to return null by default.

One way to do implement this would be uses the RNA subclassing mechanism that we for extending Blender in general. The C side implementation is a bit arcane, but you should be able to get something working by copying a simple case like KeyingSetInfo. The way that works actually does not require dealing with PyObject types directly, it's all abstracted by RNA.

That way you could have something like this in Python:

class USDMaterialXChaser(USDChaser):
    bl_idname = "usd_chaser_materialx"
    bl_label = "MaterialX"
    bl_default_enabled = True

    def on_export(depsgraph, stage):
        ...

def register():
  bpy.utils.register_class(USDMaterialXChaser)

def unregister():
  bpy.types.unregister_class(USDMaterialXChaser)

I would also try to avoid passing context when possible, since that comes with things like a a specific window or editor, or a scene that the user may be editing, while in general USD export should be able to run isolated in a background job.

In terms of user interface, it's of course a bit obscure for a users to know what the name of the chaser is. Easiest could be to just enable all by default if that makes sense,?

And then if there is a need, the UI could have checkboxes to turn chasers on/off, and perhaps these could have a boolean indicating if they should be enabled by default.

The term "chaser" is also a bit obscure to me, I had not heard it before but see it is called like that in maya-usd too. Personally I would find "translator" or "hook" easier to guess its meaning.

One way to do implement this would be uses the RNA subclassing mechanism that we for extending Blender in general. The C side implementation is a bit arcane, but you should be able to get something working by copying a simple case like `KeyingSetInfo`. The way that works actually does not require dealing with `PyObject` types directly, it's all abstracted by RNA. That way you could have something like this in Python: ``` class USDMaterialXChaser(USDChaser): bl_idname = "usd_chaser_materialx" bl_label = "MaterialX" bl_default_enabled = True def on_export(depsgraph, stage): ... def register(): bpy.utils.register_class(USDMaterialXChaser) def unregister(): bpy.types.unregister_class(USDMaterialXChaser) ``` I would also try to avoid passing context when possible, since that comes with things like a a specific window or editor, or a scene that the user may be editing, while in general USD export should be able to run isolated in a background job. In terms of user interface, it's of course a bit obscure for a users to know what the name of the chaser is. Easiest could be to just enable all by default if that makes sense,? And then if there is a need, the UI could have checkboxes to turn chasers on/off, and perhaps these could have a boolean indicating if they should be enabled by default. The term "chaser" is also a bit obscure to me, I had not heard it before but see it is called like that in maya-usd too. Personally I would find "translator" or "hook" easier to guess its meaning.
Author
Member

Thank you, Brecht. This is very helpful. I'll investigate the approach you suggested. I won't pass the context as an argument, in that case. Providing as arguments the depsgraph and a dictionary that maps USD primitives to Blender objects would probably make sense, but this can be discussed further.

Thank you, Brecht. This is very helpful. I'll investigate the approach you suggested. I won't pass the context as an argument, in that case. Providing as arguments the depsgraph and a dictionary that maps USD primitives to Blender objects would probably make sense, but this can be discussed further.
Michael Kowalski added 2 commits 2023-06-19 15:12:36 +02:00
Added USDHook struct and its RNA definition to
represent a registerable class for extending
USD IO functionality.
Michael Kowalski added 1 commit 2023-06-19 15:26:08 +02:00
USD hook: remove unused function.
Some checks failed
buildbot/vexp-code-patch-coordinator Build done.
f5d285fbc6
Author
Member

@blender-bot build

@blender-bot build
Michael Kowalski added 1 commit 2023-06-19 16:15:37 +02:00
Fixed linux compile error.
All checks were successful
buildbot/vexp-code-patch-coordinator Build done.
f4a7f72627
Author
Member

@blender-bot build

@blender-bot build
Author
Member

@brecht Following your suggestion, I've implemented a new USDHook class, using KeyingSetInfo as an example. I'm fairly new to working with RNA, so doubtless I missed some things, but this appears to be working on a basic level. As mentioned in the updated description, this is a hybrid approach because I need to use the boost::python function call API, rather than the one provided by RNA, but maybe there is a more elegant way to solve this issue.

@brecht Following your suggestion, I've implemented a new `USDHook` class, using `KeyingSetInfo` as an example. I'm fairly new to working with `RNA`, so doubtless I missed some things, but this appears to be working on a basic level. As mentioned in the updated description, this is a hybrid approach because I need to use the `boost::python` function call API, rather than the one provided by `RNA`, but maybe there is a more elegant way to solve this issue.
Brecht Van Lommel requested changes 2023-06-19 18:03:45 +02:00
Brecht Van Lommel left a comment
Owner

If you need to pass around USD data structures or other custom data structures then it may indeed be required to call directly through the Python API. API documentation will be missing them, but I think that can be solved by adding those methods in bpy_types.py with a docstring.

If you need to pass around USD data structures or other custom data structures then it may indeed be required to call directly through the Python API. API documentation will be missing them, but I think that can be solved by adding those methods in `bpy_types.py` with a docstring.
@ -0,0 +17,4 @@
using namespace boost;
/* USD Hook Type declarations */
static ListBase usd_hook_types = {NULL, NULL};

This list should be freed on Blender exit.

This list should be freed on Blender exit.

You could use a std::list to make that automatic.

You could use a `std::list` to make that automatic.
Author
Member

I updated the code to use std::list, per your suggestion.

I updated the code to use `std::list`, per your suggestion.
makowalski marked this conversation as resolved
@ -152,0 +156,4 @@
typedef struct USDHook {
struct USDHook *next, *prev;
/* identifier used for class name, which KeyingSet instances reference as "Typeinfo Name" */

KeyingSet -> USDHook

KeyingSet -> USDHook
makowalski marked this conversation as resolved
@ -152,0 +158,4 @@
/* identifier used for class name, which KeyingSet instances reference as "Typeinfo Name" */
char idname[64];
/* identifier so that user can hook this up to a KeyingSet (used as label). */

KeyingSet -> USDHook

KeyingSet -> USDHook
makowalski marked this conversation as resolved
First-time contributor

In terms of user interface, it's of course a bit obscure for a users to know what the name of the chaser is. Easiest could be to just enable all by default if that makes sense,? And then if there is a need, the UI could have checkboxes to turn chasers on/off, and perhaps these could have a boolean indicating if they should be enabled by default.

I agree with the approach @brecht suggests here, where Hooks are opt out from the UI perspective. In general, if an add on has been enabled by the user, I think they'd implicitly expect it to just run by default.

> In terms of user interface, it's of course a bit obscure for a users to know what the name of the chaser is. Easiest could be to just enable all by default if that makes sense,? And then if there is a need, the UI could have checkboxes to turn chasers on/off, and perhaps these could have a boolean indicating if they should be enabled by default. I agree with the approach @brecht suggests here, where Hooks are opt out from the UI perspective. In general, if an add on has been enabled by the user, I think they'd implicitly expect it to just run by default.

An even more general solution would be to let hooks register and draw their own properties for the USD operators, with the simplest case being a boolean to turn themselves on or off. I don't think there is a convenient mechanism in Blender to do that now, but if there is really a need for it, it could be done.

An even more general solution would be to let hooks register and draw their own properties for the USD operators, with the simplest case being a boolean to turn themselves on or off. I don't think there is a convenient mechanism in Blender to do that now, but if there is really a need for it, it could be done.
Michael Kowalski added 2 commits 2023-06-22 00:37:50 +02:00
USD: Use std::list to store hooks.
All checks were successful
buildbot/vexp-code-patch-coordinator Build done.
652b9ad73a
Fixes per review comments:

Now storing registered USDHook instances in
an std::list, instead of a ListBase, so that
the list will be automatically destroyed on
Blender exit.

Also fixed incorrect comments.
Author
Member

Thanks for the review @brecht. I've made the requested changes.

Thanks for the review @brecht. I've made the requested changes.
Author
Member

@blender-bot build

@blender-bot build
Brecht Van Lommel requested changes 2023-06-22 15:48:20 +02:00
Brecht Van Lommel left a comment
Owner

Can you add a code example in doc/python_api/examples/bpy.types.USDHook.py?

Though I think we should perhaps only land this when the implementation is complete enough to support a practical use case,to validate that it works.

I'm also wondering if/how this will work for Hydra. If there is a method to specifically export one individual material, it's an easier fit because then you don't have to worry about how that is part of the scene, and it can work with incremental updates automatically.

Can you add a code example in `doc/python_api/examples/bpy.types.USDHook.py`? Though I think we should perhaps only land this when the implementation is complete enough to support a practical use case,to validate that it works. I'm also wondering if/how this will work for Hydra. If there is a method to specifically export one individual material, it's an easier fit because then you don't have to worry about how that is part of the scene, and it can work with incremental updates automatically.
@ -0,0 +127,4 @@
FunctionRNA *func;
PropertyRNA *parm;
srna = RNA_def_struct(brna, "USDHook", NULL);

Add with RNA_def_struct_ui_text with a description of what this is.

Add with `RNA_def_struct_ui_text` with a description of what this is.
Author
Member

I completely agree that this shouldn't land until the implementation is more complete.

Indeed, I agree that supporting hooks for exporting individual materials is essential (this is our use case when converting to MDL in our local branch). I will update this PR with a prototype implementation of material export as well.

I completely agree that this shouldn't land until the implementation is more complete. Indeed, I agree that supporting hooks for exporting individual materials is essential (this is our use case when converting to MDL in our local branch). I will update this PR with a prototype implementation of material export as well.
makowalski marked this conversation as resolved
@ -0,0 +151,4 @@
RNA_def_property_string_sdna(prop, NULL, "description");
RNA_def_property_string_maxlength(prop, RNA_DYN_DESCR_MAX); /* else it uses the pointer size! */
RNA_def_property_flag(prop, PROP_REGISTER_OPTIONAL);
RNA_def_property_ui_text(prop, "Description", "A short description of the keying set");

keying set -> USD hook

keying set -> USD hook
makowalski marked this conversation as resolved
Dhruv Govil reviewed 2023-06-23 03:15:25 +02:00
@ -0,0 +110,4 @@
python::call_method<void>(hook_obj,
func_name,
python::object(python::handle<>(depsgraph_obj)),
stage);
First-time contributor

Would it make sense to pass in a struct or object with the stage as a field instead of the stage directly?
I'm not sure how much work it would be, but it would have the advantage that when you add extra parameters, they could simply be added as fields on the struct/object.

Maya does something similar where chasers are passed a factoryContext , and you do factoryContext.GetStage() to get the stage.

The reason I suggest that is because if in the future we add more arguments, it changes the function signature, thereby potentially breaking existing add-ons unless they also specify *args, **kwargs to capture unknowns. Whereas if they simply accept a singular object as the argument, then new parameters can be passed in on that object, keeping the signature of the function intact.

Would it make sense to pass in a struct or object with the stage as a field instead of the stage directly? I'm not sure how much work it would be, but it would have the advantage that when you add extra parameters, they could simply be added as fields on the struct/object. Maya does something similar where chasers are passed a `factoryContext` , and you do `factoryContext.GetStage()` to get the stage. The reason I suggest that is because if in the future we add more arguments, it changes the function signature, thereby potentially breaking existing add-ons unless they also specify `*args, **kwargs` to capture unknowns. Whereas if they simply accept a singular object as the argument, then new parameters can be passed in on that object, keeping the signature of the function intact.
Author
Member

Thanks for the suggestion, Dhruv. What you propose definitely makes sense. I'm guessing falling back on *args, **kwargs for arbitrary arguments would be the least amount of work, but a custom struct would be a much cleaner API. We can discuss further. I'm currently finishing up the changes requested by Brecht as well as a simple example of material conversion. I should be done with this in the next couple of days. In the meantime, I'll give some thought to this issue.

Thanks for the suggestion, Dhruv. What you propose definitely makes sense. I'm guessing falling back on `*args, **kwargs` for arbitrary arguments would be the least amount of work, but a custom struct would be a much cleaner API. We can discuss further. I'm currently finishing up the changes requested by Brecht as well as a simple example of material conversion. I should be done with this in the next couple of days. In the meantime, I'll give some thought to this issue.
Author
Member

Dhruv, I've determined that providing a struct as an argument isn't much extra work, so I'll update the prototype to use this approach in the next day or so. Thanks, again, for suggesting this.

Dhruv, I've determined that providing a struct as an argument isn't much extra work, so I'll update the prototype to use this approach in the next day or so. Thanks, again, for suggesting this.
First-time contributor

That's great to hear, and thank you for doing that. I think it'll help long term with API stability.

That's great to hear, and thank you for doing that. I think it'll help long term with API stability.
First-time contributor

That's great to hear, and thank you for doing that. I think it'll help long term with API stability.

That's great to hear, and thank you for doing that. I think it'll help long term with API stability.
makowalski marked this conversation as resolved
First-time contributor

Just leaving a general note:I think the approach and add-on API interface here looks really good to me.
Nothing in the code implementation stood out as needing changes either other than the Struct thing mentioned above, and what Brecht mentioned.

I think this will be a really great change, and will help a lot of production pipelines.

Just leaving a general note:I think the approach and add-on API interface here looks really good to me. Nothing in the code implementation stood out as needing changes either other than the Struct thing mentioned above, and what Brecht mentioned. I think this will be a really great change, and will help a lot of production pipelines.
Michael Kowalski added 8 commits 2023-06-29 23:38:44 +02:00
Exposing a USDSceneExportContext class to Python
to encapsulate arguments for scene export which
is now provided as an argument to the on_export()
Python hook.
Added a USDMaterialExportContext class to be provided
as an argument when invoking the on_material_export()
hook function.  Now invoking this material export hook
when converting materials.

Also, unrelated change to avoid const cast in
PointerRNAToPython::convert().
Per review comments by Brecht, fixes to the USDHook
RNA definitions.
USD: hook example minor edits.
All checks were successful
buildbot/vexp-code-patch-coordinator Build done.
8f4b0d6451
Author
Member

@blender-bot build

@blender-bot build
Author
Member

@DhruvGovil @brecht Just FYI, I've updated the code with the suggested changes, that includes the following:

  • Arguments are now passed as structs.
  • Registration of converters for these structs happens in a register_export_hook_converters() function that must be called at runtime. Maybe there is a better way to do this.
  • I added an example material converter hook.

I think an open question is what other arguments does it make sense to expose in these structs. For example, for the scene on_export() callback, we could provide a mapping between USD prims and the Blender objects they were converted from, which I believe Maya does. One way to do this is to add a function to the struct to query the Blender object from a given USD path. But this might not be essential to add right away.

I agree with Brecht that it would be good to try out real-world use cases before merging this in. @DhruvGovil would you be interested in doing such a test? I.e., maybe try implementing a chaser with this system and see if it works for you and what might be missing? No worries if you don't have time to do this.

@DhruvGovil @brecht Just FYI, I've updated the code with the suggested changes, that includes the following: - Arguments are now passed as structs. - Registration of converters for these structs happens in a `register_export_hook_converters()` function that must be called at runtime. Maybe there is a better way to do this. - I added an example material converter hook. I think an open question is what other arguments does it make sense to expose in these structs. For example, for the scene `on_export()` callback, we could provide a mapping between USD prims and the Blender objects they were converted from, which I believe Maya does. One way to do this is to add a function to the struct to query the Blender object from a given USD path. But this might not be essential to add right away. I agree with Brecht that it would be good to try out real-world use cases before merging this in. @DhruvGovil would you be interested in doing such a test? I.e., maybe try implementing a chaser with this system and see if it works for you and what might be missing? No worries if you don't have time to do this.
First-time contributor

Yep I’m down for that. I’ll take some time tomorrow and next week to get this branch building locally and then create a little add on.

Yep I’m down for that. I’ll take some time tomorrow and next week to get this branch building locally and then create a little add on.

@BogdanNagirniak, @BrianSavery, do you think the Material export mechanism here will work for Hydra as well? So that the MaterialX add-on implements a USD Hook, and this can then affect both USD export and Hydra?

@BogdanNagirniak, @BrianSavery, do you think the Material export mechanism here will work for Hydra as well? So that the MaterialX add-on implements a USD Hook, and this can then affect both USD export and Hydra?
First-time contributor

Hey @makowalski , this looks to be working really well for my test cases.
I did have one suggestion though which is that there is currently no error handling.

I think it would be good if raised exceptions could error out the current export to prevent exports that seem like they finished but may be accidentally missing the results of a Hook if it errored out. It would also help during development of a Hook for TDs to be able to see how they're tripping up.

Hey @makowalski , this looks to be working really well for my test cases. I did have one suggestion though which is that there is currently no error handling. I think it would be good if raised exceptions could error out the current export to prevent exports that seem like they finished but may be accidentally missing the results of a Hook if it errored out. It would also help during development of a Hook for TDs to be able to see how they're tripping up.
Author
Member

I think it would be good if raised exceptions could error out the current export to prevent exports that seem like they finished but may be accidentally missing the results of a Hook if it errored out. It would also help during development of a Hook for TDs to be able to see how they're tripping up.

Thanks so much for testing, @DhruvGovil. That's an excellent point. I'll add the error handling, and I appreciate the suggestion.

> I think it would be good if raised exceptions could error out the current export to prevent exports that seem like they finished but may be accidentally missing the results of a Hook if it errored out. It would also help during development of a Hook for TDs to be able to see how they're tripping up. Thanks so much for testing, @DhruvGovil. That's an excellent point. I'll add the error handling, and I appreciate the suggestion.

@BogdanNagirniak, @BrianSavery, do you think the Material export mechanism here will work for Hydra as well? So that the MaterialX add-on implements a USD Hook, and this can then affect both USD export and Hydra?

Sure, the materialx add-on exports a materialx xml though, not sure if you want to convert that to USD Shade with export as well, but should be easy.

> @BogdanNagirniak, @BrianSavery, do you think the Material export mechanism here will work for Hydra as well? So that the MaterialX add-on implements a USD Hook, and this can then affect both USD export and Hydra? Sure, the materialx add-on exports a materialx xml though, not sure if you want to convert that to USD Shade with export as well, but should be easy.
Brecht Van Lommel requested changes 2023-07-05 19:13:14 +02:00
Brecht Van Lommel left a comment
Owner

This looks good to me now, I just have one nitpick (besides the error handling that was mentioned).

This looks good to me now, I just have one nitpick (besides the error handling that was mentioned).
@ -0,0 +24,4 @@
- ``get_stage()`` returns the USD stage to be saved.
- ``get_blender_material()`` returns the source Blender material being convered.
- ``get_usd_material()`` returns the target USD material to be exported.

I like the context for future proofing, but would still make the Blender and USD material arguments to on_material_export, to clarify when they are available.

I like the context for future proofing, but would still make the Blender and USD material arguments to `on_material_export`, to clarify when they are available.
Author
Member

Thanks for the suggestion. I see your point. To clarify, the arguments should be

on_material_export(blender_material, usd_material, export_context)

and remove the export_context.get_blender_material() and export_context.get_usd_material() accessors. Is that right?

Thanks for the suggestion. I see your point. To clarify, the arguments should be `on_material_export(blender_material, usd_material, export_context)` and remove the `export_context.get_blender_material()` and `export_context.get_usd_material()` accessors. Is that right?

Yes, that's what I meant.

Just maybe change the argument order, we generally put the context as first argument.

on_material_export(export_context, blender_material, usd_material)

Also now that I think about it, shouldn't these methods be marked with @staticmethod? Otherwise the first argument would be expected to be self.

Yes, that's what I meant. Just maybe change the argument order, we generally put the context as first argument. ``` on_material_export(export_context, blender_material, usd_material) ``` Also now that I think about it, shouldn't these methods be marked with `@staticmethod`? Otherwise the first argument would be expected to be `self`.
makowalski marked this conversation as resolved
Bastien Montagne added this to the USD project 2023-07-07 11:03:10 +02:00
Michael Kowalski added 1 commit 2023-07-16 23:37:41 +02:00
Michael Kowalski added 1 commit 2023-07-17 04:25:37 +02:00
Added logic to report USD hook call exceptions as
Blender errors.
Author
Member

I'm incrementally implementing the requested changes, but this work is still in progress. My apologies for the delay. I added improved handling for exceptions so far and will address Brecht's comments next. I'll provide an update when this is done.

I'm incrementally implementing the requested changes, but this work is still in progress. My apologies for the delay. I added improved handling for exceptions so far and will address Brecht's comments next. I'll provide an update when this is done.
Michael Kowalski added 1 commit 2023-07-25 23:36:07 +02:00
Michael Kowalski added 3 commits 2023-07-26 06:48:31 +02:00
The on_material_export() callback now takes the Blender
and USD materials as separate arguments.

Also, the hook functions are now staticmethods.
Michael Kowalski added 2 commits 2023-07-26 17:50:51 +02:00
USD hooks: format fixes.
Some checks failed
buildbot/vexp-code-patch-coordinator Build done.
f196544ba8
Author
Member

@blender-bot build

@blender-bot build
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR108823) when ready.
Brecht Van Lommel requested changes 2023-07-26 19:50:10 +02:00
Brecht Van Lommel left a comment
Owner

This is looking very close now, just one comment.

This is looking very close now, just one comment.
@ -0,0 +178,4 @@
python::object format_exception = traceback.attr("format_exception");
python::object formatted_list = format_exception(htype, hvalue, htraceback);
python::object formatted = python::str("\n").join(formatted_list);
std::string err_msg = python::extract<std::string>(formatted);

I'm unsure why there is custom code to print exceptions, I feel like this should either be trivial with the Python C API already, or there is some code to share with the rest of Blender.

Can you use PyC_Err_PrintWithFunc or perhaps directly PyErr_Print() to print the exception? For other cases we print the stacktrace to the console, I think we can do the same here.

I'm unsure why there is custom code to print exceptions, I feel like this should either be trivial with the Python C API already, or there is some code to share with the rest of Blender. Can you use `PyC_Err_PrintWithFunc` or perhaps directly `PyErr_Print()` to print the exception? For other cases we print the stacktrace to the console, I think we can do the same here.
Author
Member

Hi Brecht. I appreciate the comment. I was using PyErr_Print() originally, but was concerned the user might not notice the error in the console. The work here is to display the full exception error message text as a Blender error, so it would be more obvious. But I certainly see your point that this might be excessive. Perhaps a compromise might be to use PyErr_Print() to print the exception details and report a short, high level message in Blender. So, maybe line 183 can be:

WM_reportf( RPT_ERROR, "An exception occurred invoking USD hook '%s'. Please see the console for details", hook->name);

Would this be acceptable?

Hi Brecht. I appreciate the comment. I was using `PyErr_Print()` originally, but was concerned the user might not notice the error in the console. The work here is to display the full exception error message text as a Blender error, so it would be more obvious. But I certainly see your point that this might be excessive. Perhaps a compromise might be to use `PyErr_Print()` to print the exception details and report a short, high level message in Blender. So, maybe line 183 can be: `WM_reportf( RPT_ERROR, "An exception occurred invoking USD hook '%s'. Please see the console for details", hook->name);` Would this be acceptable?

Yes, that's fine and consistent with other Python error reporting I think.

Yes, that's fine and consistent with other Python error reporting I think.
makowalski marked this conversation as resolved
Michael Kowalski added 3 commits 2023-07-30 18:16:31 +02:00
Michael Kowalski added 3 commits 2023-07-30 20:00:02 +02:00
Now calling PyErr_Print() to print Python exception
details to the console.
USD hook: minor edit to error message.
All checks were successful
buildbot/vexp-code-patch-coordinator Build done.
fa6b9f75cc
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR108823) when ready.
Author
Member

I believe at this point all requested changes have been addressed, but please let me know if I've overlooked something.

A package of the build with the latest changes for additional testing is available here:

https://builder.blender.org/download/patch/PR108823

I believe at this point all requested changes have been addressed, but please let me know if I've overlooked something. A package of the build with the latest changes for additional testing is available here: https://builder.blender.org/download/patch/PR108823
Author
Member

@BrianSavery Just FYI, this pull request is now ready for testing, in case you want to try the material conversion hook. If you read the PR description and take a look at the attached usd_hook_test.zip example, that should provide the information you need, but please let me know if you have any questions or run into issues. No worries at all if you don't have time to try this.

As per the comment above, this patch build is available for convenience:

https://builder.blender.org/download/patch/PR108823

@BrianSavery Just FYI, this pull request is now ready for testing, in case you want to try the material conversion hook. If you read the PR description and take a look at the attached `usd_hook_test.zip` example, that should provide the information you need, but please let me know if you have any questions or run into issues. No worries at all if you don't have time to try this. As per the comment above, this patch build is available for convenience: https://builder.blender.org/download/patch/PR108823
Brecht Van Lommel approved these changes 2023-07-31 20:19:58 +02:00

I'll remove the WIP prefix since that means it's not ready for review, but I think this is.

I'll remove the WIP prefix since that means it's not ready for review, but I think this is.
Brecht Van Lommel changed title from WIP: USD export: prototype invoking Python chasers to USD export: prototype invoking Python chasers 2023-07-31 20:20:27 +02:00
Author
Member

Thank you, @brecht!

One remaining question: would it make sense for the hooks to return some sort of code or enum indicating status? For example, the functions could return one of 'Success', 'Bypass' (indicating a no-op) or 'Error' (for functions that choose not to throw an exception for errors).

In our local implementation of import hooks for materials, we found it useful to know when a material converter was a no-op, so that the USD importer could fall back on a default material conversion, but this may be less important for export.

I don't want to over-engineer this, and no sense adding this behavior if it won't be necessary. This could also be deferred and handled in the future by providing a function in the context struct for setting the status code, rather than returning a value. That way we don't necessarily need to make a decision about this now.

@DhruvGovil Do the chasers in Maya set or return some sort of status code? Do you have any opinion on this?

Thank you, @brecht! One remaining question: would it make sense for the hooks to return some sort of code or enum indicating status? For example, the functions could return one of 'Success', 'Bypass' (indicating a no-op) or 'Error' (for functions that choose not to throw an exception for errors). In our local implementation of import hooks for materials, we found it useful to know when a material converter was a no-op, so that the USD importer could fall back on a default material conversion, but this may be less important for export. I don't want to over-engineer this, and no sense adding this behavior if it won't be necessary. This could also be deferred and handled in the future by providing a function in the context struct for setting the status code, rather than returning a value. That way we don't necessarily need to make a decision about this now. @DhruvGovil Do the chasers in Maya set or return some sort of status code? Do you have any opinion on this?

I think it makes sense to have something like that. For operators in Blender there are these return values, not sure if we could just reuse those terms of it it's unclear:

{'CANCELLED'}          # Nothing was changed, swallow event
{'PASS_THROUGH'}       # Nothing was changed, pass on event
{'FINISHED'}           # Something was changed, swallow event

There is no error return value, but there is a self.reports for debug/info/warning/error messages. That could be provided here as well if there is a need for something more than exceptions.

I think it makes sense to have something like that. For operators in Blender there are these return values, not sure if we could just reuse those terms of it it's unclear: ``` {'CANCELLED'} # Nothing was changed, swallow event {'PASS_THROUGH'} # Nothing was changed, pass on event {'FINISHED'} # Something was changed, swallow event ``` There is no error return value, but there is a `self.reports` for debug/info/warning/error messages. That could be provided here as well if there is a need for something more than exceptions.

Simplest would be to just return True or False depending if it should block or bypass, and then use exceptions and reports for errors.

Probably that is good enough, and we can added more complicated return values without breaking the API later.

Simplest would be to just return True or False depending if it should block or bypass, and then use exceptions and reports for errors. Probably that is good enough, and we can added more complicated return values without breaking the API later.
Author
Member

Simplest would be to just return True or False depending if it should block or bypass, and then use exceptions and reports for errors.

Probably that is good enough, and we can added more complicated return values without breaking the API later.

That sounds good. I will go ahead and implement returning True or False for this PR.

> Simplest would be to just return True or False depending if it should block or bypass, and then use exceptions and reports for errors. > > Probably that is good enough, and we can added more complicated return values without breaking the API later. That sounds good. I will go ahead and implement returning True or False for this PR.

Hi @makowalski.

Could you please merge this with latest main? I would like to test export blender materials to MaterialX for Hydra scene delegate.

Hi @makowalski. Could you please merge this with latest `main`? I would like to test export blender materials to MaterialX for Hydra scene delegate.
Author
Member

Hi @makowalski.

Could you please merge this with latest main? I would like to test export blender materials to MaterialX for Hydra scene delegate.

I need to make one more change to this pull request today (return boolean from chaser). I will let you know when this is done.

> Hi @makowalski. > > Could you please merge this with latest `main`? I would like to test export blender materials to MaterialX for Hydra scene delegate. I need to make one more change to this pull request today (return boolean from chaser). I will let you know when this is done.

I just committed #110836, which adds USD preview surface support for Hydra.

If call_material_export_hooks is moved into create_usd_material in usd_writer_material.c, it will be called for both USD and Hydra. Maybe that will make material hooks just work for Hydra, maybe more work is needed.

I just committed #110836, which adds USD preview surface support for Hydra. If `call_material_export_hooks` is moved into `create_usd_material` in `usd_writer_material.c`, it will be called for both USD and Hydra. Maybe that will make material hooks just work for Hydra, maybe more work is needed.
Michael Kowalski added 3 commits 2023-08-07 19:29:47 +02:00
Now calling material export hooks from create_usd_material(),
to be consistent with the latest USD material export code
refactoring.
Michael Kowalski added 2 commits 2023-08-07 20:35:48 +02:00
Now expecting boolean return value when invoking
the Python hooks.
Fixed return value in USD hook example.
All checks were successful
buildbot/vexp-code-patch-coordinator Build done.
54ee7c863b
Author
Member

@brecht call_material_export_hooks() is now called in create_usd_material(). Also, the hooks are now expected to return booleans, as previously discussed. Note that the return values aren't actually used in the code at the moment, but supporting such return values in the API is future-proofing in case we need them at some point.

Please have a final look and if the changes are okay I can merge this in. Thanks!

@brecht `call_material_export_hooks()` is now called in `create_usd_material()`. Also, the hooks are now expected to return booleans, as previously discussed. Note that the return values aren't actually used in the code at the moment, but supporting such return values in the API is future-proofing in case we need them at some point. Please have a final look and if the changes are okay I can merge this in. Thanks!
Brecht Van Lommel approved these changes 2023-08-07 20:57:36 +02:00
Brecht Van Lommel left a comment
Owner

Looks good, thanks.

Looks good, thanks.
Author
Member

@blender-bot build

@blender-bot build
Michael Kowalski merged commit ecb3262bf0 into main 2023-08-07 23:02:54 +02:00
Michael Kowalski deleted branch test-usd-export-chaser 2023-08-07 23:02:55 +02:00
Author
Member

Hi @makowalski.

Could you please merge this with latest main? I would like to test export blender materials to MaterialX for Hydra scene delegate.

@DagerD This is now merged and ready for you to test.

> Hi @makowalski. > > Could you please merge this with latest `main`? I would like to test export blender materials to MaterialX for Hydra scene delegate. @DagerD This is now merged and ready for you to test.
Bastien Montagne removed this from the USD project 2023-08-25 23:54:38 +02:00
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
6 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#108823
No description provided.