AcademySoftwareFoundation / MaterialX

MaterialX is an open standard for the exchange of rich material and look-development content across applications and renderers.
http://www.materialx.org/
Apache License 2.0
1.87k stars 352 forks source link

Improve naming of uniforms/identifiers in codegen #624

Closed niklasharrysson closed 2 years ago

niklasharrysson commented 3 years ago

When validating names for identifiers in GLSL codegen they are allowed to take on names equal to GLSL standard functions.

Here's an example document for which a uniform gets generated with the name "mix", colliding with the standard function mix(), which makes the resulting shader not compile:

<?xml version="1.0"?>
<materialx version="1.38">
  <standard_surface name="shader_a" type="surfaceshader">
    <input name="base_color" type="color3" value="1, 0, 0" />
  </standard_surface>
  <standard_surface name="shader_b" type="surfaceshader">
    <input name="base_color" type="color3" value="0, 0, 1" />
  </standard_surface>
  <nodedef name="ND_mix_shader" node="mix_shader">
    <input name="shader_a" type="surfaceshader" />
    <input name="shader_b" type="surfaceshader" />
    <input name="mix" type="float" />
    <output name="out" type="surfaceshader" />
  </nodedef>
  <nodegraph name="IMP_mix_shader" nodedef="ND_mix_shader">
    <mix name="mix1" type="surfaceshader">
      <input name="fg" type="surfaceshader" interfacename="shader_a" />
      <input name="bg" type="surfaceshader" interfacename="shader_b" />
      <input name="mix" type="float" interfacename="mix" />
    </mix>
    <output name="out" type="surfaceshader" nodename="mix1" />
  </nodegraph>
  <mix_shader name="node1" type="surfaceshader" nodedef="ND_mix_shader">
    <input name="shader_a" type="surfaceshader" nodename="shader_a" />
    <input name="shader_b" type="surfaceshader" nodename="shader_b" />
    <input name="mix" type="float" value="0.5" />
  </mix_shader>
  <surfacematerial name="mat1" type="material">
    <input name="surfaceshader" type="surfaceshader" nodename="node1" />
  </surfacematerial>
</materialx>

A quick fix is to add the names of all standard GLSL functions to the list of reserved words here: https://github.com/materialx/MaterialX/blob/main/source/MaterialXGenGlsl/GlslSyntax.cpp#L158

But a better solution might be to make the generated identifiers more unique by obfuscating them a bit, for example appending some number to their name. The "mix" uniform in the example above would in code be generated as something like "mix_372". We already keep a separate record of parameter names (in UI) vs identifier names (in code), so the obfuscated names would not need to be displayed anywhere.

jstone-lucasfilm commented 2 years ago

The "mix" keyword has been added to the reserved list for GLSL, so I think it makes sense to close out this issue for now. It's still worthwhile to consider the idea of obfuscated identifiers in generated code, though this change would come with its own set of tradeoffs, especially with respect to the clarity of generated shading code for developers aiming to understand it.