Smoothstep breaks GLSL compilation when multiple type signature nodes are used

[HardCoreCodin]

When multiple signature variants of smoothstep participate in a graph, the viewport render goes blank and this shows in the output:

Error in compiling fragment shader:
0(493) : error C1013: function "mx_smoothstep_float" is already defined at 0(483)

This is likely because the function mx_smoothstep_float exists in multiple headers that get included in such cases.

See attached example file. smoothstep_failure_mtlx.txt

[kwokcb]

There are includes which I assume are not being checked to see if that file has already been included in the non-float code implementations

#include "mx_smoothstep_float.glsl"

This breaks GLSL and Metal at least.

[jstone-lucasfilm]

Good catch, @HardCoreCodin, and this sounds like a bug in our GLSL shader generation that should be fixed.

[ld-kerley]

I think the fix is probably a pretty easy #ifndef/#define trick in the right place - I can investigate and put up a PR soon

[kwokcb]

@ld-kerley , not sure if this is the place to look but I believe #includes are parsed and code is embedded -- but it should not try to include twice based on ShaderStage::addInclude().

[ld-kerley]

In this case the #include statement is in a file that is already being included - a quick local test and adding a standard include guard around the actual source does appear to work in both GLSL and MSL.

[ld-kerley]

I think I see what's happening.... The float version of smooth step is added first, but because this is a ShaderSourceNode it doesn't added with addInclude() which is where we do the de-duplication of the includes, but instead just directly calls addBlock().

The second (color) smoothstep node is also a ShaderSourceNode so it also just adds itself using addBlock() which does then parse the #include statements, and call addInclude() for each of them, but because the initial inclusion of the float smoothstep code wasn't added via addInclude() its not in its de-duplication map.

I can see a few possible solves here.

• We could just add #ifndef/#define guards around code that is included, that way if other holes appear in the de-duplication logic they won't fail. This approach could lead to longer shader code files, but the shader compilers will just ignore the additional code by way of the pre-processor.
• We could change the ShaderSourceNode implementation to call addInclude() instead of addBlock(), I'm not sure if this might leave other holes. Also ShaderSourceNode doesn't actually directly call addBlock() it calls emitBlock() in the ShaderGenerator interface, and I don't currently see any equivalent emitInclude() in that classes interface.
• We could also impose a constraint that shader source files cannot include any other shader source files, and instead common code must be restructured in to a common header file. Concretely here that would mean that the float smoothstep file would just include some new file in genglsl/lib that would contain the actual implementation. It would be nice to add some sort of validation/unit test that we don't break this new constraint if we adopt this approach.

I think in the long term I would vote for option 3, but I also think that option 1 might be the pragmatic answer in the short term, and perhaps we file these ideas as an issue to be working on in the future.

Curious what you think, @kwokcb, @niklasharrysson, @jstone-lucasfilm ?

[kwokcb]

I’ll just throw out one more option. Turn the vector versions of this into graphs that use the float version node instances — hence no include dependence exists. Would have to think some more on the other options, but adding the common include seems like a good idea if it is added on demand.

[ld-kerley]

I like the nodegraph implementation idea, though if we applied this to all destination languages, then I think we would be constructing less optimal OSL code.

  <implementation name="IM_smoothstep_vector3_genosl" nodedef="ND_smoothstep_vector3" target="genosl" sourcecode="smoothstep({{low}}, {{high}}, {{in}})" />

OSL currently provides a smoothstep() that can take a vector type that will allow for more aggressive OSL optimizer optimizations.

I guess that raises a hole in my MaterialX understanding. If a nodegraph implementation exists, alongside a target specific implementation as above, what happens? I guess the implementation that is more specific would ideally be selected, if so then we could safely deploy the nodegraph idea (which I think in general is how we should try and build all nodes to make them more maintainable), without any loss of OSL performance.

[ld-kerley]

Interestingly we already have "some" smoothstep nodegraph implementations, so I think that casts an even stronger vote for that solution.

[dbsmythe]

Yes, according to the Spec:

It is allowable for there to be both a <nodegraph> and an <implementation> for the same nodedef target/version, with the <implementation> generally prevailing in order to allow for optimized native-code node implementations, although ultimately it would be up to the host application to determine which implementation to actually use.

(The above sentence is in the paragraph right above the https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/documents/Specification/MaterialX.Specification.md#example-custom-node-defined-by-a-nodegraph section)

[HardCoreCodin]

There may be strong reasons in the near term to also have a custom smoothstep function, even for float, for all back-end targets, including OSL. And they would all likely need to change soon - see this other discussion on the OSL docs issue (it would also related to this GLSL implementation from the MaterialX's perspective).

[kwokcb]

@ld-kerley for your implementation matching question:

• The target is sent over when searching for the implementation so it should search for the anything that matches the target first before checking non-target specific implementations.
• The target value comes from the code generator.

[jstone-lucasfilm]

Thanks for this report, @HardCoreCodin, and to @ld-kerley for the fix in #1982!