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.

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

MaterialX : adds MaterialX #190

Closed
wants to merge 2 commits into from

Conversation

proberts-cinesite
Copy link
Contributor

fixes #185


"cmake/MaterialXConfig.cmake",
"cmake/MaterialXConfig-noconfig.cmake",
"cmake/MaterialXConfigVersion.cmake"
Copy link
Member

Choose a reason for hiding this comment

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

Missing comma means we're not including this file or the one after it.

"lib/libMaterialXCore.so",
"lib/libMaterialXFormat.so.1.38.0",
"lib/libMaterialXFormat.so.1",
"lib/libMaterialXFormat.so"
Copy link
Member

Choose a reason for hiding this comment

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

Another missing comma. If I fix these then I can import the Python bindings.

"lib/libMaterialXGenGlsl.so.1.38.0",
"lib/libMaterialXGenGlsl.so.1",
"lib/libMaterialXGenGlsl.so",
"lib/libMaterialXGenOsl.so.1.38.0",
Copy link
Member

Choose a reason for hiding this comment

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

.so is the extension on Linux, but it'll be .dylib on Mac. Including the version numbers is going to make maintenance harder than necessary too. Let's do as we do for other projects and use a catch-all lib/libMaterialX*{sharedLibraryExtension}*.

Comment on lines 86 to 87
"cmake/MaterialXConfig.cmake",
"cmake/MaterialXConfig-noconfig.cmake",
Copy link
Member

Choose a reason for hiding this comment

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

We don't include CMake files for any other projects so let's omit these for now.

Comment on lines 116 to 124
"include/MaterialXCore",
"include/MaterialXFormat",
"include/MaterialXGenShader",
"include/MaterialXGenGlsl",
"include/MaterialXGenOsl",
"include/MaterialXRender",
"include/MaterialXRenderOsl",
"include/MaterialXRenderHw",
"include/MaterialXRenderGlsl",
Copy link
Member

Choose a reason for hiding this comment

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

Likewise, let's use a catch-all include/MaterialX* here, to simplify future maintenance.

Comment on lines 125 to 132
"resources/Geometry",
"resources/Images",
"resources/Lights",
"resources/Materials",
"libraries/bxdf",
"libraries/lights",
"libraries/pbrlib",
"libraries/stdlib",
Copy link
Member

Choose a reason for hiding this comment

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

It would be great if we could install these in a subfolder somehow, to make it clear that they're MaterialX things and not Gaffer things. Do you know if that is possible? Also, are the resources perhaps just for use by the viewer (which it seems we're not building)?

@johnhaddon
Copy link
Member

Sorry, one more comment - could you add - MaterialX : Added version 1.38.0 to Changes.md please?

@johnhaddon
Copy link
Member

I tried building the latest MaterialX with this, and some settings and flags have changed. Closing in favour of some future PR based on a local branch I've got...

@johnhaddon johnhaddon closed this Feb 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include MaterialX
2 participants