Implement geompropvalue as GLSL varying parameters #1046

JGamache-autodesk · 2020-12-02T15:49:13Z

The geompropvalue node in GLSL must be coded as varying parameter since there is no OpenGL API that allows a shader to request a named stream from the geometry.

So the GLSL codegen for these nodes was changed to pass varying input streams.
This will not work for boolean and string geompropvalues so the GLSL codegen correctly leaves them as unimplemented.
Fixes #1042

JGamache-autodesk · 2020-12-02T15:51:21Z

source/MaterialXGenGlsl/GlslShaderGenerator.cpp

    registerImplementation("IM_geompropvalue_integer_" + GlslShaderGenerator::LANGUAGE, GeomPropValueNodeGlsl::create);
-    registerImplementation("IM_geompropvalue_boolean_" + GlslShaderGenerator::LANGUAGE, GeomPropValueNodeGlsl::create);
-    registerImplementation("IM_geompropvalue_string_" + GlslShaderGenerator::LANGUAGE, GeomPropValueNodeGlsl::create);


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

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.

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".

@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?

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.

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.

JGamache-autodesk · 2020-12-02T15:59:27Z

source/MaterialXGenGlsl/Nodes/GeomPropValueNodeGlsl.cpp

+    const ShaderOutput* output = node.getOutput();
+    if (output->getType() == Type::BOOLEAN || output->getType() == Type::STRING) {
+        throw ExceptionShaderGenError("GLSL: unsupported type for geompropvalue node '" + node.getName() + "', can not generate code.");
+    }


Copied from TexCoordNodeGlsl.cpp, but with geomprop instead of index appended.

JGamache-autodesk · 2020-12-02T16:00:07Z

source/MaterialXGenOgsXml/OgsFragment.h

+    mx::ShaderPtr getShader() const
+    {
+        return _glslShader;
+    }


Helped with debugging.

JGamache-autodesk · 2020-12-02T16:00:34Z

source/MaterialXGenOgsXml/OgsXmlGenerator.cpp

@@ -85,6 +83,13 @@ namespace
        { "u_time", "Time" }
    };

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


We want to handle texcoord_1 and color_2 as well.

JGamache-autodesk · 2020-12-02T16:03:33Z

source/MaterialXGenShader/ShaderGraph.cpp

@@ -1508,7 +1508,7 @@ void ShaderGraph::calculateScopes()
                {
                    newScopeInfo.adjustAtConditionalInput(node, int(inputIndex), 0x12);
                }
-                else if (isSwitch)
+                else if (isSwitch && inputIndex != node->numInputs() - 1)


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

JGamache-autodesk · 2020-12-02T16:04:58Z

source/MaterialXTest/MaterialXGenShader/GenShaderUtil.cpp

            if (output)
            {
                outputNode = output->getConnectedNode();
                // Handle connected upstream material nodes later on.
                if (outputNode->getType() != mx::MATERIAL_TYPE_STRING)
                {
                    nodeDef = outputNode->getNodeDef();
+
+                    // We sometimes have complex graph where the node we want to skip is upstream:


The geompropvalue graphs have an extra test node downstream when the final value is not GLSL compatible. We need to traverse to find the unimplementable nodes.

JGamache-autodesk · 2020-12-02T16:05:37Z

source/PyMaterialX/PyMaterialXGenOgsXml/PyOgsFragment.cpp

@@ -25,6 +25,7 @@ void bindPyOgsFragment(py::module& mod)
        .def_property_readonly("document", &my::OgsFragment::getDocument)
        .def_property_readonly("fragmentSource", &my::OgsFragment::getFragmentSource)
        .def_property_readonly("fragmentName", &my::OgsFragment::getFragmentName)
+        .def_property_readonly("shader", &my::OgsFragment::getShader)


Definitely helped with debugging.

niklasharrysson

Looks good! Thanks Jerry

bernardkwok · 2020-12-02T17:44:07Z

resources/Materials/TestSuite/stdlib/geometric/geompropvalue.mtlx

@@ -8,7 +8,7 @@
    <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" />
+    <parameter name="which" type="integer" nodename="geompropvalue_integer" />


Thanks for fixing this.

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

bernardkwok · 2020-12-02T17:46:32Z

source/MaterialXGenGlsl/GlslShaderGenerator.cpp

    registerImplementation("IM_geompropvalue_integer_" + GlslShaderGenerator::LANGUAGE, GeomPropValueNodeGlsl::create);
-    registerImplementation("IM_geompropvalue_boolean_" + GlslShaderGenerator::LANGUAGE, GeomPropValueNodeGlsl::create);
-    registerImplementation("IM_geompropvalue_string_" + GlslShaderGenerator::LANGUAGE, GeomPropValueNodeGlsl::create);


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.

source/MaterialXGenGlsl/GlslShaderGenerator.cpp

bernardkwok · 2020-12-02T17:50:54Z

source/MaterialXGenGlsl/Nodes/GeomPropValueNodeGlsl.cpp

@@ -23,24 +23,48 @@ void GeomPropValueNodeGlsl::createVariables(const ShaderNode& node, GenContext&,
        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();
+    if (output->getType() == Type::BOOLEAN || output->getType() == Type::STRING) {
+        throw ExceptionShaderGenError("GLSL: unsupported type for geompropvalue node '" + node.getName() + "', can not generate code.");


As mentioned above. Maybe we should just default back to uniform generation to not lose functionality.

bernardkwok · 2020-12-02T17:52:27Z

source/MaterialXGenGlsl/Nodes/GeomPropValueNodeGlsl.cpp

+    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");


Nitpick. Should probably be ". Don't" vs "don't" to start a new sentence.

The naming of geompropvalue inputs will now reflect the uniform vs varying nature.

JGamache-autodesk · 2020-12-04T20:41:41Z

resources/Materials/TestSuite/stdlib/geometric/geompropvalue.mtlx

@@ -8,7 +8,7 @@
    <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" />
+    <parameter name="which" type="integer" nodename="geompropvalue_integer" />


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

JGamache-autodesk · 2020-12-04T20:43:22Z

source/MaterialXGenGlsl/Nodes/GeomPropValueNodeGlsl.cpp

+    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);


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

JGamache-autodesk · 2020-12-04T20:44:21Z

source/MaterialXGenOgsXml/OgsXmlGenerator.cpp

+    {
+        { "texcoord_", "mayaUvCoordSemantic" },
+        { "color_", "colorset" },
+        { "i_geomprop_", "mayaUvCoordSemantic" }


Now looking for explicit varying input using i_geomprop_

JGamache-autodesk · 2020-12-04T20:45:17Z

source/MaterialXTest/MaterialXRenderGlsl/RenderGlsl.cpp

@@ -210,12 +209,107 @@ void addAdditionalTestStreams(mx::MeshPtr mesh)
        mesh->addStream(colorStream2);
    }

+    const std::string GEOM_INT_STREAM_NAME("i_" + mx::MeshStream::GEOMETRY_PROPERTY_ATTRIBUTE + "_geompropvalue_integer");
+    int32_t* geomIntData = nullptr;


Added tests for all known varying geompropvalue input types.

bernardkwok

Thanks for preserving the uniform support capability. LGTM.

JGamache-autodesk added 2 commits December 1, 2020 15:45

GLSL codegen implementation of geompropvalue as varying parameter

fc8742d

Update unit tests.

c0c26e3

JGamache-autodesk commented Dec 2, 2020

View reviewed changes

JGamache-autodesk requested review from bernardkwok and niklasharrysson December 2, 2020 16:06

niklasharrysson approved these changes Dec 2, 2020

View reviewed changes

bernardkwok reviewed Dec 3, 2020

View reviewed changes

JGamache-autodesk added 2 commits December 4, 2020 10:01

Brought back boolean and string as uniform geompropvalues

91194c7

Fix bad grammar.

c41d65a

JGamache-autodesk requested a review from bernardkwok December 4, 2020 15:49

Implemented varying stream unit tests

2746664

The naming of geompropvalue inputs will now reflect the uniform vs varying nature.

JGamache-autodesk commented Dec 4, 2020

View reviewed changes

Fix Linux and OS/X builds

9708a4f

bernardkwok approved these changes Dec 7, 2020

View reviewed changes

JGamache-autodesk merged commit a70027b into adsk_contrib/dev Dec 7, 2020

JGamache-autodesk deleted the t_gamaj/glsl_varying_geompropvalue branch December 7, 2020 15:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement geompropvalue as GLSL varying parameters #1046

Implement geompropvalue as GLSL varying parameters #1046

JGamache-autodesk commented Dec 2, 2020 •

edited

Loading

JGamache-autodesk Dec 2, 2020

bernardkwok Dec 2, 2020

niklasharrysson Dec 3, 2020 •

edited

Loading

bernardkwok Dec 4, 2020

JGamache-autodesk Dec 4, 2020

bernardkwok Dec 7, 2020

JGamache-autodesk Dec 2, 2020

JGamache-autodesk Dec 2, 2020

JGamache-autodesk Dec 2, 2020

JGamache-autodesk Dec 2, 2020

JGamache-autodesk Dec 2, 2020

JGamache-autodesk Dec 2, 2020

niklasharrysson left a comment

bernardkwok Dec 2, 2020

JGamache-autodesk Dec 4, 2020

bernardkwok Dec 2, 2020

bernardkwok Dec 2, 2020

bernardkwok Dec 2, 2020

JGamache-autodesk Dec 4, 2020

JGamache-autodesk Dec 4, 2020

JGamache-autodesk Dec 4, 2020

JGamache-autodesk Dec 4, 2020

bernardkwok left a comment

Implement geompropvalue as GLSL varying parameters #1046

Implement geompropvalue as GLSL varying parameters #1046

Conversation

JGamache-autodesk commented Dec 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasharrysson Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasharrysson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bernardkwok left a comment

Choose a reason for hiding this comment

JGamache-autodesk commented Dec 2, 2020 •

edited

Loading

niklasharrysson Dec 3, 2020 •

edited

Loading