Parse MaterialX metadata into SdrProperty #1895
Conversation
Fixes PixarAnimationStudios#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
|
Filed as internal issue #USD-7414 |
pxr/usd/usdMtlx/parser.cpp
Outdated
| 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()); |
There was a problem hiding this comment.
TfToken should auto-convert to std::string
pxr/usd/usdMtlx/parser.cpp
Outdated
| ParseMetadata(metadata, SdrPropertyMetadata->Label, element, "uiname"); | ||
| ParseMetadata(metadata, SdrPropertyMetadata->Help, element, "doc"); | ||
| ParseMetadata(metadata, SdrPropertyMetadata->Page, element, "uifolder"); |
There was a problem hiding this comment.
Any reason why these three literals, plus "enum" and "enumvalues" above aren't also added to the private tokens?
There was a problem hiding this comment.
Yes, but not necessarily a good one. These strings are only used as std::string on the MaterialX side of things. Would be best to have them as tokens as well since that prevents an unnecessary malloc/strcpy.
|
|
||
| if (!metadata.count(SdrPropertyMetadata->Help) && | ||
| metadata.count(_tokens->unit)) { | ||
| // The unit can be helpful if there is no documentation. |
There was a problem hiding this comment.
Not sure what unittype is, but would it also be useful? Seems like defaultgeomprop would also be handy to provide?
There was a problem hiding this comment.
I see only two places in MaterialX where unittype is defined. There are a few angles defined as being "unittype = radian" and one thin_film thickness being tagged with "unittype = nanometer". I suspect ST coordinates could be in meters in some future, but we are not there yet.
There was a problem hiding this comment.
Oh hmmm... these will want to be very prominently documented, as they diverge from USD norms. All angles are degrees in USD schemas, and linear units are generally relative to stage metersPerUnit . So yeah, I'd double-down on wanting to fold unittype into the description, if the nodes that use them don't already provide documentation that mentions them!
|
Just a couple questions/sugs, but looks good overall, @JGamache-autodesk . Let us know if you plan on updating the PR? |
|
@JGamache-autodesk , we might be able to get this into 23.02 if you can update the PR this week/early next week? |
|
Hi @spiffmon, I have a patch in my local repo. I will try to get it merged into the PR early next week. |
All C strings were replaced by TfTokens. Reinforced warning about unit divergence between MaterialX and USD.
seando-adsk
requested review from
spiffmon and
JGamache-autodesk
and removed request for
spiffmon
Description of Change(s)
Fixes #1874