Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal : Provide a Functionally equivalent operator #1987

Closed
kwokcb opened this issue Aug 23, 2024 · 9 comments
Closed

Proposal : Provide a Functionally equivalent operator #1987

kwokcb opened this issue Aug 23, 2024 · 9 comments

Comments

@kwokcb
Copy link
Contributor

kwokcb commented Aug 23, 2024

Proposal

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

  1. The order of attributes for an element differs
  2. 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
Copy link
Member

@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
Copy link
Contributor Author

kwokcb commented Aug 23, 2024

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

kwokcb commented Aug 23, 2024

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
Copy link
Member

@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
Copy link
Contributor Author

kwokcb commented Aug 24, 2024

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).

@kwokcb kwokcb changed the title Equivalence operator fails if order of attributes or children differs Proposal : Provide a Functionally equivalent operator Aug 24, 2024
@jstone-lucasfilm
Copy link
Member

Thanks for this proposal, @kwokcb, as well as for the implementation in #2003!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants