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 353 forks source link

Graph Editor: Directly connecting input to output node doesn't work #1454

Closed marwie closed 1 year ago

marwie commented 1 year ago

If I understood you correctly the input inside the nodegraph should work and I should be able to pipe it through an output to connect it with some standard_surface/input. But it doesn't seem to work without adding some node inbetween (like a constant node).

I recorded a short video below.
There seem to be some other weird things going on as well in there (like having an output element connected to two other elements)

https://github.com/AcademySoftwareFoundation/MaterialX/assets/5083203/c401ed55-4a05-4aea-a682-cfbaab8c10ca

Originally posted in: https://github.com/AcademySoftwareFoundation/MaterialX/issues/1445#issuecomment-1679341987

marwie commented 1 year ago

According to @kwokcb this might be invalid: For the direct connection, I think this is considered to be invalid but it's not stated explicitly in the spec. We end up with validation errors currently. The editor seems to be allowing this invalid case so you get the weird fan-in case.

cc @dbsmythe @jstone-lucasfilm

kwokcb commented 1 year ago
  1. Taking a quick look, I don't see any existing syntax that can even describe this as everything is node based for upstream connections based on the spec

This is basically the same config as the what's seen in the video (after save)

<?xml version="1.0"?>
<materialx version="1.38" colorspace="lin_rec709">
  <nodegraph name="nodegraph1">
    <input name="input_color3" type="color3" value="0.855746, 0, 0" xpos="11.275362" ypos="-1.224138" />
    <output name="output_color3" type="color3" xpos="14.514493" ypos="-1.267241" />
  </nodegraph>
  <surface_unlit name="surface_unlit" type="surfaceshader" xpos="13.905797" ypos="-1.344828">
    <input name="emission_color" type="color3" output="output_color3" nodegraph="nodegraph1" />
  </surface_unlit>
  <surfacematerial name="surfacematerial" type="material" xpos="16.782608" ypos="-1.094828">
    <input name="surfaceshader" type="surfaceshader" nodename="surface_unlit" />
  </surfacematerial>
</materialx>

I see no way to specify an upstream input to output connection on the same nodegraph. This is the closest (made-up syntax) I can think of:

<output_color3 interfacename="input_color3" />
  1. I think we encountered this a long time ago and the recommendation is to use an intermediate dot node which allows pass-through. This is how we do pass-through for shader translation nodes.

@marwie, Here is the "dot" version of the above for simplicity. If you need pass through then I think you'll need to do this.

<?xml version="1.0"?>
<materialx version="1.38" colorspace="lin_rec709">
  <nodegraph name="nodegraph1">
    <input name="input_color3" type="color3" value="0.855746, 0, 0" xpos="11.137681" ypos="-1.491379" />
    <output name="output_color3" type="color3" xpos="14.514493" ypos="-1.258621" nodename="dot_color3" />
    <dot name="dot_color3" type="color3" xpos="12.985507" ypos="-1.896552">
      <input name="in" type="color3" interfacename="input_color3" />
    </dot>
  </nodegraph>
  <surface_unlit name="surface_unlit" type="surfaceshader" xpos="13.905797" ypos="-1.344828">
    <input name="emission_color" type="color3" output="output_color3" nodegraph="nodegraph1" />
  </surface_unlit>
  <surfacematerial name="surfacematerial" type="material" xpos="16.782608" ypos="-1.094828">
    <input name="surfaceshader" type="surfaceshader" nodename="surface_unlit" />
  </surfacematerial>
</materialx>
  1. @lfl-eholthouser, I think this is another case where the editor should prevent this from occurring, as no connection actually get's stored even though the link exists in the UiNode. Thoughts ?
marwie commented 1 year ago
  1. thanks that's good to know. I have been using constant at the moment but will replace it now with a dot
jstone-lucasfilm commented 1 year ago

This sounds like a good analysis to me, @kwokcb, and I believe you're correct that our current syntax doesn't support this. I'll close out this original issue for now, and we can keep the option open of supporting this through future syntaxes if needed.

hybridherbst commented 1 year ago

I think you may want to keep the issue open so that the graph editor is adjusted to not allow this invalid connection.

jstone-lucasfilm commented 1 year ago

Ah, I see what you mean, @hybridherbst, and I'll reopen this for tracking the UI clarity issue in the graph editor.

kwokcb commented 1 year ago

I had a firewall fix, but forgot to put up a PR. There is one now.