From 91d90f3929b138d93d4079c8a5c328a1b91900da Mon Sep 17 00:00:00 2001 From: Jerry Gamache Date: Wed, 25 May 2022 17:29:10 -0400 Subject: [PATCH 1/6] Parse MaterialX metadata into SdrProperty Fixes #1874 - "enum" and "enumvalues" are parsed as options - "doc" is parsed as help - "uiname" is parsed as label - "uifolder" is parsed as page - all other MaterialX metadata is added as hints --- pxr/usd/usdMtlx/parser.cpp | 105 ++++++++++++++++++ pxr/usd/usdMtlx/testenv/testUsdMtlxParser.py | 34 ++++++ .../testUsdMtlxParser.testenv/test.mtlx | 6 +- 3 files changed, 142 insertions(+), 3 deletions(-) diff --git a/pxr/usd/usdMtlx/parser.cpp b/pxr/usd/usdMtlx/parser.cpp index a7a031da6f..8dfd31507c 100644 --- a/pxr/usd/usdMtlx/parser.cpp +++ b/pxr/usd/usdMtlx/parser.cpp @@ -48,6 +48,14 @@ TF_DEFINE_PRIVATE_TOKENS( ((discoveryType, "mtlx")) ((sourceType, "")) + + (uimin) + (uimax) + (uisoftmin) + (uisoftmax) + (uistep) + (unit) + (unittype) ); // This environment variable lets users override the name of the primary @@ -123,6 +131,73 @@ struct ShaderBuilder { std::map _propertyNameRemapping; }; +static +void +ParseMetadata( + NdrTokenMap& metadata, + const TfToken& key, + const mx::ConstElementPtr& element, + const std::string& attribute) +{ + const auto& value = element->getAttribute(attribute); + if (!value.empty()) { + metadata.emplace(key, value); + } +} + +static +void +ParseOptions( + NdrOptionVec& options, + const mx::ConstElementPtr& element +) +{ + const auto& enumLabels = element->getAttribute("enum"); + if (enumLabels.empty()) { + return; + } + + const auto& enumValues = element->getAttribute("enumvalues"); + std::vector allLabels = UsdMtlxSplitStringArray(enumLabels); + std::vector allValues = UsdMtlxSplitStringArray(enumValues); + + if (allValues.size() && allValues.size() != allLabels.size()) { + // An array of vector2 values will produce twice the expected number of + // elements. We can fix that by regrouping them. + if (allValues.size() > allLabels.size() && + allValues.size() % allLabels.size() == 0) { + + size_t stride = allValues.size() / allLabels.size(); + std::vector rebuiltValues; + std::string currentValue; + for (size_t i = 0; i < allValues.size(); ++i) { + if (i % stride != 0) { + currentValue += mx::ARRAY_PREFERRED_SEPARATOR; + } + currentValue += allValues[i]; + if ((i+1) % stride == 0) { + rebuiltValues.push_back(currentValue); + currentValue = ""; + } + } + allValues.swap(rebuiltValues); + } else { + // Can not reconcile the size difference: + allValues.clear(); + } + } + + auto itLabels = allLabels.cbegin(); + auto itValues = allValues.cbegin(); + while (itLabels != allLabels.cend()) { + TfToken value; + if (itValues != allValues.cend()) { + value = TfToken(*itValues++); + } + options.emplace_back(TfToken(*itLabels++), value); + } +} + void ShaderBuilder::AddProperty( const mx::ConstTypedElementPtr& element, @@ -239,6 +314,36 @@ ShaderBuilder::AddProperty( metadata[SdrPropertyMetadata->ImplementationName] = j->second; } + if (!isOutput) { + ParseMetadata(metadata, SdrPropertyMetadata->Label, element, "uiname"); + ParseMetadata(metadata, SdrPropertyMetadata->Help, element, "doc"); + ParseMetadata(metadata, SdrPropertyMetadata->Page, element, "uifolder"); + + ParseMetadata(metadata, _tokens->uimin, element, "uimin"); + ParseMetadata(metadata, _tokens->uimax, element, "uimax"); + ParseMetadata(metadata, _tokens->uisoftmin, element, "uisoftmin"); + ParseMetadata(metadata, _tokens->uisoftmax, element, "uisoftmax"); + ParseMetadata(metadata, _tokens->uistep, element, "uistep"); + ParseMetadata(metadata, _tokens->unit, element, "unit"); + ParseMetadata(metadata, _tokens->unittype, element, "unittype"); + + for (const auto& pair : metadata) { + const TfToken attrName = pair.first; + const std::string attrValue = pair.second; + + if (std::find(SdrPropertyMetadata->allTokens.begin(), + SdrPropertyMetadata->allTokens.end(), + attrName) != SdrPropertyMetadata->allTokens.end()){ + continue; + } + + // Attribute hasn't been handled yet, so put it into the hints dict + hints.insert({attrName, attrValue}); + } + + ParseOptions(options, element); + } + // Add the property. properties.push_back( SdrShaderPropertyUniquePtr( diff --git a/pxr/usd/usdMtlx/testenv/testUsdMtlxParser.py b/pxr/usd/usdMtlx/testenv/testUsdMtlxParser.py index f68ebcdaa5..f0f91a1de3 100644 --- a/pxr/usd/usdMtlx/testenv/testUsdMtlxParser.py +++ b/pxr/usd/usdMtlx/testenv/testUsdMtlxParser.py @@ -55,6 +55,40 @@ def test_NodeParser(self): self.assertEqual(sorted(node.GetInputNames()), ["in", "note"]) self.assertEqual(node.GetOutputNames(), ['out']) + # Verify some metadata: + node = Sdr.Registry().GetShaderNodeByIdentifier( + 'UsdMtlxTestNamespace:nd_vector') + self.assertEqual(node.GetHelp(), "Vector help") + # Properties without a Page metadata end up in an unnamed page. This + # means that all MaterialX outputs will be assigned to the unnamed page + # when the metadata is used. + self.assertEqual(node.GetPages(), ["UI Page", ""]) + self.assertEqual(node.GetPropertyNamesForPage("UI Page"), ["in",]) + self.assertEqual(node.GetPropertyNamesForPage(""), ["note", "out"]) + input = node.GetInput("in") + self.assertEqual(input.GetHelp(), "Property help") + self.assertEqual(input.GetLabel(), "UI Vector") + self.assertEqual(input.GetPage(), "UI Page") + self.assertEqual(input.GetOptions(), + [("X", "1, 0, 0"), ("Y", "0, 1, 0"), ("Z", "0, 0, 1")]) + + node = Sdr.Registry().GetShaderNodeByIdentifier( + 'UsdMtlxTestNamespace:nd_float') + expected = { + "uimin": "-360.0", + "uimax": "360.0", + "uisoftmin": "0.0", + "uisoftmax": "180.0", + "uistep": "1.0", + "unittype": "angle", + "unit": "degree" + } + hints = node.GetInput("in").GetHints() + metadata = node.GetInput("in").GetMetadata() + for key in expected.keys(): + self.assertEqual(hints[key], expected[key]) + self.assertEqual(metadata[key], expected[key]) + # Verify converted types. typeNameMap = { 'boolean': 'bool', diff --git a/pxr/usd/usdMtlx/testenv/testUsdMtlxParser.testenv/test.mtlx b/pxr/usd/usdMtlx/testenv/testUsdMtlxParser.testenv/test.mtlx index 5a5bd4cf4d..8f7849138d 100644 --- a/pxr/usd/usdMtlx/testenv/testUsdMtlxParser.testenv/test.mtlx +++ b/pxr/usd/usdMtlx/testenv/testUsdMtlxParser.testenv/test.mtlx @@ -6,7 +6,7 @@ - + @@ -15,8 +15,8 @@ - - + + From f6f7c95d5e0a0946efa557c1d262f4a6f92b7fe3 Mon Sep 17 00:00:00 2001 From: Jerry Gamache Date: Fri, 27 May 2022 15:29:56 -0400 Subject: [PATCH 2/6] Improve readability --- pxr/usd/usdMtlx/parser.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pxr/usd/usdMtlx/parser.cpp b/pxr/usd/usdMtlx/parser.cpp index 8dfd31507c..c3b595380b 100644 --- a/pxr/usd/usdMtlx/parser.cpp +++ b/pxr/usd/usdMtlx/parser.cpp @@ -161,7 +161,7 @@ ParseOptions( std::vector allLabels = UsdMtlxSplitStringArray(enumLabels); std::vector allValues = UsdMtlxSplitStringArray(enumValues); - if (allValues.size() && allValues.size() != allLabels.size()) { + if (!allValues.empty() && allValues.size() != allLabels.size()) { // An array of vector2 values will produce twice the expected number of // elements. We can fix that by regrouping them. if (allValues.size() > allLabels.size() && @@ -331,13 +331,13 @@ ShaderBuilder::AddProperty( const TfToken attrName = pair.first; const std::string attrValue = pair.second; - if (std::find(SdrPropertyMetadata->allTokens.begin(), - SdrPropertyMetadata->allTokens.end(), - attrName) != SdrPropertyMetadata->allTokens.end()){ + const auto& allTokens = SdrPropertyMetadata->allTokens; + if (std::find(allTokens.begin(), allTokens.end(), attrName) != + allTokens.end()) { continue; } - // Attribute hasn't been handled yet, so put it into the hints dict + // Attribute hasn't been handled yet, so put it into the hints dict. hints.insert({attrName, attrValue}); } From 5e2be0de390b80f662f795a9f96985e23694e2db Mon Sep 17 00:00:00 2001 From: Jerry Gamache Date: Mon, 30 May 2022 16:03:06 -0400 Subject: [PATCH 3/6] Add unit documentation when possible. --- pxr/usd/usdMtlx/parser.cpp | 8 ++++++++ pxr/usd/usdMtlx/testenv/testUsdMtlxParser.py | 6 ++++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/pxr/usd/usdMtlx/parser.cpp b/pxr/usd/usdMtlx/parser.cpp index c3b595380b..be5cb4ef0f 100644 --- a/pxr/usd/usdMtlx/parser.cpp +++ b/pxr/usd/usdMtlx/parser.cpp @@ -327,6 +327,14 @@ ShaderBuilder::AddProperty( ParseMetadata(metadata, _tokens->unit, element, "unit"); ParseMetadata(metadata, _tokens->unittype, element, "unittype"); + if (!metadata.count(SdrPropertyMetadata->Help) && + metadata.count(_tokens->unit)) { + // The unit can be helpful if there is no documentation. + metadata.emplace(SdrPropertyMetadata->Help, + TfStringPrintf("Unit is %s.", + metadata[_tokens->unit].c_str())); + } + for (const auto& pair : metadata) { const TfToken attrName = pair.first; const std::string attrValue = pair.second; diff --git a/pxr/usd/usdMtlx/testenv/testUsdMtlxParser.py b/pxr/usd/usdMtlx/testenv/testUsdMtlxParser.py index f0f91a1de3..93ca25167b 100644 --- a/pxr/usd/usdMtlx/testenv/testUsdMtlxParser.py +++ b/pxr/usd/usdMtlx/testenv/testUsdMtlxParser.py @@ -83,8 +83,10 @@ def test_NodeParser(self): "unittype": "angle", "unit": "degree" } - hints = node.GetInput("in").GetHints() - metadata = node.GetInput("in").GetMetadata() + input = node.GetInput("in") + self.assertEqual(input.GetHelp(), "Unit is degree.") + hints = input.GetHints() + metadata = input.GetMetadata() for key in expected.keys(): self.assertEqual(hints[key], expected[key]) self.assertEqual(metadata[key], expected[key]) From 06b163af8dd4ef36e8a1060f91785d7545352e40 Mon Sep 17 00:00:00 2001 From: Jerry Gamache Date: Mon, 20 Jun 2022 14:01:09 -0400 Subject: [PATCH 4/6] Added defaultgeomprop. Cleaned up strings. --- pxr/usd/usdMtlx/parser.cpp | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/pxr/usd/usdMtlx/parser.cpp b/pxr/usd/usdMtlx/parser.cpp index be5cb4ef0f..e49ea30265 100644 --- a/pxr/usd/usdMtlx/parser.cpp +++ b/pxr/usd/usdMtlx/parser.cpp @@ -56,6 +56,7 @@ TF_DEFINE_PRIVATE_TOKENS( (uistep) (unit) (unittype) + (defaultgeomprop) ); // This environment variable lets users override the name of the primary @@ -145,6 +146,19 @@ ParseMetadata( } } +static +void +ParseMetadata( + NdrTokenMap& metadata, + const TfToken& key, + const mx::ConstElementPtr& element) +{ + const auto& value = element->getAttribute(key.GetString()); + if (!value.empty()) { + metadata.emplace(key, value); + } +} + static void ParseOptions( @@ -282,14 +296,13 @@ ShaderBuilder::AddProperty( auto name = element->getName(); // Record builtin primvar references for this node's inputs. - static const std::string defaultgeompropName("defaultgeomprop"); if (!isOutput && primvars != nullptr) { // If an input has "defaultgeomprop", that means it reads from the // primvar specified unless connected. We mark these in Sdr as // always-required primvars; note that this means we might overestimate // which primvars are referenced in a material. - const auto& defaultgeomprop = element->getAttribute(defaultgeompropName); + const auto& defaultgeomprop = element->getAttribute(_tokens->defaultgeomprop.GetString()); if (!defaultgeomprop.empty()) { // Note: MaterialX uses a default texcoord of "UV0", which we // inline replace with the configured default. @@ -319,13 +332,14 @@ ShaderBuilder::AddProperty( ParseMetadata(metadata, SdrPropertyMetadata->Help, element, "doc"); ParseMetadata(metadata, SdrPropertyMetadata->Page, element, "uifolder"); - ParseMetadata(metadata, _tokens->uimin, element, "uimin"); - ParseMetadata(metadata, _tokens->uimax, element, "uimax"); - ParseMetadata(metadata, _tokens->uisoftmin, element, "uisoftmin"); - ParseMetadata(metadata, _tokens->uisoftmax, element, "uisoftmax"); - ParseMetadata(metadata, _tokens->uistep, element, "uistep"); - ParseMetadata(metadata, _tokens->unit, element, "unit"); - ParseMetadata(metadata, _tokens->unittype, element, "unittype"); + ParseMetadata(metadata, _tokens->uimin, element); + ParseMetadata(metadata, _tokens->uimax, element); + ParseMetadata(metadata, _tokens->uisoftmin, element); + ParseMetadata(metadata, _tokens->uisoftmax, element); + ParseMetadata(metadata, _tokens->uistep, element); + ParseMetadata(metadata, _tokens->unit, element); + ParseMetadata(metadata, _tokens->unittype, element); + ParseMetadata(metadata, _tokens->defaultgeomprop, element); if (!metadata.count(SdrPropertyMetadata->Help) && metadata.count(_tokens->unit)) { From 254a9222dbf3524b46bff0bb67e73f97c039bc13 Mon Sep 17 00:00:00 2001 From: Jerry Gamache Date: Thu, 1 Dec 2022 15:59:20 -0500 Subject: [PATCH 5/6] Cleanup use of C strings All C strings were replaced by TfTokens. Reinforced warning about unit divergence between MaterialX and USD. --- pxr/usd/usdMtlx/parser.cpp | 61 ++++++++++++++++++++++---------------- 1 file changed, 35 insertions(+), 26 deletions(-) diff --git a/pxr/usd/usdMtlx/parser.cpp b/pxr/usd/usdMtlx/parser.cpp index 58661bcf48..afffceff2d 100644 --- a/pxr/usd/usdMtlx/parser.cpp +++ b/pxr/usd/usdMtlx/parser.cpp @@ -49,14 +49,25 @@ TF_DEFINE_PRIVATE_TOKENS( ((discoveryType, "mtlx")) ((sourceType, "")) - (uimin) + (colorspace) + (defaultgeomprop) + (defaultinput) + (doc) + (enum) + (enumvalues) + (nodecategory) + (nodegroup) + (target) + (uifolder) (uimax) - (uisoftmin) + (uimin) + (uiname) (uisoftmax) + (uisoftmin) (uistep) (unit) (unittype) - (defaultgeomprop) + (UV0) ); // This environment variable lets users override the name of the primary @@ -69,7 +80,7 @@ static const std::string _GetPrimaryUvSetName() { static const std::string env = TfGetEnvSetting(USDMTLX_PRIMARY_UV_NAME); if (env.empty()) { - return UsdUtilsGetPrimaryUVSetName().GetString(); + return UsdUtilsGetPrimaryUVSetName(); } return env; } @@ -154,7 +165,7 @@ ParseMetadata( const TfToken& key, const mx::ConstElementPtr& element) { - const auto& value = element->getAttribute(key.GetString()); + const auto& value = element->getAttribute(key); if (!value.empty()) { metadata.emplace(key, value); } @@ -167,12 +178,12 @@ ParseOptions( const mx::ConstElementPtr& element ) { - const auto& enumLabels = element->getAttribute("enum"); + const auto& enumLabels = element->getAttribute(_tokens->enum); if (enumLabels.empty()) { return; } - const auto& enumValues = element->getAttribute("enumvalues"); + const auto& enumValues = element->getAttribute(_tokens->enumvalues); std::vector allLabels = UsdMtlxSplitStringArray(enumLabels); std::vector allValues = UsdMtlxSplitStringArray(enumValues); @@ -266,27 +277,24 @@ ShaderBuilder::AddProperty( } // If this is an output then save the defaultinput, if any. - static const std::string defaultinputName("defaultinput"); if (isOutput) { - const auto& defaultinput = element->getAttribute(defaultinputName); + const auto& defaultinput = element->getAttribute(_tokens->defaultinput); if (!defaultinput.empty()) { metadata.emplace(SdrPropertyMetadata->DefaultInput, defaultinput); } } // Record the targets on inputs. - static const std::string targetName("target"); if (!isOutput) { - const auto& target = element->getAttribute(targetName); + const auto& target = element->getAttribute(_tokens->target); if (!target.empty()) { metadata.emplace(SdrPropertyMetadata->Target, target); } } // Record the colorspace on inputs and outputs. - static const std::string colorspaceName("colorspace"); if (isOutput || element->isA()) { - const auto& colorspace = element->getAttribute(colorspaceName); + const auto& colorspace = element->getAttribute(_tokens->colorspace); if (!colorspace.empty() && colorspace != element->getParent()->getActiveColorSpace()) { metadata.emplace(SdrPropertyMetadata->Colorspace, colorspace); @@ -303,11 +311,11 @@ ShaderBuilder::AddProperty( // primvar specified unless connected. We mark these in Sdr as // always-required primvars; note that this means we might overestimate // which primvars are referenced in a material. - const auto& defaultgeomprop = element->getAttribute(_tokens->defaultgeomprop.GetString()); + const auto& defaultgeomprop = element->getAttribute(_tokens->defaultgeomprop); if (!defaultgeomprop.empty()) { // Note: MaterialX uses a default texcoord of "UV0", which we // inline replace with the configured default. - if (defaultgeomprop == "UV0") { + if (defaultgeomprop == _tokens->UV0) { if (!addedTexcoordPrimvar) { primvars->push_back(_GetPrimaryUvSetName()); } @@ -321,7 +329,7 @@ ShaderBuilder::AddProperty( // multiple outputs. The default name would be the name of the // nodedef itself, which seems wrong. We pick a different name. if (auto nodeDef = element->asA()) { - name = UsdMtlxTokens->DefaultOutputName.GetString(); + name = UsdMtlxTokens->DefaultOutputName; } // Remap property name. @@ -331,9 +339,9 @@ ShaderBuilder::AddProperty( } if (!isOutput) { - ParseMetadata(metadata, SdrPropertyMetadata->Label, element, "uiname"); - ParseMetadata(metadata, SdrPropertyMetadata->Help, element, "doc"); - ParseMetadata(metadata, SdrPropertyMetadata->Page, element, "uifolder"); + ParseMetadata(metadata, SdrPropertyMetadata->Label, element, _tokens->uiname); + ParseMetadata(metadata, SdrPropertyMetadata->Help, element, _tokens->doc); + ParseMetadata(metadata, SdrPropertyMetadata->Page, element, _tokens->uifolder); ParseMetadata(metadata, _tokens->uimin, element); ParseMetadata(metadata, _tokens->uimax, element); @@ -348,7 +356,8 @@ ShaderBuilder::AddProperty( metadata.count(_tokens->unit)) { // The unit can be helpful if there is no documentation. metadata.emplace(SdrPropertyMetadata->Help, - TfStringPrintf("Unit is %s.", + TfStringPrintf("Unit is %s. Please note that this MaterialX" + " unit can diverge from USD norms.", metadata[_tokens->unit].c_str())); } @@ -446,10 +455,10 @@ ParseElement(ShaderBuilder* builder, const mx::ConstNodeDefPtr& nodeDef) // Metadata builder->metadata[SdrNodeMetadata->Label] = nodeDef->getNodeString(); - ParseMetadata(builder, SdrNodeMetadata->Category, nodeDef, "nodecategory"); - ParseMetadata(builder, SdrNodeMetadata->Help, nodeDef, "doc"); - ParseMetadata(builder, SdrNodeMetadata->Target, nodeDef, "target"); - ParseMetadata(builder, SdrNodeMetadata->Role, nodeDef, "nodegroup"); + ParseMetadata(builder, SdrNodeMetadata->Category, nodeDef, _tokens->nodecategory); + ParseMetadata(builder, SdrNodeMetadata->Help, nodeDef, _tokens->doc); + ParseMetadata(builder, SdrNodeMetadata->Target, nodeDef, _tokens->target); + ParseMetadata(builder, SdrNodeMetadata->Role, nodeDef, _tokens->nodegroup); // XXX -- version @@ -512,7 +521,7 @@ ParseElement(ShaderBuilder* builder, const mx::ConstNodeDefPtr& nodeDef) // Note: MaterialX uses a default texcoord of "UV0", which we // inline replace with the configured default. for (auto& name : split) { - if (name == "UV0") { + if (name == _tokens->UV0) { name = _GetPrimaryUvSetName(); } } @@ -571,7 +580,7 @@ UsdMtlxParserPlugin::Parse(const NdrNodeDiscoveryResult& discoveryResult) return GetInvalidNode(discoveryResult); } - auto nodeDef = document->getNodeDef(discoveryResult.identifier.GetString()); + auto nodeDef = document->getNodeDef(discoveryResult.identifier); if (!nodeDef) { TF_WARN("Invalid MaterialX NodeDef; unknown node name ' %s '", discoveryResult.identifier.GetText()); From 4545ae105044cca56338afa607680eb77a10b1ac Mon Sep 17 00:00:00 2001 From: Jerry Gamache Date: Tue, 24 Jan 2023 11:37:16 -0500 Subject: [PATCH 6/6] Fix invalid use of C++ identifier --- pxr/usd/usdMtlx/parser.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pxr/usd/usdMtlx/parser.cpp b/pxr/usd/usdMtlx/parser.cpp index afffceff2d..235bf62cbb 100644 --- a/pxr/usd/usdMtlx/parser.cpp +++ b/pxr/usd/usdMtlx/parser.cpp @@ -53,7 +53,7 @@ TF_DEFINE_PRIVATE_TOKENS( (defaultgeomprop) (defaultinput) (doc) - (enum) + ((enum_, "enum")) (enumvalues) (nodecategory) (nodegroup) @@ -178,7 +178,7 @@ ParseOptions( const mx::ConstElementPtr& element ) { - const auto& enumLabels = element->getAttribute(_tokens->enum); + const auto& enumLabels = element->getAttribute(_tokens->enum_); if (enumLabels.empty()) { return; }