Update to MaterialX v1.38.3 #1738
Conversation
- Use CMake config directly from MaterialX install - Update for API and library changes in v1.38.3 - Strenghten NodeDef discovery This is the last update that is not version tagged in the C++ code. Future updates will be bracketed using MATERIALX_MINOR_VERSION defines introduced in v1.38.3.
| @@ -384,6 +379,9 @@ HdStMaterialXShaderGen::_EmitMxFunctions( | |||
| emitInclude(ShaderGenerator::T_FILE_TRANSFORM_UV, mxContext, mxStage); | |||
| } | |||
|
|
|||
| // Add light sampling functions | |||
| emitLightFunctionDefinitions(mxGraph, mxContext, mxStage); | |||
|
|
|||
There was a problem hiding this comment.
Adding a check here to see if needed: The albedo table initiation was affected by light refactoring and this fixes it. be0d3d65727ad343eaf469e9c179f772d31a9be2
There was a problem hiding this comment.
Storm has its own dome light functions (see line 531) and does not use the MaterialX albedo table. Adding this was necessary to get access to scene light functions. So no check required.
|
|
||
| INCLUDE_DIRS | ||
| ${MATERIALX_INCLUDE_DIRS} | ||
| MaterialXCore |
There was a problem hiding this comment.
I'm curious why explicit library names are now used vs ${MATEIALX_LIBRARIES}. Will this affect users which use more than the
explicitly specified library currently ?
(This appears to be done in a few build configurations).
There was a problem hiding this comment.
It allows linking only the libraries that are in use instead of all libraries.
Also, with this modern CMake implementation, the required include dirs (and also the required compiler definitions) gets added on a per-library basis.
| @@ -1284,7 +1299,7 @@ _Context::AddShaderNode(const mx::ConstNodePtr& mtlxShaderNode) | |||
|
|
|||
| // Get the nodeDef for this shaderNode. | |||
| mx::ConstNodeDefPtr mtlxNodeDef = mtlxShaderNode->getNodeDef(); | |||
| if (mtlxShaderNode->getNodeDefString().empty()) { | |||
| if (!mtlxNodeDef) { | |||
There was a problem hiding this comment.
I wanted to double check here that this handles finding the / associations found on .
There was a problem hiding this comment.
Context: at this exact point in the code, the MaterialX library has not been loaded. This means that a node that has a NodeDef string will fail to resolve, and previously would not be tried again. Changing the test to retry when the NodeDef was not found allows trying a second time to find the NodeDef using the string, but this time with libraries loaded.
| MaterialXFormat | ||
| MaterialXGenShader | ||
| MaterialXGenOsl | ||
| MaterialXRender |
There was a problem hiding this comment.
I'm curious as to why there is a dependence on MaterialXRender ?
Generally I would assume this is not required.
There was a problem hiding this comment.
I grepped for #include <MaterialX in the code and added what was found. In this case, I saw one MaterialXRender include being accessed.
| @@ -289,7 +290,11 @@ _FindMatchingNodeDef( | |||
| } | |||
|
|
|||
| // Filter by types. | |||
| if (mtlxInterface && !mtlxInterface->isTypeCompatible(mtlxNodeDef)) { | |||
| if (mtlxInterface && !mtlxInterface->hasExactInputMatch(mtlxNodeDef)) { | |||
There was a problem hiding this comment.
This is just a query: This assumes the latest API. I'm curious if code which handles multiple versions is not desirable e.g. if version preprocessor statements ?
There was a problem hiding this comment.
This requires knowing which version of MaterialX is in use and that can only be done starting with MaterialX v1.38.3.
I fully expect that submitting code that handles multiple versions will be the default for all future PRs.
|
|
||
| cmakeOptions += buildArgs; | ||
|
|
||
| if "v1.38.3.zip" in MATERIALX_URL: | ||
| # This will be fixed in v1.38.4: |
There was a problem hiding this comment.
Perhaps comment that this is for when Python is not required.
There was a problem hiding this comment.
The default in USD is to build MaterialX without Python. This is how I found the fix was required.
I could possibly add a comment pointing to the fix if requested: autodesk-forks/MaterialX#1344
And the fix for CMake relocability: autodesk-forks/MaterialX#1350
That were both merged in ILM repo via: AcademySoftwareFoundation/MaterialX#805
| @@ -297,7 +297,7 @@ _ReadFromAsset(mx::DocumentPtr doc, const ArResolvedPath& resolvedPath, | |||
| newReadOptions); | |||
| }; | |||
|
|
|||
| mx::readFromXmlString(doc, s, &readOptions); | |||
| mx::readFromXmlString(doc, s, searchPath, &readOptions); | |||
There was a problem hiding this comment.
Is this purely to update the API, or will there be any behaviour change here.
If syntatical then should this instead be an empty search path ? Seems passing
searchPath is correct though. Just wondering.
There was a problem hiding this comment.
Just updating to the latest API. No behavior change.
|
Github decided to fail all builds done in Windows 2016 VMs: actions/runner-images#4312 |
|
Filed as internal issue #USD-7129 |
|
Closing, per @seando-adsk , in favor of PR #1792 |
This is the last update that is not version tagged in the C++ code.
Future updates will be bracketed using MATERIALX_MINOR_VERSION defines
introduced in v1.38.3.