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

Fix edge case in HW shader generation for texcoords #1559

Merged
merged 7 commits into from
Oct 13, 2023
Merged

Fix edge case in HW shader generation for texcoords #1559

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0772867
Fix issue #1464
madmann91 Oct 11, 2023
f5437f1
Add test for texcoord bug #1464.
madmann91 Oct 12, 2023
07270d5
Use static string for `index` port name.
madmann91 Oct 13, 2023
e8f3cd0
Simplify swizzling logic for texcoord nodes.
madmann91 Oct 13, 2023
9209855
Add checks to prevent incorrect port widening.
madmann91 Oct 13, 2023
0160273
Remove extra newline
jstone-lucasfilm Oct 13, 2023
35b1674
Minor formatting update
jstone-lucasfilm Oct 13, 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
25 changes: 25 additions & 0 deletions resources/Materials/TestSuite/stdlib/texture/texcoord.mtlx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<?xml version="1.0"?>
jstone-lucasfilm marked this conversation as resolved.
Show resolved Hide resolved
<materialx version="1.38" colorspace="lin_rec709">
<!--
Test for texture coordinates. This test makes sure that two texture coordinate nodes with various
widths can coexist.
-->
<standard_surface name="standard_surface" type="surfaceshader" xpos="6.152174" ypos="-0.560345">
<input name="base" type="float" value="1" />
<input name="base_color" type="color3" nodename="swizzle_vector2_color3" />
<input name="specular_roughness" type="float" value="0.1" />
<input name="subsurface" type="float" value="0.4" />
<input name="subsurface_color" type="color3" nodename="swizzle_vector3_color3" />
</standard_surface>
<surfacematerial name="surfacematerial" type="material" xpos="8.695652" ypos="0.000000">
<input name="surfaceshader" type="surfaceshader" nodename="standard_surface" />
</surfacematerial>
<texcoord name="texcoord_vector2" type="vector2" xpos="1.666667" ypos="-1.405172" />
<texcoord name="texcoord_vector3" type="vector3" xpos="0.268116" ypos="1.068966" />
<swizzle name="swizzle_vector3_color3" type="color3" xpos="3.101449" ypos="0.698276">
<input name="in" type="vector3" nodename="texcoord_vector3" />
</swizzle>
<swizzle name="swizzle_vector2_color3" type="color3" xpos="4.260870" ypos="-2.896552">
<input name="in" type="vector2" nodename="texcoord_vector2" />
</swizzle>
</materialx>
6 changes: 3 additions & 3 deletions source/MaterialXGenGlsl/GlslShaderGenerator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
#include <MaterialXGenGlsl/Nodes/NormalNodeGlsl.h>
#include <MaterialXGenGlsl/Nodes/TangentNodeGlsl.h>
#include <MaterialXGenGlsl/Nodes/BitangentNodeGlsl.h>
#include <MaterialXGenGlsl/Nodes/TexCoordNodeGlsl.h>
#include <MaterialXGenGlsl/Nodes/GeomColorNodeGlsl.h>
#include <MaterialXGenGlsl/Nodes/GeomPropValueNodeGlsl.h>
#include <MaterialXGenGlsl/Nodes/FrameNodeGlsl.h>
Expand All @@ -34,6 +33,7 @@
#include <MaterialXGenShader/Nodes/CombineNode.h>
#include <MaterialXGenShader/Nodes/SwitchNode.h>
#include <MaterialXGenShader/Nodes/HwImageNode.h>
#include <MaterialXGenShader/Nodes/HwTexCoordNode.h>
#include <MaterialXGenShader/Nodes/ClosureSourceCodeNode.h>
#include <MaterialXGenShader/Nodes/ClosureCompoundNode.h>
#include <MaterialXGenShader/Nodes/ClosureLayerNode.h>
Expand Down Expand Up @@ -174,8 +174,8 @@ GlslShaderGenerator::GlslShaderGenerator() :
// <!-- <bitangent> -->
registerImplementation("IM_bitangent_vector3_" + GlslShaderGenerator::TARGET, BitangentNodeGlsl::create);
// <!-- <texcoord> -->
registerImplementation("IM_texcoord_vector2_" + GlslShaderGenerator::TARGET, TexCoordNodeGlsl::create);
registerImplementation("IM_texcoord_vector3_" + GlslShaderGenerator::TARGET, TexCoordNodeGlsl::create);
registerImplementation("IM_texcoord_vector2_" + GlslShaderGenerator::TARGET, HwTexCoordNode::create);
registerImplementation("IM_texcoord_vector3_" + GlslShaderGenerator::TARGET, HwTexCoordNode::create);
// <!-- <geomcolor> -->
registerImplementation("IM_geomcolor_float_" + GlslShaderGenerator::TARGET, GeomColorNodeGlsl::create);
registerImplementation("IM_geomcolor_color3_" + GlslShaderGenerator::TARGET, GeomColorNodeGlsl::create);
Expand Down
2 changes: 1 addition & 1 deletion source/MaterialXGenGlsl/GlslShaderGenerator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class MX_GENGLSL_API GlslShaderGenerator : public HwShaderGenerator
ShaderNodeImplPtr getImplementation(const NodeDef& nodedef, GenContext& context) const override;

/// Determine the prefix of vertex data variables.
virtual string getVertexDataPrefix(const VariableBlock& vertexData) const;
string getVertexDataPrefix(const VariableBlock& vertexData) const override;

public:
/// Unique identifier for this generator target
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 @@ -10,7 +10,6 @@
#include <MaterialXGenMsl/Nodes/NormalNodeMsl.h>
#include <MaterialXGenMsl/Nodes/TangentNodeMsl.h>
#include <MaterialXGenMsl/Nodes/BitangentNodeMsl.h>
#include <MaterialXGenMsl/Nodes/TexCoordNodeMsl.h>
#include <MaterialXGenMsl/Nodes/GeomColorNodeMsl.h>
#include <MaterialXGenMsl/Nodes/GeomPropValueNodeMsl.h>
#include <MaterialXGenMsl/Nodes/FrameNodeMsl.h>
Expand All @@ -34,6 +33,7 @@
#include <MaterialXGenShader/Nodes/CombineNode.h>
#include <MaterialXGenShader/Nodes/SwitchNode.h>
#include <MaterialXGenShader/Nodes/HwImageNode.h>
#include <MaterialXGenShader/Nodes/HwTexCoordNode.h>
#include <MaterialXGenShader/Nodes/ClosureSourceCodeNode.h>
#include <MaterialXGenShader/Nodes/ClosureCompoundNode.h>
#include <MaterialXGenShader/Nodes/ClosureLayerNode.h>
Expand Down Expand Up @@ -178,8 +178,8 @@ MslShaderGenerator::MslShaderGenerator() :
// <!-- <bitangent> -->
registerImplementation("IM_bitangent_vector3_" + MslShaderGenerator::TARGET, BitangentNodeMsl::create);
// <!-- <texcoord> -->
registerImplementation("IM_texcoord_vector2_" + MslShaderGenerator::TARGET, TexCoordNodeMsl::create);
registerImplementation("IM_texcoord_vector3_" + MslShaderGenerator::TARGET, TexCoordNodeMsl::create);
registerImplementation("IM_texcoord_vector2_" + MslShaderGenerator::TARGET, HwTexCoordNode::create);
registerImplementation("IM_texcoord_vector3_" + MslShaderGenerator::TARGET, HwTexCoordNode::create);
// <!-- <geomcolor> -->
registerImplementation("IM_geomcolor_float_" + MslShaderGenerator::TARGET, GeomColorNodeMsl::create);
registerImplementation("IM_geomcolor_color3_" + MslShaderGenerator::TARGET, GeomColorNodeMsl::create);
Expand Down
2 changes: 1 addition & 1 deletion source/MaterialXGenMsl/MslShaderGenerator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class MX_GENMSL_API MslShaderGenerator : public HwShaderGenerator
ShaderNodeImplPtr getImplementation(const NodeDef& nodedef, GenContext& context) const override;

/// Determine the prefix of vertex data variables.
virtual string getVertexDataPrefix(const VariableBlock& vertexData) const;
string getVertexDataPrefix(const VariableBlock& vertexData) const override;

public:
/// Unique identifier for this generator target
Expand Down
62 changes: 0 additions & 62 deletions source/MaterialXGenMsl/Nodes/TexCoordNodeMsl.cpp
View file
Open in desktop

This file was deleted.

26 changes: 0 additions & 26 deletions source/MaterialXGenMsl/Nodes/TexCoordNodeMsl.h
View file
Open in desktop

This file was deleted.

3 changes: 3 additions & 0 deletions source/MaterialXGenShader/HwShaderGenerator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,9 @@ class MX_GENSHADER_API HwShaderGenerator : public ShaderGenerator
/// Unbind all light shaders previously bound.
static void unbindLightShaders(GenContext& context);

/// Determine the prefix of vertex data variables.
virtual string getVertexDataPrefix(const VariableBlock& vertexData) const = 0;

/// Types of closure contexts for HW.
enum ClosureContextType
{
Expand Down
36 changes: 26 additions & 10 deletions ...terialXGenGlsl/Nodes/TexCoordNodeGlsl.cpp → ...terialXGenShader/Nodes/HwTexCoordNode.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,21 @@
// SPDX-License-Identifier: Apache-2.0
//

#include <MaterialXGenGlsl/Nodes/TexCoordNodeGlsl.h>

#include <MaterialXGenShader/Nodes/HwTexCoordNode.h>
#include <MaterialXGenShader/HwShaderGenerator.h>
#include <MaterialXGenShader/Shader.h>

MATERIALX_NAMESPACE_BEGIN

ShaderNodeImplPtr TexCoordNodeGlsl::create()
ShaderNodeImplPtr HwTexCoordNode::create()
{
return std::make_shared<TexCoordNodeGlsl>();
return std::make_shared<HwTexCoordNode>();
}

void TexCoordNodeGlsl::createVariables(const ShaderNode& node, GenContext&, Shader& shader) const
void HwTexCoordNode::createVariables(const ShaderNode& node, GenContext&, Shader& shader) const
{
const ShaderOutput* output = node.getOutput();
const ShaderInput* indexInput = node.getInput(INDEX);
const ShaderInput* indexInput = getIndexInput(node);
const string index = indexInput ? indexInput->getValue()->getValueString() : "0";

ShaderStage& vs = shader.getStage(Stage::VERTEX);
Expand All @@ -27,11 +27,11 @@ void TexCoordNodeGlsl::createVariables(const ShaderNode& node, GenContext&, Shad
addStageConnector(HW::VERTEX_DATA, output->getType(), HW::T_TEXCOORD + "_" + index, vs, ps);
}

void TexCoordNodeGlsl::emitFunctionCall(const ShaderNode& node, GenContext& context, ShaderStage& stage) const
void HwTexCoordNode::emitFunctionCall(const ShaderNode& node, GenContext& context, ShaderStage& stage) const
{
const GlslShaderGenerator& shadergen = static_cast<const GlslShaderGenerator&>(context.getShaderGenerator());
const HwShaderGenerator& shadergen = static_cast<const HwShaderGenerator&>(context.getShaderGenerator());

const ShaderInput* indexInput = node.getInput(INDEX);
const ShaderInput* indexInput = getIndexInput(node);
const string index = indexInput ? indexInput->getValue()->getValueString() : "0";
const string variable = HW::T_TEXCOORD + "_" + index;

Expand All @@ -54,9 +54,25 @@ void TexCoordNodeGlsl::emitFunctionCall(const ShaderNode& node, GenContext& cont
ShaderPort* texcoord = vertexData[variable];
shadergen.emitLineBegin(stage);
shadergen.emitOutput(node.getOutput(), true, false, context, stage);
shadergen.emitString(" = " + prefix + texcoord->getVariable(), stage);

// Extract the requested number of components from the texture coordinates (which may be a
jstone-lucasfilm marked this conversation as resolved.
Show resolved Hide resolved
// larger datatype than the requested number of texture coordinates, if several texture
// coordinate nodes with different width coexist).
std::array<const char*, 5> components { "", "x", "xy", "xyz", "xyzw" };
const string texCoord = shadergen.getSyntax().getSwizzledVariable(
prefix + texcoord->getVariable(), texcoord->getType(),
components[node.getOutput()->getType()->getSize()],
node.getOutput()->getType());

shadergen.emitString(" = " + texCoord, stage);
shadergen.emitLineEnd(stage);
}
}

const ShaderInput* HwTexCoordNode::getIndexInput(const ShaderNode& node) const
{
return node.getInput("index");
}

MATERIALX_NAMESPACE_END

12 changes: 7 additions & 5 deletions ...MaterialXGenGlsl/Nodes/TexCoordNodeGlsl.h → ...MaterialXGenShader/Nodes/HwTexCoordNode.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,24 @@
// SPDX-License-Identifier: Apache-2.0
//

#ifndef MATERIALX_TEXCOORDNODEGLSL_H
#define MATERIALX_TEXCOORDNODEGLSL_H
#ifndef MATERIALX_HWTEXCOORDNODE_H
#define MATERIALX_HWTEXCOORDNODE_H

#include <MaterialXGenGlsl/GlslShaderGenerator.h>
#include <MaterialXGenShader/Nodes/SourceCodeNode.h>

MATERIALX_NAMESPACE_BEGIN

/// TexCoord node implementation for GLSL
class MX_GENGLSL_API TexCoordNodeGlsl : public GlslImplementation
/// Generic texture coordinate node for hardware languages.
class MX_GENSHADER_API HwTexCoordNode : public ShaderNodeImpl
{
public:
static ShaderNodeImplPtr create();

void createVariables(const ShaderNode& node, GenContext& context, Shader& shader) const override;

void emitFunctionCall(const ShaderNode& node, GenContext& context, ShaderStage& stage) const override;

virtual const ShaderInput* getIndexInput(const ShaderNode& node) const;
};

MATERIALX_NAMESPACE_END
Expand Down
6 changes: 6 additions & 0 deletions source/MaterialXGenShader/ShaderStage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,12 @@ ShaderPort* VariableBlock::add(const TypeDesc* type, const string& name, ValuePt
auto it = _variableMap.find(name);
if (it != _variableMap.end())
{
// Automatically try to widen the type of the shader port if the requested type differs from
madmann91 marked this conversation as resolved.
Show resolved Hide resolved
// the existing port's type.
if (it->second->getType()->getSize() < type->getSize())
{
it->second->setType(type);
}
return it->second.get();
}

Expand Down