Proposal : Provide a Functionally equivalent operator

kwokcb commented 2 months ago

Proposal

The current equivalence operator (operator==) will fail if:

The order of attributes for an element differs
The order of children differs

This is the desired logic.

However, if there can easily be a case where the attributes or children can be added in different order -- in which case the logic fails if you want to perform a functionally equivalent comparison.

The proposal is support such a comparison.

Workflows

It's very easy to add / remove nodes or inputs to a graph which changes ordering. A simple cut (remove) and paste (add last) could change order.
Attributes can be changed and assuming this more for metadata like colorspace, units, geom properties, and UI formatting such as position.

Proposal

It would be useful to add an option / create a new function which allows for ordering to be ignored.
Note that nodedef lookup as duplicated + different logic for named input equivalency.

Current Logic

Key logic in code commented in code

bool Element::operator==(const Element& rhs) const
{
    if (getCategory() != rhs.getCategory() ||
        getName() != rhs.getName())
    {
        return false;
    }

    // Compare attributes.
    if (getAttributeNames() != rhs.getAttributeNames()) // This is the ordered list. An unordered list option would be useful.
        return false;
    for (const string& attr : rhs.getAttributeNames())
    {
        if (getAttribute(attr) != rhs.getAttribute(attr))
            return false;
    }

    // Compare children.
    const vector<ElementPtr>& c1 = getChildren();
    const vector<ElementPtr>& c2 = rhs.getChildren();
    if (c1.size() != c2.size())
        return false;
    for (size_t i = 0; i < c1.size(); i++)
    {
        if (*c1[i] != *c2[i]) // This array ordered. Getting children by name would resolve this.
            return false;
    }
    return true;
}

Example

File 1

<?xml version="1.0"?>
<materialx version="1.39" colorspace="srgb_texture">
  <nodegraph name="patternGraph">
   <input name="ramp_right" type="color3" value="0, 0, 0" /> 
    <input name="ramp_left" type="color3" value="1, 1, 1" />
    <output name="out" type="color3" nodename="ramplr_color4" />
    <ramplr name="ramplr_color4" type="color3" xpos="1", ypos="2"> 
      <input name="valuel" type="color3" interfacename="ramp_right"/>
      <input name="valuer" type="color3" interfacename="ramp_left"/  />
     </ramplr>
  </nodegraph>
  <gltf_pbr name="gltf_shader" type="surfaceshader">
    <input name="base_color" type="color3" nodegraph="patternGraph" />
  </gltf_pbr>
</materialx>

File 2:

<?xml version="1.0"?>
<materialx version="1.39" colorspace="srgb_texture">
  <gltf_pbr name="gltf_shader" type="surfaceshader"> // Child ordering
    <input name="base_color" type="color3" nodegraph="patternGraph" />
  </gltf_pbr>
  <nodegraph name="patternGraph">
    <output name="out" type="color3" nodename="ramplr_color4" /> // Child ordering
    <input name="ramp_left" type="color3" value="1, 1, 1" /> 
    <input name="ramp_right" type="color3" value="0, 0, 0" /> 
    <ramplr name="ramplr_color4" type="color3" ypos="2" xpos="1" > // Attribute ordering
      <input name="valuel" type="color3" interfacename="ramp_right"/>
      <input name="valuer" type="color3" interfacename="ramp_left"/  />    
  </nodegraph>
</materialx>

jstone-lucasfilm commented 2 months ago

@kwokcb This definitely sounds like it's a design choice, since the order in which input elements are specified will affect the display of a document in an editor. Since we make a point of preserving the order of child elements in MaterialX, it seems consistent that our equivalence operator would respect this order as well.

kwokcb commented 2 months ago

This makes sense for nodes but not so much for nodegraphs especially as they can be dynamically added / removed. Also order of nodes in a graph shouldn't make any different, nor the order of attributes as this should have no affect on display. Perhaps node inputs should be strict on ordering but the rest I don't see any reason for having to match ordering. What do you think ?

jstone-lucasfilm commented 2 months ago

A couple of additional thoughts:

I'd expect both of the cases that you mention to affect the display of a document in an editor, since the order of input elements in a nodegraph affects the display of its interface, and the order of nodes in a nodegraph affects the behavior of auto-layout logic.
Taking a step back, my sense is that element order conveys meaning in MaterialX, which is why we preserve this order when documents are loaded, processed, or saved to disk. Although there may be cases where the order doesn't affect a particular process, in the general case we consider order to be part of the content that a document expresses, and it would seem inconsistent for us to ignore it when comparing the equivalence of two documents.

ld-kerley commented 2 months ago

The MaterialX document serves multiple purposes.

Delivering the shader to a final render target
Delivering the necessary information to construct the UI to further author the material.

In the first case I would certainly hope we're not dependent on any ordering of child elements of attributes, but certainly as @jstone-lucasfilm points out the second case has some places in the document where the order is important.

I can very much imagine a world where I want to compare two MaterialX documents to know if they will render the same image, and thus don't care if inputs are defined in a different order. As well as also wanting to know the documents are actually identical.

Perhaps this suggest we need more than one document comparison function?

jstone-lucasfilm commented 2 months ago

That's a good note, @ld-kerley, and I can certainly imagine a future method that aims to determine whether two MaterialX documents generate identical renders, independent of whether they would behave identically in an artist UI.

kwokcb commented 2 months ago

That is basically what I need.

That is -- is the document functionally equivalent even when not organizationally identical.
For interop, I'm round tripping from MTLX and back to MTLX and one thing that can't be guaranteed is order within the document of nodes.
For the rendering aspect, I've run across this in the past for MTLX->USD->MTLX where your don't have the original anymore after conversion to USD and then to MTLX for code generation.

This issue can be rejigged from an issue to a be a proposal for a new interface if that's agreeable.

jstone-lucasfilm commented 2 months ago

@kwokcb Got it, and that's definitely a fundamentally different task than comparing whether two documents are identical. Just to suggest an approach, what if you were to sort all of the nodegraphs topologically before comparing the documents in your framework?

kwokcb commented 2 months ago

The existing topological sort method cannot be used as it's just connection order sort. (GraphElement.topogicalSort). This is what I tried out:

def getSortedChildren(graphElement, useTopologicalSort=True):

    sortedNames = []
    if useTopologicalSort:
        sorted = graphElement.topologicalSort()
    else:        
        sorted = graphElement.getChildren()
    for node in sorted:
        sortedNames.append(node.getNamePath())

    # sort the names
    if not useTopologicalSort:
        sortedNames.sort()
    return sortedNames

def printSorted(doc, topologicalSort=True):
    sorted = getSortedChildren(doc, topologicalSort)    
    sorted2 = []
    sortedPaths = []
    for (i, nodeName) in enumerate(sorted):
        print(str(i) + ": " + nodeName)
        sortedPaths.append(nodeName)
        node = doc.getChild(nodeName)
        if node.getCategory() == "nodegraph":
            sorted2 = getSortedChildren(node, topologicalSort)
            for j, nodeName2 in enumerate(sorted2):
                print("  " + str(j) + ": " + nodeName2)
                sortedPaths.append(nodeName)

    return sortedPaths

print('----------------------')
print('Use topological sort. Is not name sorted')
print('----------------------')
sorted1 = printSorted(doc)
print('- vs -')
sorted2 = printSorted(doc2)
print('Is equivalent:', sorted1 == sorted2)

print('----------------------')
print('Use getChild + sort. Is name sorted')
print('----------------------')
sorted1 = printSorted(doc, False)
print('- vs -')
sorted2 = printSorted(doc2, False)
print('Is equivalent:', sorted1 == sorted2)

with test files:

<?xml version="1.0"?>
<materialx version="1.39" colorspace="lin_rec709">
  <nodegraph name="unlit_graph">
    <output name="output_color4" type="color3" nodename="ramplr_color4" />
    <ramplr name="ramplr_color4" type="color3">
      <input name="valuel" type="color3" value="1,0.8,0" />
      <input name="valuer" type="color3" value="0,0.5,1" />
      <input name="texcoord" type="vector2" value="0,0" />
    </ramplr>
  </nodegraph>
  <surface_unlit name="unlitshader" type="surfaceshader">
    <input name="emission_color" type="color3" nodegraph="unlit_graph" />
  </surface_unlit>
  <surfacematerial name="MAT_unlitshader" type="material">
    <input name="surfaceshader" type="surfaceshader" nodename="unlitshader" />
  </surfacematerial>
</materialx>

<?xml version="1.0"?>
<materialx version="1.39" colorspace="lin_rec709">
  <surface_unlit name="unlitshader" type="surfaceshader">
    <input name="emission_color" type="color3" nodegraph="unlit_graph" />
  </surface_unlit>
  <nodegraph name="unlit_graph">
    <ramplr name="ramplr_color4" type="color3">
      <input name="valuel" type="color3" value="1,0.8,0" />
      <input name="valuer" type="color3" value="0,0.5,1" />
      <input name="texcoord" type="vector2" value="0,0" />
    </ramplr>
    <output name="output_color4" type="color3" nodename="ramplr_color4" />
  </nodegraph>
  <surfacematerial name="MAT_unlitshader" type="material">
    <input name="surfaceshader" type="surfaceshader" nodename="unlitshader" />
  </surfacematerial>
</materialx>

gives this result:

----------------------
Use topological sort. Is not name sorted
----------------------
0: unlit_graph
  0: unlit_graph/ramplr_color4
  1: unlit_graph/output_color4
1: unlitshader
2: MAT_unlitshader
- vs -
0: unlitshader
1: unlit_graph
  0: unlit_graph/ramplr_color4
  1: unlit_graph/output_color4
2: MAT_unlitshader
Is equivalent: False
----------------------
Use getChild + sort. Is name sorted
----------------------
0: MAT_unlitshader
1: unlit_graph
  0: unlit_graph/output_color4
  1: unlit_graph/ramplr_color4
2: unlitshader
- vs -
0: MAT_unlitshader
1: unlit_graph
  0: unlit_graph/output_color4
  1: unlit_graph/ramplr_color4
2: unlitshader
Is equivalent: True

Thus I think I'll refactor the operator== code into a new function to work in-place as it's the fastest way without revisiting the document in multiple passes. Just requires a attribute name sort and a node child name sort and can reuse the string normalization utility in place as well (or as a pre-pass).

jstone-lucasfilm commented 3 weeks ago

Thanks for this proposal, @kwokcb, as well as for the implementation in https://github.com/AcademySoftwareFoundation/MaterialX/pull/2003!

AcademySoftwareFoundation / MaterialX