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

Implement geompropvalue as GLSL varying parameters #1046

Merged
merged 6 commits into from
Dec 7, 2020
Merged

Implement geompropvalue as GLSL varying parameters #1046

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fc8742d
GLSL codegen implementation of geompropvalue as varying parameter
JGamache-autodesk Dec 1, 2020
c0c26e3
Update unit tests.
JGamache-autodesk Dec 2, 2020
91194c7
Brought back boolean and string as uniform geompropvalues
JGamache-autodesk Dec 4, 2020
c41d65a
Fix bad grammar.
JGamache-autodesk Dec 4, 2020
2746664
Implemented varying stream unit tests
JGamache-autodesk Dec 4, 2020
9708a4f
Fix Linux and OS/X builds
JGamache-autodesk Dec 4, 2020
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: 5 additions & 5 deletions resources/Materials/TestSuite/stdlib/geometric/geompropvalue.mtlx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@
<output name="geompropvalue_integer_out" type="vector4" nodename="switch1" xpos="12.4207" ypos="4.42" />
<switch name="switch1" type="vector4" xpos="5.74483" ypos="4.12">
<input name="in1" type="vector4" value="0.0, 0.0, 0.0, 1.0" />
<input name="in2" type="vector4" value="0.3000, 0.3000, 0.3000, 0.3000" />
<input name="in3" type="vector4" value="0.5000, 0.5000, 0.5000, 0.5000" />
<input name="in4" type="vector4" value="0.7000, 0.7000, 0.7000, 0.7000" />
<input name="in5" type="vector4" value="1.0000, 1.0000, 1.0000, 1.0000" />
<parameter name="which" type="integer" nodnname="geompropvalue_integer" />
<input name="in2" type="vector4" value="0.0, 1.0, 1.0, 1.0" />
<input name="in3" type="vector4" value="1.0, 0.0, 1.0, 1.0" />
<input name="in4" type="vector4" value="1.0, 0.0, 0.0, 1.0" />
<input name="in5" type="vector4" value="0.0, 1.0, 0.0, 1.0" />
<parameter name="which" type="integer" nodename="geompropvalue_integer" />
Copy link

Choose a reason for hiding this comment

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

Thanks for fixing this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Made the result visually more interesting when used with a varying input stream.

</switch>
<geompropvalue name="geompropvalue_integer" type="integer" xpos="9.00908" ypos="16.4387">
<parameter name="geomprop" type="string" value="geompropvalue_integer" />
Expand Down
9 changes: 7 additions & 2 deletions source/MaterialXGenGlsl/GlslShaderGenerator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,8 +220,8 @@ GlslShaderGenerator::GlslShaderGenerator() :
registerImplementation("IM_geomcolor_color4_" + GlslShaderGenerator::LANGUAGE, GeomColorNodeGlsl::create);
// <!-- <geompropvalue> -->
registerImplementation("IM_geompropvalue_integer_" + GlslShaderGenerator::LANGUAGE, GeomPropValueNodeGlsl::create);
registerImplementation("IM_geompropvalue_boolean_" + GlslShaderGenerator::LANGUAGE, GeomPropValueNodeGlsl::create);
registerImplementation("IM_geompropvalue_string_" + GlslShaderGenerator::LANGUAGE, GeomPropValueNodeGlsl::create);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will throw an exception when generating code for a document that contains these unimplementable nodes.

Copy link

Choose a reason for hiding this comment

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

I'm wondering if we should let these through and support them as uniforms for now so we don't lose any functionality. I think for the code generation we can test for these cases. As discussed with Niklas, later on we can add in some way to specify this in MaterialX. Then we can support the general concept of "primvars" which may be uniform or varying.

Copy link

@niklasharrysson niklasharrysson Dec 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes for longer term we should start using the geompropdef element. This is already in the spec and should be used to define new geometric stream/properties like this.

So before you can call the geompropvalue node to get the value from a geomprop this geomprop has to be defined first using a geompropdef element. If/when we start using this we could introduce a "uniform" attribute on these definitions to tag the geomprops as being uniform or varying. Boolean and string would naturally always be "uniform".

Copy link

Choose a reason for hiding this comment

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

@JGamache-autodesk , @niklasharrysson : For the current implementation to avoid a regression I'd like to leave the uniform support for boolean and string so graphs that used to work don't fail now. Is that agreeable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can I then recommend splitting the nodedef into varyinggeompropvalue and uniformgeompropvalue in some future update? Otherwise some of the downstream code (like xmlSetProperty in OgsXmlGenerator.cpp) will require some creativity to find out if the value is uniform or varying.

Copy link

Choose a reason for hiding this comment

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

Hi @JGamache-autodesk, Yes, let's do a follow-up to see what works best e.g. two new types as you mention or one type with a reference with an attribute indicating variation.

registerImplementation("IM_geompropvalue_boolean_" + GlslShaderGenerator::LANGUAGE, GeomPropValueNodeGlsl_asUniform::create);
registerImplementation("IM_geompropvalue_string_" + GlslShaderGenerator::LANGUAGE, GeomPropValueNodeGlsl_asUniform::create);
registerImplementation("IM_geompropvalue_float_" + GlslShaderGenerator::LANGUAGE, GeomPropValueNodeGlsl::create);
registerImplementation("IM_geompropvalue_color2_" + GlslShaderGenerator::LANGUAGE, GeomPropValueNodeGlsl::create);
registerImplementation("IM_geompropvalue_color3_" + GlslShaderGenerator::LANGUAGE, GeomPropValueNodeGlsl::create);
Expand Down Expand Up @@ -753,6 +753,11 @@ void GlslShaderGenerator::emitVariableDeclaration(const ShaderPort* variable, co
else
{
string str = qualifier.empty() ? EMPTY_STRING : qualifier + " ";
// Varying parameters of type int must be flat qualified on output from vertex stage and
// input to pixel stage. The only way to get these is with geompropvalue_integer nodes.
if (qualifier.empty() && variable->getType() == Type::INTEGER && !assignValue && variable->getName().rfind(HW::T_IN_GEOMPROP, 0) == 0) {
str += GlslSyntax::FLAT_QUALIFIER + " ";
bernardkwok marked this conversation as resolved.
Show resolved Hide resolved
}
str += _syntax->getTypeName(variable->getType()) + " " + variable->getVariable();

// If an array we need an array qualifier (suffix) for the variable name
Expand Down
1 change: 1 addition & 0 deletions source/MaterialXGenGlsl/GlslSyntax.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ const string GlslSyntax::INPUT_QUALIFIER = "in";
const string GlslSyntax::OUTPUT_QUALIFIER = "out";
const string GlslSyntax::UNIFORM_QUALIFIER = "uniform";
const string GlslSyntax::CONSTANT_QUALIFIER = "const";
const string GlslSyntax::FLAT_QUALIFIER = "flat";
const string GlslSyntax::SOURCE_FILE_EXTENSION = ".glsl";
const StringVec GlslSyntax::VEC2_MEMBERS = { ".x", ".y" };
const StringVec GlslSyntax::VEC3_MEMBERS = { ".x", ".y", ".z" };
Expand Down
1 change: 1 addition & 0 deletions source/MaterialXGenGlsl/GlslSyntax.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ class GlslSyntax : public Syntax
static const string OUTPUT_QUALIFIER;
static const string UNIFORM_QUALIFIER;
static const string CONSTANT_QUALIFIER;
static const string FLAT_QUALIFIER;
static const string SOURCE_FILE_EXTENSION;

static const StringVec VEC2_MEMBERS;
Expand Down
62 changes: 59 additions & 3 deletions source/MaterialXGenGlsl/Nodes/GeomPropValueNodeGlsl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,22 +20,78 @@ void GeomPropValueNodeGlsl::createVariables(const ShaderNode& node, GenContext&,
const ShaderInput* geomPropInput = node.getInput(GEOMPROP);
if (!geomPropInput || !geomPropInput->getValue())
{
throw ExceptionShaderGenError("No 'geomprop' parameter found on geompropvalue node '" + node.getName() + "', don't know what property to bind");
throw ExceptionShaderGenError("No 'geomprop' parameter found on geompropvalue node '" + node.getName() + "'. Don't know what property to bind");
}
const string geomProp = geomPropInput->getValue()->getValueString();
const ShaderOutput* output = node.getOutput();

ShaderStage& vs = shader.getStage(Stage::VERTEX);
ShaderStage& ps = shader.getStage(Stage::PIXEL);

addStageInput(HW::VERTEX_INPUTS, output->getType(), HW::T_IN_GEOMPROP + "_" + geomProp, vs);
addStageConnector(HW::VERTEX_DATA, output->getType(), HW::T_IN_GEOMPROP + "_" + geomProp, vs, ps);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Systematic use of T_IN_GEOMPROP insures the parameter ends up being named i_geomprop_* when varying and u_geomprop_* when uniform.

}

void GeomPropValueNodeGlsl::emitFunctionCall(const ShaderNode& node, GenContext& context, ShaderStage& stage) const
{
const ShaderGenerator& shadergen = context.getShaderGenerator();

const ShaderInput* geomPropInput = node.getInput(GEOMPROP);
if (!geomPropInput)
{
throw ExceptionShaderGenError("No 'geomprop' parameter found on geompropvalue node '" + node.getName() + "'. Don't know what property to bind");
}
const string geomname = geomPropInput->getValue()->getValueString();
const string variable = HW::T_IN_GEOMPROP + "_" + geomname;

BEGIN_SHADER_STAGE(stage, Stage::VERTEX)
VariableBlock& vertexData = stage.getOutputBlock(HW::VERTEX_DATA);
const string prefix = vertexData.getInstance() + ".";
ShaderPort* geomprop = vertexData[variable];
if (!geomprop->isEmitted())
{
shadergen.emitLine(prefix + geomprop->getVariable() + " = " + HW::T_IN_GEOMPROP + "_" + geomname, stage);
geomprop->setEmitted();
}
END_SHADER_STAGE(shader, Stage::VERTEX)

BEGIN_SHADER_STAGE(stage, Stage::PIXEL)
VariableBlock& vertexData = stage.getInputBlock(HW::VERTEX_DATA);
const string prefix = vertexData.getInstance() + ".";
ShaderPort* geomprop = vertexData[variable];
shadergen.emitLineBegin(stage);
shadergen.emitOutput(node.getOutput(), true, false, context, stage);
shadergen.emitString(" = " + prefix + geomprop->getVariable(), stage);
shadergen.emitLineEnd(stage);
END_SHADER_STAGE(shader, Stage::PIXEL)
}

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

void GeomPropValueNodeGlsl_asUniform::createVariables(const ShaderNode& node, GenContext&, Shader& shader) const
{
const ShaderInput* geomPropInput = node.getInput(GEOMPROP);
if (!geomPropInput || !geomPropInput->getValue())
{
throw ExceptionShaderGenError("No 'geomprop' parameter found on geompropvalue node '" + node.getName() + "'. Don't know what property to bind");
}
const string geomProp = geomPropInput->getValue()->getValueString();
ShaderStage& ps = shader.getStage(Stage::PIXEL);
ShaderPort* uniform = addStageUniform(HW::PRIVATE_UNIFORMS, node.getOutput()->getType(), HW::T_GEOMPROP + "_" + geomProp, ps);
uniform->setPath(geomPropInput->getPath());
}

void GeomPropValueNodeGlsl::emitFunctionCall(const ShaderNode& node, GenContext& context, ShaderStage& stage) const
void GeomPropValueNodeGlsl_asUniform::emitFunctionCall(const ShaderNode& node, GenContext& context, ShaderStage& stage) const
{
BEGIN_SHADER_STAGE(stage, Stage::PIXEL)
const ShaderGenerator& shadergen = context.getShaderGenerator();
const ShaderInput* geomPropInput = node.getInput(GEOMPROP);
if (!geomPropInput)
{
throw ExceptionShaderGenError("No 'geomprop' parameter found on geompropvalue node '" + node.getName() + "', don't know what property to bind");
throw ExceptionShaderGenError("No 'geomprop' parameter found on geompropvalue node '" + node.getName() + "'. Don't know what property to bind");
}
const string attrName = geomPropInput->getValue()->getValueString();
shadergen.emitLineBegin(stage);
Expand Down
13 changes: 13 additions & 0 deletions source/MaterialXGenGlsl/Nodes/GeomPropValueNodeGlsl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,19 @@ class GeomPropValueNodeGlsl : public GlslImplementation
void createVariables(const ShaderNode& node, GenContext& context, Shader& shader) const override;

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

bool isEditable(const ShaderInput& /*input*/) const override { return false; }
};

/// GeomPropValue node non-implementation for GLSL
class GeomPropValueNodeGlsl_asUniform : public GlslImplementation
{
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;
};

} // namespace MaterialX
Expand Down
6 changes: 6 additions & 0 deletions source/MaterialXGenOgsXml/OgsFragment.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,12 @@ class OgsFragment
return _element ? _element->getDocument() : mx::DocumentPtr();
}

/// Get the GLSL shader generated for this fragment
mx::ShaderPtr getShader() const
{
return _glslShader;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Helped with debugging.


/// Return the source of the OGS fragment as a string.
const std::string& getFragmentSource() const;

Expand Down
15 changes: 13 additions & 2 deletions source/MaterialXGenOgsXml/OgsXmlGenerator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,6 @@ namespace
{ "Tm", "Tm" },
{ "Bw", "Bw" },
{ "Bm", "Bm" },
{ "texcoord_0", "mayaUvCoordSemantic" },
{ "color_0", "colorset" },

{ "u_worldMatrix", "World" },
{ "u_worldInverseMatrix", "WorldInverse" },
Expand Down Expand Up @@ -85,6 +83,13 @@ namespace
{ "u_time", "Time" }
};

static const vector<std::pair<string, const pugi::char_t*> > OGS_SEMANTICS_PREFIX_MAP =
{
{ "texcoord_", "mayaUvCoordSemantic" },
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We want to handle texcoord_1 and color_2 as well.

{ "color_", "colorset" },
{ "i_geomprop_", "mayaUvCoordSemantic" }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now looking for explicit varying input using i_geomprop_

};

namespace OgsParameterFlags
{
static const std::string
Expand Down Expand Up @@ -143,6 +148,12 @@ namespace
{
prop.append_attribute(SEMANTIC) = semantic->second;
}
for (auto const& ogsSemantic: OGS_SEMANTICS_PREFIX_MAP) {
// STL startswith:
if (name.rfind(ogsSemantic.first, 0) == 0) {
prop.append_attribute(SEMANTIC) = ogsSemantic.second;
}
}
if (!parameterFlags.empty())
{
prop.append_attribute(FLAGS) = parameterFlags.c_str();
Expand Down
3 changes: 3 additions & 0 deletions source/MaterialXGenShader/HwShaderGenerator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ namespace HW
const string T_IN_NORMAL = "$inNormal";
const string T_IN_TANGENT = "$inTangent";
const string T_IN_TEXCOORD = "$inTexcoord";
const string T_IN_GEOMPROP = "$inGeomprop";
const string T_IN_COLOR = "$inColor";
const string T_POSITION_WORLD = "$positionWorld";
const string T_NORMAL_WORLD = "$normalWorld";
Expand Down Expand Up @@ -75,6 +76,7 @@ namespace HW
const string IN_NORMAL = "i_normal";
const string IN_TANGENT = "i_tangent";
const string IN_TEXCOORD = "i_texcoord";
const string IN_GEOMPROP = "i_geomprop";
const string IN_COLOR = "i_color";
const string POSITION_WORLD = "positionWorld";
const string NORMAL_WORLD = "normalWorld";
Expand Down Expand Up @@ -166,6 +168,7 @@ HwShaderGenerator::HwShaderGenerator(SyntaxPtr syntax) :
_tokenSubstitutions[HW::T_IN_NORMAL] = HW::IN_NORMAL;
_tokenSubstitutions[HW::T_IN_TANGENT] = HW::IN_TANGENT;
_tokenSubstitutions[HW::T_IN_TEXCOORD] = HW::IN_TEXCOORD;
_tokenSubstitutions[HW::T_IN_GEOMPROP] = HW::IN_GEOMPROP;
_tokenSubstitutions[HW::T_IN_COLOR] = HW::IN_COLOR;
_tokenSubstitutions[HW::T_POSITION_WORLD] = HW::POSITION_WORLD;
_tokenSubstitutions[HW::T_NORMAL_WORLD] = HW::NORMAL_WORLD;
Expand Down
2 changes: 2 additions & 0 deletions source/MaterialXGenShader/HwShaderGenerator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ namespace HW
extern const string T_IN_NORMAL;
extern const string T_IN_TANGENT;
extern const string T_IN_TEXCOORD;
extern const string T_IN_GEOMPROP;
extern const string T_IN_COLOR;
extern const string T_POSITION_WORLD;
extern const string T_NORMAL_WORLD;
Expand Down Expand Up @@ -140,6 +141,7 @@ namespace HW
extern const string IN_NORMAL;
extern const string IN_TANGENT;
extern const string IN_TEXCOORD;
extern const string IN_GEOMPROP;
extern const string IN_COLOR;
extern const string POSITION_WORLD;
extern const string NORMAL_WORLD;
Expand Down
2 changes: 1 addition & 1 deletion source/MaterialXGenShader/ShaderGraph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1508,7 +1508,7 @@ void ShaderGraph::calculateScopes()
{
newScopeInfo.adjustAtConditionalInput(node, int(inputIndex), 0x12);
}
else if (isSwitch)
else if (isSwitch && inputIndex != node->numInputs() - 1)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not mine. Possibly broke when the which "parameter" became an "input" with 1.38.

{
const uint32_t fullMask = (1 << node->numInputs()) - 1;
newScopeInfo.adjustAtConditionalInput(node, int(inputIndex), fullMask);
Expand Down
15 changes: 13 additions & 2 deletions source/MaterialXRenderGlsl/GlslProgram.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,11 @@ void GlslProgram::bindAttribute(const GlslProgram::InputMap& inputs, MeshPtr mes

glEnableVertexAttribArray(location);
_enabledStreamLocations.insert(location);
glVertexAttribPointer(location, stride, GL_FLOAT, GL_FALSE, 0, nullptr);
if (input.second->gltype != GL_INT) {
glVertexAttribPointer(location, stride, GL_FLOAT, GL_FALSE, 0, nullptr);
} else {
glVertexAttribIPointer(location, stride, GL_INT, 0, nullptr);
}
}
}

Expand Down Expand Up @@ -463,7 +467,14 @@ void GlslProgram::bindStreams(MeshPtr mesh)
bindAttribute(foundList, mesh);
}

// Bind any named geometric property information
// Bind any named varying geometric property information
findInputs(HW::IN_GEOMPROP + "_", attributeList, foundList, false);
if (foundList.size())
{
bindAttribute(foundList, mesh);
}

// Bind any named uniform geometric property information
const GlslProgram::InputMap& uniformList = getUniformsList();
findInputs(HW::GEOMPROP + "_", uniformList, foundList, false);
for (const auto& input : foundList)
Expand Down
6 changes: 3 additions & 3 deletions source/MaterialXTest/MaterialXGenGlsl/GenGlsl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,9 @@ class GlslShaderGeneratorTester : public GenShaderUtil::ShaderGeneratorTester
whiteList =
{
"ambientocclusion", "arrayappend", "backfacing", "screen", "curveadjust", "displacementshader",
"volumeshader", "IM_constant_", "IM_dot_", "IM_geompropvalue", "IM_light_genglsl",
"IM_point_light_genglsl", "IM_spot_light_genglsl", "IM_directional_light_genglsl", "IM_angle",
"surfacematerial", "volumematerial", "ND_surfacematerial", "ND_volumematerial"
"volumeshader", "IM_constant_", "IM_dot_", "IM_geompropvalue_boolean", "IM_geompropvalue_string",
"IM_light_genglsl", "IM_point_light_genglsl", "IM_spot_light_genglsl", "IM_directional_light_genglsl",
"IM_angle", "surfacematerial", "volumematerial", "ND_surfacematerial", "ND_volumematerial"
};
}
};
Expand Down
Loading