Fix 1.38 normalmap frame input upgrade

[tdavidovicNV]

MaterialX 1.39 normalmap inputs use defaultgeomprop for the normal, tangent and bitangent frame inputs. Older 1.38 documents can still contain unconnected literal values on those inputs, often zeros from DCC exports (e.g., from Houdini). Shader generation treats unconnected defaultgeomprop inputs as geometric defaults rather than literal fallback values, so the upgrade path should preserve that behavior.

Example from the Teapot asset:

<normalmap name="normalmap1_teapot_metal" type="vector3">
  <input name="in" type="vector3" nodename="tex_normal1_base_teapot_metal" value="0.5, 0.5, 1" />
  <input name="space" type="string" value="tangent" />
  <input name="scale" type="float" value="1" />
  <input name="normal" type="vector3" value="0, 0, 0" />
  <input name="tangent" type="vector3" value="0, 0, 0" />
</normalmap>
<image name="tex_normal1_base_teapot_metal" type="vector3">
  <input name="file" type="filename" value="Teapot/Texture/latest/TeapotMetal_L1Base/Teapot_TeapotMetal_Normal_concrete.jpg" />
</image>

During the 1.38 to 1.39 upgrade, the existing code would take the literal value values, and cross them to build bitangent, which would be 0. This is patently wrong. We must remove unbound normal map inputs and let the 1.39 default geomprops take over.

I would also recommend making this behavior even more clear in the spec, that value on a node that has default geomprop will be completely ignored:

* `defaultgeomprop` (string, optional): for vector2 or vector3 inputs, the name of an intrinsic geometric property that provides the default value for this input, must be one of "position", "normal", "tangent", "bitangent" or "texcoord" or vector3-type custom geometric property for vector3 inputs, or "texcoord" or vector2-type custom geometric property for vector2 inputs.  For standard geometric properties, this is effectively the same as declaring a default connection of the input to a Geometric Node with default input values>>>, and takes precedence over any unconnected node input `value` attribute<<<.  May not be specified on uniform inputs.

But that's not necessarily part of this PR.

Closes #2828.

[pablode]

@tdavidovicNV I don’t share the assumption that the current algorithm is “patently” wrong, because, as you yourself state, the issue at hand is undefined behaviour.

The upgrade algorithm was written under the assumption that an authored value takes precedence over defaultgeomprop.

Just because the shader generation ignores the value, it does not mean that the behaviour is correct.

[tdavidovicNV]

@pablode good point, I re-read the spec and it is definitely less clear now than on the first reading (and even that required some transitive jumping through points).

I was basing my take on the combination of:

• geometric property can be used as the “default value for a node input”:
  MaterialX.Specification.md, Geometric Properties
• defaultgeomprop is described as “effectively the same as declaring a default connection”:
  MaterialX.Specification.md, NodeDef Input Elements
• The current ShaderGen implementation: it explicitly says it will “// Create and connect default geometric nodes on unconnected inputs”, and the condition checks for no authored connection:
  ShaderGraph.cpp

The combination of these three, for me, meant very clearly that if there is no authored connection, then geomprop provides the connection.. and if there is a connection, it takes precedence over any value, turning the values irrelevant. (This is further supported by Houdini implementing it that way and emitting the shader example above).

But I can see how a similar set of arguments can be used against this interpretation:

• Inputs section says node inputs define “value or connection”:
  MaterialX.Specification.md, Inputs
• NodeDef input section describes value as a default “if the input remains unconnected and is not otherwise assigned a value” and also that it is ok to define “a value or a defaultgeomprop”, and that if neither is defined, the node invocation must provide “a value or connection”:
  MaterialX.Specification.md, NodeDef Input Elements

So in that way, the presence of value should make the defaultgeomprop not kick in at all, whether it acts as a connection or not.

And I would even agree that your interpretation makes more sense, because it is just weird that value= can never have a meaning on an input with defaultgeomprop.

Practicioners note:

Without the interpretation this fix implies, any assets from Houdini, up until at least 21.0.631 won't work, because it still puts the 0 value there:

<?xml version="1.0"?>
<materialx version="1.39">
  <!-- Generated in Houdini from /mat/mtlxnormalmap1 -->
  <normalmap name="mtlxnormalmap1" type="vector3">
    <input name="in" type="vector3" value="0.5, 0.5, 1" />
    <input name="scale" type="float" value="1" />
    <input name="normal" type="vector3" value="0, 0, 0" />
    <input name="tangent" type="vector3" value="0, 0, 0" />
    <input name="bitangent" type="vector3" value="0, 0, 0" />
  </normalmap>
</materialx>

If we pivot to taking the interpretation of the upgrade algorithm over the ShaderGen, this stops working. But I totally agree it is a bigger question than I originally thought.

(fwiw, I checked LookDevX, and it doesn't put the zeroes there, dunno if there is any other currently public production MaterialX tool)

[HardCoreCodin]

I'd say 2 things here:

1. Clearly, the specification is either ambiguous or self-contradictory - either of which makes it faulty, so needs correcting.
2. Inputs intended to have defaulted behaviors applied to them, are expected to be omitted. When they're not, the content itself is faulty and needs fixing, including anything that produced it.
3. Given how late this got identified, meaning there's lots of faulty content out there, and given the specification faults, a consensus should be formed about what should be done about it. Some implementations of importers may be specifically checking for this and then simply consider the input as though it were omitted.

[jstone-lucasfilm]

Thanks for this fix, @tdavidovicNV, and thanks to @pablode for raising the interpretation question, which deserves a clear answer before this work goes forward.

To ground the discussion, I tested the actual behavior of MaterialX 1.38, running the 1.38.10 release on a Houdini-style document with literal 0, 0, 0 values on the normalmap frame inputs. In each of the three shader generators (GLSL, OSL, MDL), the unconnected literal values are ignored, and the Nworld and Tworld geometric properties are passed to mx_normalmap:

mx_normalmap_float(tex_normal_out, ..., geomprop_Nworld_out1, geomprop_Tworld_out1, mtlxnormalmap1_out);

This holds even when the zeros are authored on nodegraph interface inputs, and the relevant logic in ShaderGraph::createNode is unchanged from 1.38.0 through 1.38.10. So documents following this pattern rendered correctly in 1.38-era applications, and the current upgrade path turns a working 1.38 document into a broken 1.39 one, which is a true regression, independent of how one reads the specification. It's worth noting that the current upgrade is internally inconsistent as well. Running the same document through MaterialX 1.39.4, the injected bitangent honors the literal zeros, baking normalize(cross(0, 0)) into the graph, while shader generation continues to ignore those same zeros for the normal and tangent, sourcing them from geometric properties instead.

@pablode, your reading of the specification is very reasonable, and this discussion makes it clear that the current wording supports both interpretations. Since every MaterialX shader generator has implemented defaultgeomprop as a default connection, taking precedence over unconnected value attributes, and existing content from Houdini and other applications depends on that behavior, I believe our best path is to ratify the implemented behavior in the specification rather than change it.

So I'd recommend the following interpretation going forward: an unconnected value on an input with defaultgeomprop is ignored, and version upgrades should preserve that behavior, which is exactly what this change does.

If that solution seems reasonable to you all, I'd recommend that we add test coverage for the new upgrade paths as part of this PR, e.g. in resources/Materials/TestSuite/stdlib/upgrade/syntax_1_38.mtlx, which currently has no normalmap with literal frame values. I'd suggest two cases, one following the common Houdini pattern with unconnected normal and tangent values of 0, 0, 0, and one mixed case with a single frame input connected, exercising the new geomprop-injection path in copyInputOrConnectWorldGeomProp.

On your suggestion of clarifying the specification text, I fully agree, and I'd recommend that we make that a separate documentation PR following this one.

[tdavidovicNV]

Thanks @jstone-lucasfilm for the feedback. I have just two notes:

1. I've tried and failed rather miserably to come up with an actual usecase for value=... being meaningful on a node. I can't come up with one, but if there was one, this change would just prevent it. An alternative solution would be to "just" reject 0,0,0 as invalid values, but accept everything else.
2. I made the docs and the code change a single PR in other cases as well, because to me it is "one fix". I am happy to split it, but to me "making an unclear thing consistent" was a single PR, even though some was text and some was code.

[tdavidovicNV]

On my first note: I consider this resolved. ShaderGen has ignored unconnected values on defaultgeomprop inputs since 1.38, regardless of the value, so a zero-only exception would not preserve existing behavior.
On my second note: I will propose a follow-up PR.

I pushed the required regression tests (well, examples), so we should be good to go on this.