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

LOOKDEVX-2124 - MaterialX Topo Handler #3445

Merged
merged 3 commits into from
Nov 7, 2023

Conversation

JGamache-autodesk
Copy link
Collaborator

Implements a topo neutral graph generator. This allows detecting that two native MaterialX shading graphs are topologically equivalent, generate universal shading code, and provide the right mapping to find the original node name corresponding to a renamed node.

Also adds proper exports to allow rebuilding the old MaterialX Maya surface node using this new functionnality. This also requires fixing the generated code to properly handle the "texcoord" nodes variables used in the shader.

Implements a topo neutral graph generator. This allows detecting that
two native MaterialX shading graphs are topologically equivalent,
generate universal shading code, and provide the right mapping to find
the original node name corresponding to a renamed node.

Also adds proper exports to allow rebuilding the old MaterialX Maya
surface node using this new functionnality. This also requires fixing
the generated code to properly handle the "texcoord" nodes variables
used in the shader.
@@ -65,6 +67,10 @@ mayaUsd_promoteHeaderList(HEADERS ${HEADERS} SUBDIR render/MaterialXGenOgsXml)
# install
# -----------------------------------------------------------------------------

install(FILES ${HEADERS}
DESTINATION ${CMAKE_INSTALL_PREFIX}/include/${PROJECT_NAME}/render/MaterialXGenOgsXml
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Allows LookdevX to used the shadergen code.

static const std::regex primvarParamRegex(primvarParamSource.c_str());
static const std::string texcoordParamSource
= "vec([23]) [$](" + d(HW::T_TEXCOORD) + "_[0-9]+)";
static const std::regex texcoordParamRegex(texcoordParamSource.c_str());
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As you can see above, we already had code that cleaned up shader stream variable names that were generated for "geompropvalue" nodes. We need to do the same for variable names generated for the "texcoor"nodes used in LookdevX.

@@ -462,7 +462,7 @@ bool OgsFragment::isElementAShader() const

bool OgsFragment::isTransparent() const
{
return _glslShader && _glslShader->hasAttribute(mx::HW::ATTR_TRANSPARENT);
return isTransparentSurface(_element, mx::GlslShaderGenerator::TARGET);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Using native MaterialX call.

@@ -16,7 +18,7 @@ namespace MaterialXMaya {
/// and outputs and embedding source code in one or potentially multiple target
/// shading languages (GLSL is the only such language currently supported).
///
class OgsFragment
class MAYAUSD_CORE_PUBLIC OgsFragment
{
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Allow these classes to be used in LookdevX.

const std::string SURFACEMATERIAL_CATEGORY("surfacematerial");
const std::string SURFACESHADER_TYPE("surfaceshader");

const std::set<std::string> _mtlxTopoNodeSet = {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved from material.cpp


TopoNeutralGraph::TopoNeutralGraph(const mx::ElementPtr& material)
{
if (!material) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Having a look at the test cases really helps understand what this code is doing.

mx::NodePtr TopoNeutralGraph::cloneNode(const mx::Node& node, mx::GraphElement& container)
{
auto destNode
= container.addNode(node.getCategory(), "N" + std::to_string(_nodeIndex), node.getType());
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All cloned nodes get incremental names starting at N0. This allows merging graphs that have the same nodes and connectivity, but differs in node names.

</constant>
<output name="O0" type="float" channels="g" nodename="N2" />
</nodegraph>
</materialx>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are the output graphs from a topo neutral cloning. As you can see the ordering of the nodes, outputs, and names is extremely consistent, allowing comparing two of those graphs for equality.

</standard_surface>
<surfacematerial name="Channel1" type="material" topo="Channel1_topo.mtlx">
<input name="surfaceshader" type="surfaceshader" nodename="Surf1" />
</surfacematerial>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are all the test cases, the "topo" attribute on the "surfacematerial" node indicates which file contains the output we seek. As you can see quite a few of these graphs will end up with the same baseline topo result.

vlasovi
vlasovi previously approved these changes Nov 7, 2023
Copy link
Collaborator

@vlasovi vlasovi left a comment

Choose a reason for hiding this comment

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

Looks good

vlasovi
vlasovi previously approved these changes Nov 7, 2023
feldstj
feldstj previously approved these changes Nov 7, 2023
Copy link
Collaborator

@feldstj feldstj left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple of minor comments.

auto destNode
= container.addNode(node.getCategory(), "N" + std::to_string(_nodeIndex), node.getType());
++_nodeIndex;
_nodeMap.insert({ node.getNamePath(), destNode });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like a small bug here. The second identical insert should be removed.

if (materialNode->getType() != SURFACESHADER_TYPE) {
throw mx::Exception("Material shader node is not a surfaceshader.");
}
auto dupMaterial = _doc->addMaterialNode("N" + std::to_string(_nodeIndex));
Copy link
Collaborator

Choose a reason for hiding this comment

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

(super minor) - Not sure if it's a touch more intuitive to do _nodeIndex++ here instead of the later ++_nodeIndex. I think I saw this elsewhere as well. Just personal preference perhaps.

@JGamache-autodesk JGamache-autodesk added the ready-for-merge Development process is finished, PR is ready for merge label Nov 7, 2023
@JGamache-autodesk
Copy link
Collaborator Author

@seando-adsk ready for merge.

@seando-adsk seando-adsk merged commit e5500a0 into dev Nov 7, 2023
@seando-adsk seando-adsk deleted the gamaj/LOOKDEVX-2124/materialx_topo_handler branch November 7, 2023 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge Development process is finished, PR is ready for merge vp2renderdelegate Related to VP2RenderDelegate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants