GraphEditor: visualise type-mismatched connections

[pixelsandpointers]

Detect input/output type mismatches when building the link graph and
surface them visually in three ways:

• Erroneous links are drawn in red with a heavier stroke.
• Nodes that receive a mismatched connection get a red border highlight.
• A diagnostic panel appears below the node editor, listing each mismatch
  (node name, input name, expected type, actual type); clicking an entry
  navigates to the offending node.

The Link struct gains an _erroneous flag and a new LinkDiagnostic
struct is introduced to carry per-mismatch metadata collected in
linkGraph() and consumed by connectLinks(), createNodes(), and
drawGraph().

Closes #2920

[pixelsandpointers]

Hi @jstone-lucasfilm,
I think this would be good to test.

I am also tagging @bhouston because he brought the idea up. If there's any feedback, feel free to leave it here.

Thanks!

[jstone-lucasfilm]

This looks very promising, thanks @pixelsandpointers! I've posted one suggested naming change for alignment, and I'm interested in thoughts on the UI/UX from @lfl-eholthouser, who originally authored the graph editor.

[lfl-eholthouser]

Hi @pixelsandpointers I think this change looks great! Thanks for adding this. I just have one note about the multioutput. Would it be possible to add support for multioutput nodes by potentially checking the nodedef of the node to see the potential output types and using that to check for an erroneous links or put a TODO comment so that we can track that outstanding work.

[pixelsandpointers]

Hi @lfl-eholthouser,
I've added a few more checks to support multioutput nodes.

• When outputType == MULTI_OUTPUT_TYPE_STRING, it fetches the nodedef of the upstream node and looks up the output by outPin->getName() (the specific channel, e.g. "diffuse", "specular").

• The resolved type replaces "multioutput" in the comparison, so a type mismatch is correctly flagged. The diagnostic also records the resolved type, so the error message shows the actual type that was mismatched rather than the opaque "multioutput" string.

• If the nodedef lookup fails for any reason (no nodedef, output not found), resolvedOutputType stays as "multioutput" and the guard != mx::MULTI_OUTPUT_TYPE_STRING preserves the old behaviour.

[pixelsandpointers]

Hi @lfl-eholthouser and @jstone-lucasfilm,

Sorry, I went a bit wild on the PR (but I hope the changes make sense). I implemented error propagation for nodegraphs now. Essentially, this traverses all nodegraph nodes and checks them for errors. If there are any, the nodegraph node will be outlined in orange. The diagnostic panel is being populated by all errors in the document, but the errors for each nodegraph node will be foldable. I think this still allows for better navigation when there are multiple errors in the document. The user is still able to interact with the entries for a goto invalid connection.

(Haven't had clang-format on the device I am working on, so another commit that reformats the code running clang-format.)

[jstone-lucasfilm]

This is looking really close to the mark, thanks @pixelsandpointers, and I had just two suggestions for improvements.

[jstone-lucasfilm]

In order to make this system more future-proof, I wonder if the custom type-checking logic in this function could be replaced by the following:

  if (!input->validate())
  {
      LinkDiagnostic diag;
      diag.nodeName  = node->getName();
      diag.inputName = input->getName();
      diag.inputType = input->getType();
      diag.outputType = /* resolved upstream type */;
      diag.graphPath = graphName;
      diag.nodeGraph = ng;
      _diagnostics.push_back(diag);
  }

If this simplified approach worked well, then it would make sure that your new diagnostic system remains aligned with the core Element::validate system in MaterialX, removing the possibility that they would diverge as the MaterialX project evolves.

I think your proposal is valuable in either case, but it seems worthwhile to check whether we can meaningfully align it with our core validation logic from the start, rather than merging the two further in the future.

[pixelsandpointers]

I'll have a look this evening. I'm sure this should work as well.

[pixelsandpointers]

@jstone-lucasfilm the current implementation now has one caveat. validate can return false for things beyond types, so we might populate the diagnostic panel with more than just type mismatches, but we fully align with the validation system.

One other idea might be filtering the type mismatch.

const std::string& inputType = input->getType();
std::string outputType = upstream->getType();
if (outputType == mx::MULTI_OUTPUT_TYPE_STRING)
{
    mx::NodeDefPtr nodeDef = upstream->getNodeDef();
    if (nodeDef)
    {
        mx::OutputPtr defOut = nodeDef->getOutput(input->getOutputString());
        if (defOut)
            outputType = defOut->getType();
     }
}

bool typeMismatch = !inputType.empty() && !outputType.empty() && outputType != mx::MULTI_OUTPUT_TYPE_STRING && outputType != inputType;

// Cross-check with core validation; if we report a type mismatch but
// Element::validate disagrees, the two systems have diverged.
std::string validationMsg;
if (typeMismatch && input->validate(&validationMsg))
{
    std::cerr << "Warning: type mismatch detected by graph editor but not by "
    << "Element::validate for input '" << input->getName()
    << "' on node '" << node->getName()
    << "' in '" << graphName << "'" << std::endl;
}

if (typeMismatch)
{
    LinkDiagnostic diag;
    diag.nodeName  = node->getName();
    diag.inputName = input->getName();
    diag.inputType = inputType;
    diag.outputType = outputType;
    diag.graphPath = graphName;
    diag.nodeGraph = ng;
    _diagnostics.push_back(diag);
}

[jstone-lucasfilm]

Thanks for all of your great work on this, @pixelsandpointers.

I think the caveat you mention above resolves if we fully embrace the unified approach, using Input::validate as the single detection path for both top-level links and the nested scan, keeping the multioutput resolution only as a display helper for the "got X" text, and relabeling the panel from "Type mismatch errors" to "Invalid connections" so it reflects what Input::validate reports.

Here are a few concrete suggestions on details:

1. Add a message field to the diagnostic in Graph.h and declare two helpers:

  // Describes a single invalid connection detected by core validation.
  struct LinkDiagnostic
  {
      int nodeId = -1;            // UiNode ID; -1 when the error is in a nested nodegraph
      std::string nodeName;
      std::string inputName;
      std::string inputType;
      std::string outputType;     // resolved upstream type, best-effort, for display only
      std::string message;        // message returned by Element::validate()
      std::string graphPath;      // empty = top-level; otherwise the containing nodegraph name
      mx::NodeGraphPtr nodeGraph; // nullptr = top-level; navigate here on click
  };

  // Walk all NodeGraph elements in _graphDoc and append invalid-connection diagnostics.
  void scanNestedGraphDiagnostics();

  // If the given connected input fails core validation, append a diagnostic and return true.
  bool addInvalidInputDiagnostic(mx::InputPtr input, const std::string& nodeName,
                                 int uiNodeId, const std::string& graphPath,
                                 mx::NodeGraphPtr ng);

  // Best-effort resolution of the upstream output type feeding an input (for display only).
  std::string resolveUpstreamOutputType(mx::InputPtr input) const;

2. Add shared helpers to Graph.cpp:

  std::string Graph::resolveUpstreamOutputType(mx::InputPtr input) const
  {
      mx::NodePtr upstream = input->getConnectedNode();
      if (!upstream)
          return mx::EMPTY_STRING;

      std::string outputType = upstream->getType();
      if (outputType == mx::MULTI_OUTPUT_TYPE_STRING)
      {
          mx::NodeDefPtr nodeDef = upstream->getNodeDef();
          if (nodeDef)
          {
              mx::OutputPtr defOut = nodeDef->getOutput(input->getOutputString());
              if (defOut)
                  outputType = defOut->getType();
          }
      }
      return outputType;
  }

  bool Graph::addInvalidInputDiagnostic(mx::InputPtr input, const std::string& nodeName,
                                        int uiNodeId, const std::string& graphPath,
                                        mx::NodeGraphPtr ng)
  {
      // Only consider inputs that actually carry a connection.
      if (!input || !input->getConnectedNode())
          return false;

      std::string message;
      if (input->validate(&message))
          return false;

      LinkDiagnostic diag;
      diag.nodeId     = uiNodeId;
      diag.nodeName   = nodeName;
      diag.inputName  = input->getName();
      diag.inputType  = input->getType();
      diag.outputType = resolveUpstreamOutputType(input);
      diag.message    = message;
      diag.graphPath  = graphPath;
      diag.nodeGraph  = ng;
      _diagnostics.push_back(diag);
      return true;
  }

3. Simplify the custom block in linkGraph to the following:

  if (start >= 0)
  {
      // Connect the correct output pin to this input.
      for (UiPinPtr outPin : inputNode->getOutputPins())
      {
          if (outPin->getPinId() == outputId)
          {
              outPin->setConnected(true);
              outPin->addConnection(inputs[i]);
          }
      }

      // Flag invalid connections via the core validation system.
      bool invalid = addInvalidInputDiagnostic(
          inputs[i]->getInput(), node->getName(), node->getId(),
          mx::EMPTY_STRING, nullptr);

      Link link(_state.nextUiId++, start, end, invalid);
      if (!linkExists(link))
      {
          _state.links.push_back(link);
      }
  }

4. Simplify scanNestedGraphDiagnostics to the following:

  void Graph::scanNestedGraphDiagnostics()
  {
      for (mx::NodeGraphPtr ng : _graphDoc->getNodeGraphs())
      {
          const std::string& graphName = ng->getName();
          for (mx::NodePtr node : ng->getNodes())
          {
              for (mx::InputPtr input : node->getInputs())
              {
                  addInvalidInputDiagnostic(input, node->getName(), -1, graphName, ng);
              }
          }
      }
  }

5. Update the diagnostic panel in drawGraph so that each entry shows the resolved types when it's a true type mismatch and falls back to the validate message otherwise:

  std::string label = "  " + d->nodeName + "." + d->inputName;
  if (!d->inputType.empty() && !d->outputType.empty() && d->inputType != d->outputType)
      label += "  [expects " + d->inputType + ", got " + d->outputType + "]";
  else if (!d->message.empty())
      label += "  [" + d->message + "]";

And relabel the panel title to match the broader meaning:

  ImGui::Text("Invalid connections: %d", (int) _diagnostics.size());

This approach would give us a truly unified solution, where the display is driven by our core validation system, and the two can evolve together over time.

What are your thoughts?

[pixelsandpointers]

I like this. I wasn't sure whether I should catch other errors as well, but if I should, this looks like a nice proposal. I will find some time tomorrow to change the implementation.