Default transform<X> nodes can crash shader generation (GLSL)

[kwokcb]

Issue

There are no default "from" and "to" space values so they are left empty on creation.
If you try and generate code from them, then there are a few missing empty value checks missing and code gen can crash.

Setup

This is a simple example:

<?xml version="1.0"?>
<materialx version="1.38">
  <transformnormal name="transformnormal_vector3" type="vector3">
    <input name="in" type="vector3" value="0.0, 0.0, 1.0" />
    <input name="fromspace" type="string" value="" uniform="true" />
    <input name="tospace" type="string" value="" uniform="true" />
  </transformnormal>
  <convert name="shader_transformnormal_vector3_out" type="surfaceshader">
    <input name="in" type="vector3" nodename="transformnormal_vector3" />
  </convert>
  <surfacematerial name="material_transformnormal_vector3_out" type="material">
    <input name="surfaceshader" type="surfaceshader" nodename="shader_transformnormal_vector3_out" />
  </surfacematerial>
</materialx>

Initial Triage

Loading this in will crash the view and editor where only GLSL was tried out. The fix appears to be in TransformVectorNodeGLSL.cpp but didn't check other code generators, and it seems all of the "transform" nodes will need to be patched, so wondering if there is a higher level place ?

Proposed Fix

Add in firewall checks to see if there is a value and default to empty if none:

• https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/source/MaterialXGenGlsl/Nodes/TransformVectorNodeGlsl.cpp#L20-L23
• https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/source/MaterialXGenGlsl/Nodes/TransformVectorNodeGlsl.cpp#L45-L49
• ... and other required places ...

Secondary Proposal

Propose these nodes should have some reasonable default and be enumerations if it makes sense. Otherwise it's mostly
a guess as to what strings are required and documentation validation will fail if the string is incorrect.

[madmann91]

Hello! I am going to work on this issue while the CLA agreement is being processed in my organization, if that's alright. I can suggest the following plan:

• Group all the Msl/Glsl TransformXXXXNode into a parent HwTransformNode, and do most of the logic there (including the checks for whether the source or destination space are provided),
• Make the individual TransformXXXXNodeGlsl/TransformNodeXXXXMsl inherit from that HwTransformNode.

Just like for the texcoord nodes, it seems there's a lot of code duplication in there, and functions mostly only differ by name, while their content is almost identical. Merging things this way would still open the door to further customization via virtual functions.

What do you think?

[kwokcb]

Hi @madmann91 ,

• I would say that your secondary proposal should be done in any case as it makes no sense to leave them empty IMO. They should probably be enumerations as well so there is a set of available options to the user.
  • Note that implementations like OSL have inlined direct calls to intrinsic functions, so I'm not sure but guessing that these can handle empty strings properly and hence a possible reason to not have any defaults?
• As for the firewall check, perhaps getMatrix() should have it's signature changed take an Input instead of 2 strings. Then the check for from/tospace can be done in one place there. You can add the inheritance if it's still useful.