Apple : UsdMtlx MaterialXConfigAPI schema and MaterialX versioning #3157

ld-kerley · 2024-07-09T01:07:04Z

Description of Change(s)

Initial pass at adding MaterialXInfoAPI applied schema, and then using this in UsdMtlx file format reader to append the version to the UsdShadeMaterial prims created.

HdMtlx also updated to explicitly write the MaterialX version in to the document that is reconstructed.

Outstanding: I need some input on the best way to get to the MaterialX version attribute on the UsdShadeMaterial prim from inside HdMtlx. I've left a hardcoded version string in the code so far at the place I think I need it.

Fixes Issue(s)

I have verified that all unit tests pass with the proposed changes

I have submitted a signed Contributor License Agreement

jesschimein · 2024-07-09T17:06:08Z

Filed as internal issue #USD-9835

❗ Please make sure that a signed CLA has been submitted!

jesschimein · 2024-07-09T17:06:09Z

/AzurePipelines run

azure-pipelines · 2024-07-09T17:06:20Z

Azure Pipelines successfully started running 1 pipeline(s).

crydalch · 2024-07-23T00:56:25Z

pxr/usd/usdMtlx/materialXInfoAPI.h

+    /// | C++ Type | std::string |
+    /// | \ref Usd_Datatypes "Usd Type" | SdfValueTypeNames->String |
+    USDMTLX_API
+    UsdAttribute GetMtlxinfoVersionAttr() const;


Style question: should "info" and "x" be capitalized in the function @ld-kerley ? (i.e. GetMtlXInfoVersionAttr())

Following other MaterialX use in USD I think it should be Mtlx, so lower case x. But I think you're right about the Info part - thanks - I'll make sure that gets changed next time I push something here.

Cool that's great. Thank you for getting this PR up and going!

crydalch · 2024-07-23T00:57:14Z

pxr/imaging/hdMtlx/hdMtlx.cpp

@@ -562,6 +562,12 @@ HdMtlxCreateMtlxDocumentFromHdMaterialNetworkInterface(

    // Initialize a MaterialX Document
    mx::DocumentPtr mxDoc = mx::createDocument();
+
+    // Need to figure out how to read the actual value from the netInterface
+    std::string materialXVersionString = "1.37";


@ld-kerley , should this default value be 1.38 like the schema has? Or maybe it's just a temporary/filler value that doesn't matter?

Yes - this is place holder - I'm hoping that someone with a bit more intimate hydra understanding can guide me as to the right way to get the real MaterialX version string from USD here.

tcauchois

Thanks for putting this together! Left some thoughts on the rest, but for the question of how to get the version string through I think we need to discuss our options :). But we're thinking about it, and will get back to you with ideas!

tcauchois · 2024-07-23T19:29:01Z

pxr/usd/usdMtlx/api.h

+//    and its affiliates, except as required to comply with Section 4(c) of
+//    the License and to reproduce the content of the NOTICE file.
+//
+// You may obtain a copy of the Apache License at


Note that we recently updated our license text, so you'll want to go with the top-of-tree license text. If you're running usdSchemaGen with an old USD version (< 24.08) that might put the wrong text in.

tcauchois · 2024-07-23T19:30:56Z

pxr/usd/usdMtlx/reader.cpp

@@ -1458,9 +1460,14 @@ UsdShadeMaterial
 _Context::BeginMaterial(const mx::ConstNodePtr& mtlxMaterial)
 {
    if (TF_VERIFY(!_usdMaterial)) {
+        auto mtlxVersionValue = VtValue(mtlxMaterial->getDocument()->getVersionString());


I'm guessing this will return one of "" and "1.39" at the moment? Do we want to handle the fallback case and put 1.38 in instead here? Or does the document function do that already?

tcauchois · 2024-07-23T19:34:24Z

pxr/imaging/hdMtlx/hdMtlx.cpp

@@ -585,6 +591,23 @@ HdMtlxCreateMtlxDocumentFromHdMaterialNetworkInterface(
        mxType,
        mxShaderNode);

+    // just for testing...
+    bool localDebug = false;


There's a system in hydra called debug codes that would be perfect here. The idea is that you create a local "debugCodes.h/.cpp", like the ones in imaging/hd (for example); add a new debug code, such as HDMTLX_VERSION_UPGRADE; and here, replace "localDebug" with TfDebug::IsEnabled(HDMTLX_VERSION_UPGRADE).

You can trigger this either with the debug codes editor in usdview, or by setting the environment variable "TF_DEBUG" to "HDMTLX_VERSION_UPGRADE".

Having this as a debug code seems like a fantastic idea!

tcauchois · 2024-07-23T23:23:33Z

pxr/imaging/hdMtlx/hdMtlx.cpp

@@ -562,6 +562,12 @@ HdMtlxCreateMtlxDocumentFromHdMaterialNetworkInterface(

    // Initialize a MaterialX Document
    mx::DocumentPtr mxDoc = mx::createDocument();
+
+    // Need to figure out how to read the actual value from the netInterface


A thought that @spiffmon had:

1.) Rename the property info:mtlx:version or something (such that it starts with "info:").
2.) Update materialAdapter.cpp (a) and dataSourceMaterial.cpp (b) to grab all attributes with the "info:" namespace prefix, and insert them into a new VtDictionary "info" on HdMaterialNetwork (a)/a new HdContainerDataSource named "info" on the material data source (b).
3.) Update dataSourceLegacyPrim.cpp and sceneIndexAdapterSceneDelegate.cpp to translate from the VtDictionary to the ContainerDataSource and back.
4.) Finally grab the version string off the material network here.

How does that feel, design wise/difficulty wise?

ld-kerley · 2024-08-30T01:00:15Z

@tcauchois - I took a stab at what you suggested. I think it's functional, but likely there are style things that could be tidied up, but before going too far down that road I wanted to share where I'm at for feedback, and make sure this is along the right lines.

tcauchois · 2024-08-30T18:15:55Z

pxr/usdImaging/usdImaging/materialAdapter.cpp

@@ -379,6 +379,19 @@ UsdImagingMaterialAdapter::GetMaterialResource(UsdPrim const &prim,
            time);
    }

+    // finally collect any 'info' on the Material prim
+    VtDictionary infoDict;


(1) Do we only want to include authored attributes, or are we expecting the fallback of "1.38" to ever be useful? This won't pull fallbacks, but GetAttributes would(). Maybe moot since usdMtlx/reader.cpp looks to be manually creating the attribute.

(2) We may want to use prim.GetPropertiesInNamespace("info") (or GetAuthoredPropertiesInNamespace) instead here? And then auto attr = prop.As(), which returns null if it's not an attribute (e.g. if it's a relationship). See also: https://github.com/PixarAnimationStudios/OpenUSD/blob/release/pxr/usd/usdGeom/primvarsAPI.cpp#L215.

I can't see that it would hurt any.

tcauchois · 2024-08-30T18:20:58Z

pxr/imaging/hd/utils.cpp

+    infoValues.reserve(hdNetworkMap.info.size());
+    for (const auto& infoEntry : hdNetworkMap.info)
+    {
+      // from _dataSourceLegacyPrim.cpp - _ToContainerDS(VtDictionary)


Here & elsewhere, no need to update this but if it's annoying you I think we could add VtDictionary <-> ContainerDS translators somewhere accessible (maybe retainedDataSource.h?).

There were other places in the code where these conversions were happening, but I couldn't see a clean way to share that code. I don't want to bite off more than I can chew here, so maybe I'll leave that for a later refacor - or if anyone wants to jump in and make that change, thats also fine.

pxr/imaging/hdMtlx/hdMtlx.cpp

jesschimein · 2024-09-03T15:55:52Z

/AzurePipelines run

azure-pipelines · 2024-09-03T15:56:01Z

Azure Pipelines successfully started running 1 pipeline(s).

spiffmon · 2024-09-03T22:24:57Z

Hi @ld-kerley , we've been discussing the latest changes, and realizing that since the new data we're adding will potentially affect the network that gets built, it will, for instanced materials, be "prototype-affecting" data, rather than "per-instance data". With that in mind, we were thinking that the term "Config" captures the "weight" of the data better than "Info", which kind of implies it's just advisory (admitting it's unfortunate "info" is what's used in Shader/NodeDefAPI, a legacy Pixar (or maybe Katana?) thing).

Would you be willing to change the schema name to MaterialXConfigAPI and the new datasource member to config?

ld-kerley · 2024-09-03T22:56:46Z

I'm personally open to any naming, and can see why "Info" implies advisory. Is there precedent for "Config" elsewhere in USD? I'd like to raise this with the wider MaterialX/USD community and ensure the name change doesn't raise any flags for anyone else. If there's no existing precedent for "Config" then I might open the discussion for alternatives, but if the "Config" suggestion fits in to some already established USD nomenclature, we should certainly stick with that.

spiffmon · 2024-09-03T23:49:24Z

There is "internal pixar" precedent in the Presto scenegraph that "config attributes" imply structural changes to a prim's definition; while we are continuing discussions on whether that mechanism will need to be reflected in OpenUSD, it's not a sure thing.

So sure, please do verify with the wider group and let us know if there are alt sugs!

nvmkuruc · 2024-09-04T00:36:41Z

Perhaps of note, config is currently a keyword in usda (but I don't think that limits its usage in property names and namespace elements)

https://github.com/PixarAnimationStudios/OpenUSD/blob/dev/pxr/usd/sdf/textFileFormatParser.h#L99

spiffmon · 2024-09-04T00:54:07Z

That's correct, @nvmkuruc . In fact, we've stopped authoring it in Presto/menva, but don't disallow it, for old assets. And yes, it doesn't restrict uses in property names or namespaces.

jesschimein · 2024-09-05T23:52:13Z

/AzurePipelines run

azure-pipelines · 2024-09-05T23:52:23Z

Azure Pipelines successfully started running 1 pipeline(s).

…n run the upgrade. Still need to figure out how to get to the materialX library version in the USD document.

Add 'info' VtDictionary and transport thru hydra.

ld-kerley changed the title ~~APPLE WIP : UsdMtlx MaterialXInfoAPI schema and MaterialX versioning~~ Apple WIP : UsdMtlx MaterialXInfoAPI schema and MaterialX versioning Jul 9, 2024

ld-kerley mentioned this pull request Jul 10, 2024

Apple : Compilation support for MaterialX 1.38.X and 1.39.X #3159

Closed

2 tasks

crydalch reviewed Jul 23, 2024

View reviewed changes

tcauchois reviewed Jul 23, 2024

View reviewed changes

ld-kerley force-pushed the usdmtlx_version_schema branch from fc7a0a7 to 51a693e Compare August 30, 2024 00:58

tcauchois reviewed Aug 30, 2024

View reviewed changes

pxr/imaging/hdMtlx/hdMtlx.cpp Outdated Show resolved Hide resolved

ld-kerley changed the title ~~Apple WIP : UsdMtlx MaterialXInfoAPI schema and MaterialX versioning~~ Apple : UsdMtlx MaterialXInfoAPI schema and MaterialX versioning Sep 4, 2024

ld-kerley added 6 commits October 25, 2024 17:24

add matX version to UsdShadeMaterial

84bbaa9

update HdMtlx to explicitly write the version to the document and the…

64fbd5d

…n run the upgrade. Still need to figure out how to get to the materialX library version in the USD document.

Add documentation and clean up

af49ce9

Rename materialX version attribute to info:mtlx:version.

11d8199

Add 'info' VtDictionary and transport thru hydra.

Address notes from Tom.

47ef477

Rename "info" to "config"

efbfb79

ld-kerley added 3 commits October 25, 2024 17:27

Updating 'info' to 'config' in some comments

eb7b330

Fix unit tests to work with the new MaterialX version API.

5f3ec88

Change how the config dictionary is stored.

82b8f48

ld-kerley force-pushed the usdmtlx_version_schema branch from 8353f22 to 82b8f48 Compare October 28, 2024 22:08

ld-kerley changed the title ~~Apple : UsdMtlx MaterialXInfoAPI schema and MaterialX versioning~~ Apple : UsdMtlx MaterialXConfigAPI schema and MaterialX versioning Oct 28, 2024

pixar-oss closed this in 4c880b5 Jan 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apple : UsdMtlx MaterialXConfigAPI schema and MaterialX versioning #3157

Apple : UsdMtlx MaterialXConfigAPI schema and MaterialX versioning #3157

ld-kerley commented Jul 9, 2024

jesschimein commented Jul 9, 2024

jesschimein commented Jul 9, 2024

azure-pipelines bot commented Jul 9, 2024

crydalch Jul 23, 2024

ld-kerley Jul 23, 2024

crydalch Jul 23, 2024

crydalch Jul 23, 2024 •

edited

Loading

ld-kerley Jul 23, 2024

tcauchois left a comment

tcauchois Jul 23, 2024

tcauchois Jul 23, 2024

tcauchois Jul 23, 2024

tcauchois Jul 23, 2024

ld-kerley commented Aug 30, 2024

tcauchois Aug 30, 2024

ld-kerley Aug 30, 2024

tcauchois Aug 30, 2024

ld-kerley Aug 30, 2024

jesschimein commented Sep 3, 2024

azure-pipelines bot commented Sep 3, 2024

spiffmon commented Sep 3, 2024

ld-kerley commented Sep 3, 2024

spiffmon commented Sep 3, 2024

nvmkuruc commented Sep 4, 2024

spiffmon commented Sep 4, 2024

jesschimein commented Sep 5, 2024

azure-pipelines bot commented Sep 5, 2024

Apple : UsdMtlx MaterialXConfigAPI schema and MaterialX versioning #3157

Apple : UsdMtlx MaterialXConfigAPI schema and MaterialX versioning #3157

Conversation

ld-kerley commented Jul 9, 2024

Description of Change(s)

Fixes Issue(s)

jesschimein commented Jul 9, 2024

jesschimein commented Jul 9, 2024

azure-pipelines bot commented Jul 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crydalch Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcauchois left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ld-kerley commented Aug 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesschimein commented Sep 3, 2024

azure-pipelines bot commented Sep 3, 2024

spiffmon commented Sep 3, 2024

ld-kerley commented Sep 3, 2024

spiffmon commented Sep 3, 2024

nvmkuruc commented Sep 4, 2024

spiffmon commented Sep 4, 2024

jesschimein commented Sep 5, 2024

azure-pipelines bot commented Sep 5, 2024

crydalch Jul 23, 2024 •

edited

Loading