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

Refine dot node elision and remove unused code #1522

Merged
merged 4 commits into from
Sep 8, 2023
Merged

Refine dot node elision and remove unused code #1522

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6510b97
Refined the dot elision code. Fixed invalid switch code. Removed dead…
JGamache-autodesk Sep 8, 2023
98dcacc
Merge branch 'main' into gamaj/refine_dot_elision
JGamache-autodesk Sep 8, 2023
5aecd59
Add unit testing for filename dot node elision
JGamache-autodesk Sep 8, 2023
857ab86
Move filename dot unit test to proper location.
JGamache-autodesk Sep 8, 2023
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
10 changes: 10 additions & 0 deletions resources/Materials/TestSuite/stdlib/organization/organization.mtlx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,4 +90,14 @@
</transformmatrix>
<output name="out" type="vector4" nodename="transformvector1" />
</nodegraph>
<nodegraph name="dot_filename">
<dot name="dot1" type="filename">
<input name="in" type="filename" value="resources/Images/grid.png" />
<input name="note" type="string" value="Checking for spurious samplers" />
</dot>
<image name="image_color3" type="color3">
<input name="file" type="filename" nodename="dot1" />
</image>
<output name="out" type="color3" nodename="image_color3" />
</nodegraph>
</materialx>
40 changes: 4 additions & 36 deletions source/MaterialXGenShader/ShaderGraph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1027,48 +1027,16 @@ void ShaderGraph::optimize(GenContext& context)
}
else if (node->hasClassification(ShaderNode::Classification::DOT))
{
// Dot nodes without modifiers can be elided by moving their connection downstream.
// Filename dot nodes must be elided so they do not create extra samplers.
ShaderInput* in = node->getInput("in");
if (in->getChannels().empty())
if (in->getChannels().empty() && in->getType() == Type::FILENAME)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still solves the issue with extra samplers, but preserves the distinction between "dot" nodes and "constant" nodes.

{
bypass(context, node, 0);
++numEdits;
}
}
else if (node->hasClassification(ShaderNode::Classification::IFELSE))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This used to target a node called "compare" which was removed in 1.37.

{
// Check if we have a constant conditional expression
ShaderInput* intest = node->getInput("intest");
if (!intest->getConnection())
{
// Find which branch should be taken
ShaderInput* cutoff = node->getInput("cutoff");
ValuePtr value = intest->getValue();
const float intestValue = value ? value->asA<float>() : 0.0f;
const int branch = (intestValue <= cutoff->getValue()->asA<float>() ? 2 : 3);

// Bypass the conditional using the taken branch
bypass(context, node, branch);

++numEdits;
}
}
else if (node->hasClassification(ShaderNode::Classification::SWITCH))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This worked in 1.37 where the "which" input was a "parameter" instead of an "input". Since this input is not marked as "uniform", it should not be elided.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be restored as it checks to see if indeed a uniform unconnected value. I have log an new issue and can discuss there, but now new shaders will always add more code in. (#1526)

{
// Check if we have a constant conditional expression
const ShaderInput* which = node->getInput("which");
if (!which->getConnection())
{
// Find which branch should be taken
ValuePtr value = which->getValue();
const int branch = int(value == nullptr ? 0 : (which->getType() == Type::FLOAT ? value->asA<float>() : value->asA<int>()));

// Bypass the conditional using the taken branch
bypass(context, node, branch);

++numEdits;
}
}
// Adding more nodes here requires them to have an input that is tagged
// "uniform" in the NodeDef or to handle very specific cases, like FILENAME.
}

if (numEdits > 0)
Expand Down
10 changes: 0 additions & 10 deletions source/MaterialXGenShader/ShaderNode.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,6 @@ const ShaderNodePtr ShaderNode::NONE = createEmptyNode();
const string ShaderNode::CONSTANT = "constant";
const string ShaderNode::DOT = "dot";
const string ShaderNode::IMAGE = "image";
const string ShaderNode::COMPARE = "compare";
const string ShaderNode::SWITCH = "switch";
const string ShaderNode::SURFACESHADER = "surfaceshader";
const string ShaderNode::SCATTER_MODE = "scatter_mode";
const string ShaderNode::BSDF_R = "R";
Expand Down Expand Up @@ -292,14 +290,6 @@ ShaderNodePtr ShaderNode::create(const ShaderGraph* parent, const string& name,
{
newNode->_classification = Classification::TEXTURE | Classification::DOT;
}
else if (nodeDef.getNodeString() == COMPARE)
{
newNode->_classification = Classification::TEXTURE | Classification::CONDITIONAL | Classification::IFELSE;
}
else if (nodeDef.getNodeString() == SWITCH)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure way it means to remove the texture classification here. I think nodes like try to perform filtered sample upstream will no longer work. But it could be that it does not work anyways.

{
newNode->_classification = Classification::TEXTURE | Classification::CONDITIONAL | Classification::SWITCH;
}
// Third, check for file texture classification by group name
else if (groupName == TEXTURE2D_GROUPNAME || groupName == TEXTURE3D_GROUPNAME)
{
Expand Down
13 changes: 4 additions & 9 deletions source/MaterialXGenShader/ShaderNode.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,23 +350,18 @@ class MX_GENSHADER_API ShaderNode
static const uint32_t VOLUME = 1 << 15; /// A volume shader node
static const uint32_t LIGHT = 1 << 16; /// A light shader node
static const uint32_t UNLIT = 1 << 17; /// An unlit surface shader node
// Specific conditional types
static const uint32_t IFELSE = 1 << 18; /// An if-else statement
static const uint32_t SWITCH = 1 << 19; /// A switch statement
// Types based on nodegroup
static const uint32_t SAMPLE2D = 1 << 20; /// Can be sampled in 2D (uv space)
static const uint32_t SAMPLE3D = 1 << 21; /// Can be sampled in 3D (position)
static const uint32_t GEOMETRIC = 1 << 22; /// Geometric input
static const uint32_t DOT = 1 << 23; /// A dot node
static const uint32_t SAMPLE2D = 1 << 18; /// Can be sampled in 2D (uv space)
static const uint32_t SAMPLE3D = 1 << 19; /// Can be sampled in 3D (position)
static const uint32_t GEOMETRIC = 1 << 20; /// Geometric input
static const uint32_t DOT = 1 << 21; /// A dot node
};

static const ShaderNodePtr NONE;

static const string CONSTANT;
static const string DOT;
static const string IMAGE;
static const string COMPARE;
static const string SWITCH;
static const string SURFACESHADER;
static const string SCATTER_MODE;
static const string BSDF_R;
Expand Down