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

Cleanup old unused code for conditional nodes #1344

Merged
merged 4 commits into from
Apr 26, 2023
Merged

Cleanup old unused code for conditional nodes #1344

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
0df88cf
Cleanup old unused code for handling of conditional nodes.
niklasharrysson Apr 19, 2023
e660d9d
Cleanup additional left-over code for handling of conditional nodes.
niklasharrysson Apr 19, 2023
422c368
Merge remote-tracking branch 'aswf/main' into code_cleanup
niklasharrysson Apr 25, 2023
fdc543e
Added wrapper to keep but deprecate the old method
niklasharrysson Apr 26, 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
5 changes: 2 additions & 3 deletions source/MaterialXGenGlsl/GlslShaderGenerator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,11 +336,10 @@ void GlslShaderGenerator::emitVertexStage(const ShaderGraph& graph, GenContext&
emitLine("vec4 hPositionWorld = " + HW::T_WORLD_MATRIX + " * vec4(" + HW::T_IN_POSITION + ", 1.0)", stage);
emitLine("gl_Position = " + HW::T_VIEW_PROJECTION_MATRIX + " * hPositionWorld", stage);

// For vertex stage just emit all function calls in order
// and ignore conditional scope.
// Emit all function calls in order
for (const ShaderNode* node : graph.getNodes())
{
emitFunctionCall(*node, context, stage, false);
emitFunctionCall(*node, context, stage);
}

emitFunctionBodyEnd(graph, context, stage);
Expand Down
2 changes: 1 addition & 1 deletion source/MaterialXGenGlsl/Nodes/LightSamplerNodeGlsl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ void LightSamplerNodeGlsl::emitFunctionDefinition(const ShaderNode& node, GenCon
{
shadergen.emitLine(ifstatement + "(light.type == " + std::to_string(it.first) + ")", stage, false);
shadergen.emitScopeBegin(stage);
shadergen.emitFunctionCall(*it.second, context, stage, false);
shadergen.emitFunctionCall(*it.second, context, stage);
shadergen.emitScopeEnd(stage);
ifstatement = "else if ";
}
Expand Down
6 changes: 3 additions & 3 deletions source/MaterialXGenMsl/MslShaderGenerator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -692,11 +692,11 @@ void MslShaderGenerator::emitVertexStage(const ShaderGraph& graph, GenContext& c
emitFunctionCalls(graph, context, stage);
emitLineBreak(stage);
emitLine("return " + vertexData.getInstance(), stage, true);
// For vertex stage just emit all function calls in order
// and ignore conditional scope.

// Emit all function calls in order
for (const ShaderNode* node : graph.getNodes())
{
emitFunctionCall(*node, context, stage, false);
emitFunctionCall(*node, context, stage);
}

emitFunctionBodyEnd(graph, context, stage);
Expand Down
2 changes: 1 addition & 1 deletion source/MaterialXGenMsl/Nodes/LightSamplerNodeMsl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ void LightSamplerNodeMsl::emitFunctionDefinition(const ShaderNode& node, GenCont
{
shadergen.emitLine(ifstatement + "(light.type == " + std::to_string(it.first) + ")", stage, false);
shadergen.emitScopeBegin(stage);
shadergen.emitFunctionCall(*it.second, context, stage, false);
shadergen.emitFunctionCall(*it.second, context, stage);
shadergen.emitScopeEnd(stage);
ifstatement = "else if ";
}
Expand Down
2 changes: 1 addition & 1 deletion source/MaterialXGenOsl/OslShaderGenerator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,7 +444,7 @@ void OslShaderGenerator::emitFunctionCalls(const ShaderGraph& graph, GenContext&
const ShaderNode* upstream = outputSocket->getConnection() ? outputSocket->getConnection()->getNode() : nullptr;
if (upstream && upstream->hasClassification(classification))
{
emitFunctionCall(*upstream, context, stage, false);
emitFunctionCall(*upstream, context, stage);
}
}
}
Expand Down
11 changes: 1 addition & 10 deletions source/MaterialXGenShader/HwShaderGenerator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,7 @@ ShaderPtr HwShaderGenerator::createShader(const string& name, ElementPtr element
return shader;
}

void HwShaderGenerator::emitFunctionCall(const ShaderNode& node, GenContext& context, ShaderStage& stage, bool checkScope) const
void HwShaderGenerator::emitFunctionCall(const ShaderNode& node, GenContext& context, ShaderStage& stage) const
{
// Check if it's emitted already.
if (stage.isEmitted(node, context))
Expand All @@ -465,15 +465,6 @@ void HwShaderGenerator::emitFunctionCall(const ShaderNode& node, GenContext& con
return;
}

// Omit node if it's only used inside a conditional branch
if (checkScope && node.referencedConditionally())
{
emitComment("Omitted node '" + node.getName() + "'. Only used in conditional node '" +
node.getScopeInfo().conditionalNode->getName() + "'",
stage);
return;
}

bool match = true;

if (node.hasClassification(ShaderNode::Classification::CLOSURE) && !node.hasClassification(ShaderNode::Classification::SHADER))
Expand Down
3 changes: 1 addition & 2 deletions source/MaterialXGenShader/HwShaderGenerator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,8 +286,7 @@ class MX_GENSHADER_API HwShaderGenerator : public ShaderGenerator
{
public:
/// Add the function call for a single node.
void emitFunctionCall(const ShaderNode& node, GenContext& context, ShaderStage& stage,
bool checkScope = true) const override;
void emitFunctionCall(const ShaderNode& node, GenContext& context, ShaderStage& stage) const override;

/// Emit code for active light count definitions and uniforms
virtual void addStageLightingUniforms(GenContext& context, ShaderStage& stage) const;
Expand Down
13 changes: 0 additions & 13 deletions source/MaterialXGenShader/Nodes/SwitchNode.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ void SwitchNode::emitFunctionCall(const ShaderNode& node, GenContext& context, S
DEFINE_SHADER_STAGE(stage, Stage::PIXEL)
{
const ShaderGenerator& shadergen = context.getShaderGenerator();
const ShaderGraph& graph = *node.getParent();

// Declare the output variable
shadergen.emitLineBegin(stage);
Expand Down Expand Up @@ -58,23 +57,11 @@ void SwitchNode::emitFunctionCall(const ShaderNode& node, GenContext& context, S
shadergen.emitLineEnd(stage, false);

shadergen.emitScopeBegin(stage);

// Emit nodes that are ONLY needed in this scope
for (const ShaderNode* otherNode : graph.getNodes())
{
const ShaderNode::ScopeInfo& scope = otherNode->getScopeInfo();
if (scope.conditionalNode == &node && scope.usedByBranch(branch))
{
shadergen.emitFunctionCall(*otherNode, context, stage, false);
}
}

shadergen.emitLineBegin(stage);
shadergen.emitOutput(node.getOutput(), false, false, context, stage);
shadergen.emitString(" = ", stage);
shadergen.emitInput(input, context, stage);
shadergen.emitLineEnd(stage);

shadergen.emitScopeEnd(stage);
}
}
Expand Down
23 changes: 9 additions & 14 deletions source/MaterialXGenShader/ShaderGenerator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,24 +101,19 @@ void ShaderGenerator::emitFunctionDefinitions(const ShaderGraph& graph, GenConte
}
}

void ShaderGenerator::emitFunctionCall(const ShaderNode& node, GenContext& context, ShaderStage& stage,
bool checkScope) const
void ShaderGenerator::emitFunctionCall(const ShaderNode& node, GenContext& context, ShaderStage& stage) const
{
// Check if it's emitted already.
if (stage.isEmitted(node, context))
if (!stage.isEmitted(node, context))
{
// emitComment("Omitted node '" + node.getName() + "'. Already called above.", stage);
return;
stage.addFunctionCall(node, context);
}
// Omit node if it's only used inside a conditional branch
if (checkScope && node.referencedConditionally())
{
emitComment("Omitted node '" + node.getName() + "'. Only used in conditional node '" +
node.getScopeInfo().conditionalNode->getName() + "'",
stage);
return;
}
stage.addFunctionCall(node, context);
}

// Wrapper for deprecated version of this method.
void ShaderGenerator::emitFunctionCall(const ShaderNode& node, GenContext& context, ShaderStage& stage, bool /*checkScope*/) const
{
emitFunctionCall(node, context, stage);
}

void ShaderGenerator::emitFunctionCalls(const ShaderGraph& graph, GenContext& context, ShaderStage& stage, uint32_t classification) const
Expand Down
3 changes: 2 additions & 1 deletion source/MaterialXGenShader/ShaderGenerator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,8 @@ class MX_GENSHADER_API ShaderGenerator
virtual void emitFunctionDefinitions(const ShaderGraph& graph, GenContext& context, ShaderStage& stage) const;

/// Add the function call for a single node.
virtual void emitFunctionCall(const ShaderNode& node, GenContext& context, ShaderStage& stage, bool checkScope = true) const;
virtual void emitFunctionCall(const ShaderNode& node, GenContext& context, ShaderStage& stage) const;
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 parameter has no effect since the feature is not in use. But note that this is a change to shader generation API.

Since there is a default value on the parameter calls to the API are not effected, and it's only derived shader generators that my be affected by this change, if they happen to override this function. I suspect that's very rare. But let me know if this API breakage needs to be handled with more care, and we should keep the parameter in for now.

Copy link
Member

Choose a reason for hiding this comment

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

@niklasharrysson The approach I've been taking for backwards compatibility is to add a deprecated wrapper for the original function interface, so that existing clients can continue to build their code with the latest version of MaterialX.

Now that we've set C++14 as the baseline for MaterialX, we can additionally mark these wrappers with the [[deprecated]] keyword, so that clients receive a warning at compile time. Here's an example of this pattern in the current codebase:

MaterialX/source/MaterialXGenShader/Util.h

Line 93 in 7fa59fa

MX_GENSHADER_API [[deprecated]] void findRenderableMaterialNodes(ConstDocumentPtr doc, vector<TypedElementPtr>& elements, bool, std::unordered_set<ElementPtr>&);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a great pattern to follow. I have added a wrapper now.

[[deprecated]] virtual void emitFunctionCall(const ShaderNode& node, GenContext& context, ShaderStage& stage, bool checkScope) const;

/// Add all function calls for a graph. If a classification mask is given only functions for
/// nodes matching this classification will be emitted.
Expand Down
76 changes: 0 additions & 76 deletions source/MaterialXGenShader/ShaderGraph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -959,13 +959,6 @@ void ShaderGraph::finalize(GenContext& context)
// Sort the nodes in topological order.
topologicalSort();

// Calculate scopes for all nodes in the graph.
//
// TODO: Enable calculateScopes() again when support for
// conditional nodes are improved.
//
// calculateScopes();
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 is where the feature where disabled a long time ago by not performing the scope calculations.


if (context.getOptions().shaderInterfaceType == SHADER_INTERFACE_COMPLETE)
{
// Publish all node inputs that has not been connected already.
Expand Down Expand Up @@ -1235,75 +1228,6 @@ void ShaderGraph::topologicalSort()
}
}

void ShaderGraph::calculateScopes()
{
//
// Calculate scopes for all nodes, considering branching from conditional nodes
//
// TODO: Refactor the scope handling, using scope id's instead
//

if (_nodeOrder.empty())
{
return;
}

size_t lastNodeIndex = _nodeOrder.size() - 1;
ShaderNode* lastNode = _nodeOrder[lastNodeIndex];
lastNode->getScopeInfo().type = ShaderNode::ScopeInfo::GLOBAL;

std::set<ShaderNode*> nodeUsed;
nodeUsed.insert(lastNode);

// Iterate nodes in reversed toplogical order such that every node is visited AFTER
// each of the nodes that depend on it have been processed first.
for (int nodeIndex = int(lastNodeIndex); nodeIndex >= 0; --nodeIndex)
{
ShaderNode* node = _nodeOrder[nodeIndex];

// Once we visit a node the scopeInfo has been determined and it will not be changed
// By then we have visited all the nodes that depend on it already
if (nodeUsed.count(node) == 0)
{
continue;
}

const bool isIfElse = node->hasClassification(ShaderNode::Classification::IFELSE);
const bool isSwitch = node->hasClassification(ShaderNode::Classification::SWITCH);

const ShaderNode::ScopeInfo& currentScopeInfo = node->getScopeInfo();

for (size_t inputIndex = 0; inputIndex < node->numInputs(); ++inputIndex)
{
ShaderInput* input = node->getInput(inputIndex);

if (input->getConnection())
{
ShaderNode* upstreamNode = input->getConnection()->getNode();

// Create scope info for this network brach
// If it's a conditonal branch the scope is adjusted
ShaderNode::ScopeInfo newScopeInfo = currentScopeInfo;
if (isIfElse && (inputIndex == 2 || inputIndex == 3))
{
newScopeInfo.adjustAtConditionalInput(node, int(inputIndex), 0x12);
}
else if (isSwitch && inputIndex != node->numInputs() - 1)
{
const uint32_t fullMask = (1 << node->numInputs()) - 1;
newScopeInfo.adjustAtConditionalInput(node, int(inputIndex), fullMask);
}

// Add the info to the upstream node
ShaderNode::ScopeInfo& upstreamScopeInfo = upstreamNode->getScopeInfo();
upstreamScopeInfo.merge(newScopeInfo);

nodeUsed.insert(upstreamNode);
}
}
}
}

void ShaderGraph::setVariableNames(GenContext& context)
{
// Make sure inputs and outputs have variable names valid for the
Expand Down
3 changes: 0 additions & 3 deletions source/MaterialXGenShader/ShaderGraph.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,9 +167,6 @@ class MX_GENSHADER_API ShaderGraph : public ShaderNode
/// with the output's downstream connections.
void bypass(GenContext& context, ShaderNode* node, size_t inputIndex, size_t outputIndex = 0);

/// Calculate scopes for all nodes in the graph
void calculateScopes();

/// For inputs and outputs in the graph set the variable names to be used
/// in generated code. Making sure variable names are valid and unique
/// to avoid name conflicts during shader generation.
Expand Down
62 changes: 0 additions & 62 deletions source/MaterialXGenShader/ShaderNode.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,68 +159,6 @@ ShaderNode::ShaderNode(const ShaderGraph* parent, const string& name) :
{
}

bool ShaderNode::referencedConditionally() const
{
if (_scopeInfo.type == ShaderNode::ScopeInfo::SINGLE)
{
int numBranches = 0;
uint32_t mask = _scopeInfo.conditionBitmask;
for (; mask != 0; mask >>= 1)
{
if (mask & 1)
{
numBranches++;
}
}
return numBranches > 0;
}
return false;
}

void ShaderNode::ScopeInfo::adjustAtConditionalInput(ShaderNode* condNode, int branch, uint32_t fullMask)
{
if (type == ScopeInfo::GLOBAL || (type == ScopeInfo::SINGLE && conditionBitmask == fullConditionMask))
{
type = ScopeInfo::SINGLE;
conditionalNode = condNode;
conditionBitmask = 1 << branch;
fullConditionMask = fullMask;
}
else if (type == ScopeInfo::SINGLE)
{
type = ScopeInfo::MULTIPLE;
conditionalNode = nullptr;
}
}

void ShaderNode::ScopeInfo::merge(const ScopeInfo& fromScope)
{
if (type == ScopeInfo::UNKNOWN || fromScope.type == ScopeInfo::GLOBAL)
{
*this = fromScope;
}
else if (type == ScopeInfo::GLOBAL)
{
}
else if (type == ScopeInfo::SINGLE && fromScope.type == ScopeInfo::SINGLE && conditionalNode == fromScope.conditionalNode)
{
conditionBitmask |= fromScope.conditionBitmask;

// This node is needed for all branches so it is no longer conditional
if (conditionBitmask == fullConditionMask)
{
type = ScopeInfo::GLOBAL;
conditionalNode = nullptr;
}
}
else
{
// NOTE: Right now multiple scopes is not really used, it works exactly as ScopeInfo::GLOBAL
type = ScopeInfo::MULTIPLE;
conditionalNode = nullptr;
}
}

ShaderNodePtr ShaderNode::create(const ShaderGraph* parent, const string& name, const NodeDef& nodeDef, GenContext& context)
{
ShaderNodePtr newNode = std::make_shared<ShaderNode>(parent, name);
Expand Down
Loading