Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update to MaterialX v1.38.3 #1738

Closed
wants to merge 1 commit into from
Closed

Update to MaterialX v1.38.3 #1738

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions BUILDING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,9 +273,9 @@ Linux and MacOS.

The additional dependencies that must be supplied when invoking cmake are:

| Dependency Name | Description |
| ------------------ |---------------------------------------- |
| MATERIALX_ROOT | The root path to a MaterialX SDK install.|
| Dependency Name | Description |
| ------------------ |----------------------------------------------------------- |
| MaterialX_DIR | Path to the CMake package config of a MaterialX SDK install.|

See [3rd Party Library and Application Versions](VERSIONS.md) for version information.

Expand Down
35 changes: 29 additions & 6 deletions build_scripts/build_usd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1345,20 +1345,43 @@ def InstallDraco(context, force, buildArgs):
############################################################
# MaterialX

MATERIALX_URL = "https://github.com/materialx/MaterialX/archive/v1.38.0.zip"
MATERIALX_URL = "https://github.com/materialx/MaterialX/archive/v1.38.3.zip"

def InstallMaterialX(context, force, buildArgs):
with CurrentWorkingDirectory(DownloadURL(MATERIALX_URL, context, force)):
# USD requires MaterialX to be built as a shared library on Linux and MacOS
# Currently MaterialX does not support shared builds on Windows
cmakeOptions = []
if Linux() or MacOS():
cmakeOptions += ['-DMATERIALX_BUILD_SHARED_LIBS=ON']
cmakeOptions = ['-DMATERIALX_BUILD_SHARED_LIBS=ON']

cmakeOptions += buildArgs;

if "v1.38.3.zip" in MATERIALX_URL:
# This will be fixed in v1.38.4:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps comment that this is for when Python is not required.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

PatchFile(os.path.join('cmake', 'modules', 'MaterialXConfig.cmake.in'),
[('set_and_check(MATERIALX_PYTHON_DIR "@PACKAGE_CMAKE_INSTALL_PREFIX@/python")\n',
'IF (@MATERIALX_BUILD_PYTHON@ AND @MATERIALX_INSTALL_PYTHON@)\n'
' set_and_check(MATERIALX_PYTHON_DIR "@PACKAGE_CMAKE_INSTALL_PREFIX@/python")\n'
'ENDIF()\n')],
multiLineMatches=True)

RunCMake(context, force, cmakeOptions)

if "v1.38.3.zip" in MATERIALX_URL:
# This will be fixed in v1.38.4:
# Post-build fix. Make sure MaterialX config is relocatable:
filename = os.path.join(context.instDir, 'lib', 'cmake', 'MaterialX', 'MaterialXTargets.cmake')
oldLines = open(filename, 'r').readlines()
newLines = []
for s in oldLines:
if s.startswith('set(_IMPORT_PREFIX '):
pathBegin = s.find(' ') + 1
pathEnd = s.rfind(')')
s = s[:pathBegin] + '${PACKAGE_PREFIX_DIR}' + s[pathEnd:]
newLines.append(s)
if newLines != oldLines:
PrintInfo("Patching file {filename} (original in {oldFilename})..."
.format(filename=filename, oldFilename=filename + ".old"))
shutil.copy(filename, filename + ".old")
open(filename, 'w').writelines(newLines)

MATERIALX = Dependency("MaterialX", InstallMaterialX, "include/MaterialXCore/Library.h")

############################################################
Expand Down
139 changes: 0 additions & 139 deletions cmake/modules/FindMaterialX.cmake
View file
Open in desktop

This file was deleted.

6 changes: 2 additions & 4 deletions pxr/imaging/hdMtlx/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,8 @@ pxr_library(hdMtlx
tf
trace
vt
${MATERIALX_LIBRARIES}

INCLUDE_DIRS
${MATERIALX_INCLUDE_DIRS}
MaterialXCore
MaterialXFormat

PUBLIC_CLASSES
hdMtlx
Expand Down
6 changes: 4 additions & 2 deletions pxr/imaging/hdMtlx/hdMtlx.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,13 @@
#include <set>
#include <unordered_map>

namespace MaterialX {
#include <MaterialXCore/Library.h>

MATERIALX_NAMESPACE_BEGIN
class FileSearchPath;
using DocumentPtr = std::shared_ptr<class Document>;
using StringMap = std::unordered_map<std::string, std::string>;
}
MATERIALX_NAMESPACE_END

PXR_NAMESPACE_OPEN_SCOPE

Expand Down
10 changes: 8 additions & 2 deletions pxr/imaging/hdSt/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,14 @@ set(optionalIncludeDirs "")
set(optionalPublicClasses "")
set(optionalPrivateClasses "")
if (${PXR_ENABLE_MATERIALX_SUPPORT})
list(APPEND optionalLibs ${MATERIALX_LIBRARIES} hdMtlx)
list(APPEND optionalIncludeDirs ${MATERIALX_INCLUDE_DIRS})
list(APPEND optionalLibs
MaterialXGenShader
MaterialXRender
MaterialXCore
MaterialXFormat
MaterialXGenGlsl
hdMtlx
)
list(APPEND optionalPrivateClasses
materialXFilter
materialXShaderGen
Expand Down
4 changes: 2 additions & 2 deletions pxr/imaging/hdSt/materialXFilter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ TF_DEFINE_PRIVATE_TOKENS(
static mx::ShaderPtr
_GenMaterialXShader(mx::GenContext & mxContext, mx::ElementPtr const& mxElem)
{
bool hasTransparency = mx::isTransparentSurface(mxElem,
mxContext.getShaderGenerator());
bool hasTransparency = mx::isTransparentSurface(mxElem,
mxContext.getShaderGenerator().getTarget());

mx::GenContext materialContext = mxContext;
materialContext.getOptions().hwTransparency = hasTransparency;
Expand Down
15 changes: 7 additions & 8 deletions pxr/imaging/hdSt/materialXShaderGen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,8 +230,8 @@ HdStMaterialXShaderGen::_EmitMxFunctions(
mx::ShaderStage& mxStage) const
{
// Add global constants and type definitions
emitInclude("pbrlib/" + mx::GlslShaderGenerator::TARGET
+ "/lib/mx_defines.glsl", mxContext, mxStage);
emitInclude("stdlib/" + mx::GlslShaderGenerator::TARGET
+ "/lib/mx_math.glsl", mxContext, mxStage);
emitLine("#if NUM_LIGHTS > 0", mxStage, false);
emitLine("#define MAX_LIGHT_SOURCES NUM_LIGHTS", mxStage, false);
emitLine("#else", mxStage, false);
Expand Down Expand Up @@ -343,11 +343,6 @@ HdStMaterialXShaderGen::_EmitMxFunctions(
_EmitMxInitFunction(vertexData, mxStage);
}

// Emit common math functions
emitInclude("pbrlib/" + mx::GlslShaderGenerator::TARGET
+ "/lib/mx_math.glsl", mxContext, mxStage);
emitLineBreak(mxStage);

// Emit lighting and shadowing code
if (lighting) {
emitSpecularEnvironment(mxContext, mxStage);
Expand Down Expand Up @@ -384,6 +379,9 @@ HdStMaterialXShaderGen::_EmitMxFunctions(
emitInclude(ShaderGenerator::T_FILE_TRANSFORM_UV, mxContext, mxStage);
}

// Add light sampling functions
emitLightFunctionDefinitions(mxGraph, mxContext, mxStage);

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a check here to see if needed: The albedo table initiation was affected by light refactoring and this fixes it. be0d3d65727ad343eaf469e9c179f772d31a9be2

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Add all functions for node implementations
emitFunctionDefinitions(mxGraph, mxContext, mxStage);
}
Expand All @@ -405,7 +403,8 @@ HdStMaterialXShaderGen::_EmitMxSurfaceShader(
emitLine("mxInit(Peye, Neye)", mxStage);

const mx::ShaderGraphOutputSocket* outputSocket = mxGraph.getOutputSocket();
if (mxGraph.hasClassification(mx::ShaderNode::Classification::CLOSURE)) {
if (mxGraph.hasClassification(mx::ShaderNode::Classification::CLOSURE) &&
!mxGraph.hasClassification(mx::ShaderNode::Classification::SHADER)) {
// Handle the case where the mxGraph is a direct closure.
// We don't support rendering closures without attaching
// to a surface shader, so just output black.
Expand Down
6 changes: 2 additions & 4 deletions pxr/usd/plugin/usdMtlx/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,8 @@ pxr_plugin(usdMtlx
usdShade
usdUI
usdUtils
${MATERIALX_LIBRARIES}

INCLUDE_DIRS
${MATERIALX_INCLUDE_DIRS}
MaterialXCore
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

MaterialXFormat

CPPFILES
debugCodes.cpp
Expand Down
19 changes: 17 additions & 2 deletions pxr/usd/plugin/usdMtlx/reader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,7 @@ _FindMatchingNodeDef(
const mx::ConstDocumentPtr& mtlxDocument,
const mx::ConstInterfaceElementPtr& mtlxInterface,
const std::string& family,
const std::string& type,
const NdrVersion& version,
const std::string& target)
{
Expand All @@ -289,7 +290,11 @@ _FindMatchingNodeDef(
}

// Filter by types.
if (mtlxInterface && !mtlxInterface->isTypeCompatible(mtlxNodeDef)) {
if (mtlxInterface && !mtlxInterface->hasExactInputMatch(mtlxNodeDef)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

continue;
}

if (mtlxNodeDef->getType() != type) {
continue;
}

Expand Down Expand Up @@ -336,6 +341,7 @@ _FindMatchingNodeDef(
auto nodeDef = _FindMatchingNodeDef(mtlxShaderNode->getDocument(),
mtlxInterface,
mtlxShaderNode->getCategory(),
mtlxShaderNode->getType(),
UsdMtlxGetVersion(mtlxShaderNode),
mtlxShaderNode->getTarget());
if (nodeDef) {
Expand All @@ -344,9 +350,18 @@ _FindMatchingNodeDef(

// Get the standard library document and check that.
static auto standardLibraryDocument = UsdMtlxGetDocument("");

if (mtlxShaderNode->hasNodeDefString()) {
nodeDef = standardLibraryDocument->getNodeDef(mtlxShaderNode->getNodeDefString());
if (nodeDef) {
return nodeDef;
}
}

return _FindMatchingNodeDef(standardLibraryDocument,
mtlxInterface,
mtlxShaderNode->getCategory(),
mtlxShaderNode->getType(),
UsdMtlxGetVersion(mtlxShaderNode),
mtlxShaderNode->getTarget());
}
Expand Down Expand Up @@ -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) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to double check here that this handles finding the / associations found on .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// The shaderNode specified a node instead of a nodeDef. Find
// the best matching nodedef since the MaterialX API doesn't.
mtlxNodeDef =
Expand Down
2 changes: 1 addition & 1 deletion pxr/usd/plugin/usdMtlx/utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ _ReadFromAsset(mx::DocumentPtr doc, const ArResolvedPath& resolvedPath,
newReadOptions);
};

mx::readFromXmlString(doc, s, &readOptions);
mx::readFromXmlString(doc, s, searchPath, &readOptions);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just updating to the latest API. No behavior change.

}
#endif

Expand Down
10 changes: 8 additions & 2 deletions third_party/renderman-24/plugin/hdPrman/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,14 @@ endif()
set(optionalIncludeDirs "")
set(optionalPublicClasses "")
if (${PXR_ENABLE_MATERIALX_SUPPORT})
list(APPEND optionalLibs ${MATERIALX_LIBRARIES} hdMtlx)
list(APPEND optionalIncludeDirs ${MATERIALX_INCLUDE_DIRS})
list(APPEND optionalLibs
MaterialXCore
MaterialXFormat
MaterialXGenShader
MaterialXGenOsl
MaterialXRender
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious as to why there is a dependence on MaterialXRender ?
Generally I would assume this is not required.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I grepped for #include <MaterialX in the code and added what was found. In this case, I saw one MaterialXRender include being accessed.

hdMtlx
)
list(APPEND optionalPublicClasses matfiltMaterialX)

if (${PXR_ENABLE_OSL_SUPPORT})
Expand Down