Skip to content

Fix enumeration interfacename bug #2423

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

Merged
ld-kerley merged 2 commits into from
Jun 3, 2025
Merged

Fix enumeration interfacename bug #2423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9e755e1
Fix case where an enumerated input is driven by an interfacename.
ld-kerley May 29, 2025
d5ed3f8
add a unit test
ld-kerley May 29, 2025
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
30 changes: 30 additions & 0 deletions resources/Materials/TestSuite/stdlib/definition/definition_with_enum_interface.mtlx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<?xml version="1.0"?>
<materialx version="1.39">

<!-- create a custom node that exposes an enumeration input via "interfacename" -->
<nodedef name="ND_positionwrapper" node="positionwrapper">
<input name="wrap_space" type="string" value="object" enum="model, object, world" uniform="true"/>
<output name="out" type="vector3"/>
</nodedef>
<nodegraph name="NG_positionwrapper" nodedef="ND_positionwrapper">
<position name="myPos" type="vector3">
<input name="space" type="string" interfacename="wrap_space"/>
</position>
<output name="out" type="vector3" nodename="myPos"/>
</nodegraph>

<positionwrapper name="myPosWrapper" type="vector3">
<input name="wrap_space" type="string" value="object"/>
</positionwrapper>

<extract name="extract" type="float">
<input name="in" type="vector3" nodename="myPosWrapper"/>
<input name="index" type="integer" value="1"/>
</extract>
<surface_unlit name="Srf" type="surfaceshader">
<input name="emission" type="float" nodename="extract" />
</surface_unlit>
<surfacematerial name="Mtl" type="material">
<input name="surfaceshader" type="surfaceshader" nodename="Srf" />
</surfacematerial>
</materialx>
23 changes: 14 additions & 9 deletions source/MaterialXGenShader/ShaderNode.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,16 +353,21 @@ void ShaderNode::initialize(const Node& node, const NodeDef& nodeDef, GenContext
}
}
const string& valueString = portValue ? portValue->getValueString() : EMPTY_STRING;
std::pair<TypeDesc, ValuePtr> enumResult;
const string& enumNames = nodeDefInput->getAttribute(ValueElement::ENUM_ATTRIBUTE);
const TypeDesc type = context.getTypeDesc(nodeDefInput->getType());
if (context.getShaderGenerator().getSyntax().remapEnumeration(valueString, type, enumNames, enumResult))

@kwokcb kwokcb Jun 3, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The "bug" appears to be in GLSL and MSL remapEnumeration method. It is returning true when there is no value mapped. MDL returns the correct value of false in this case.

@ld-kerley ld-kerley Jun 3, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Chatted offline with Bernard - we still think the original fix is valid - but I'm gonna put up a PR to fix the return value from remapEnumeration as well.

{
input->setValue(enumResult.second);
}
else if (!valueString.empty())
if (!valueString.empty())
{
input->setValue(portValue);
// We explicitly check the valueString is not empty before checking the enumeration,
// because otherwise the enumeration value would always return nullptr
std::pair<TypeDesc, ValuePtr> enumResult;
const string& enumNames = nodeDefInput->getAttribute(ValueElement::ENUM_ATTRIBUTE);
const TypeDesc type = context.getTypeDesc(nodeDefInput->getType());
if (context.getShaderGenerator().getSyntax().remapEnumeration(valueString, type, enumNames, enumResult))
{
input->setValue(enumResult.second);
}
else
{
input->setValue(portValue);
}
}
}
}
Expand Down