Regression in implementation search logic for node definitions

[kwokcb]

Issue

This is a regression from 1.39.4 to 1.39.5.

1. In 1.39.4: Nodedef.getImplementation() will return the NodeGraph when the assocaition is stored in an implementation. This is the case for both standard surface definitions.
2. In 1.39.5: Nodedef.getImplemetnation() will return the Implementation association instead. It no longer finds the NodeGraph defined in the association

Steps:

This script will supports scanning the libraries either from a install location or a specification location. It was used to test a 1.39.4 install and the current 1.39.5 build.

 import MaterialX as mx
import os
import argparse

parser = argparse.ArgumentParser(description="Build MaterialX NodeDef Introspection JSON")
parser.add_argument('-ip', '--input_path', type=str, default='', help="Input path for MaterialX libraries. If not specified will use default search path.")
args = parser.parse_args()

input_path = args.input_path
# Convert opts.library_folder from set to list
library_list = []

path_list = []
if not input_path:

    searchPath = mx.getDefaultDataSearchPath().asString() 
    searchPath = os.path.normpath(searchPath)
    # Append 'libraries' to the search path
    searchPath = mx.FileSearchPath(os.path.join(searchPath, 'libraries'))
    path_list.append(searchPath.asString())

    libraryPath = mx.getDefaultDataSearchPath()
    libraryFolders = mx.getDefaultDataLibraryFolders()

    version = mx.getVersionString()

else:
    libraryFolders = ['libraries']
    searchPath = os.path.normpath(input_path)
    path_list.append(searchPath)
    for lib in libraryFolders:
        path_list.append(os.path.join(searchPath, lib))
    
    libraryPath = mx.FileSearchPath(searchPath)

print("Library folders", libraryFolders)
print("Search paths:", path_list)
print("Library path:", libraryPath.asString())
stdlib = mx.createDocument()
libFiles = mx.loadLibraries(libraryFolders, libraryPath, stdlib)
#for l in libFiles:
#    print("Loaded library file:", l)

# Create main document and import the library document
doc = mx.createDocument()
doc.importLibrary(stdlib)

for nodedef in doc.getNodeDefs():
    nodedef_name = nodedef.getName()

    # Add implementation children if they exist
    implementation = nodedef.getImplementation()
    if implementation:

        # Add nodegraph
        if implementation.isA(mx.NodeGraph):
            pass # print("Add nodegraph implementation for nodedef:", nodedef_name)

        else:
            # Add nodegraph implementation
            node_graph_attr = implementation.getAttribute('nodegraph')
            if node_graph_attr:
                print('FAILED to find nodegraph. Scan manually:', node_graph_attr)
                print(mx.prettyPrint(implementation))
                implementation = doc.getNodeGraph(node_graph_attr)
            else:
                pass
                # Add non-nodegraph implementation
                #print("Add non-nodegraph implementation for nodedef:", nodedef_name, "name", implementation.getNamePath())
    else:
        print(f'-- No implementation for nodedef: {nodedef_name}')

Results 1.39.4

This is taking the local library location from main without any modifications.

python ng_test.py -ip d:\work\materialx\ILM_materialx
Library folders ['libraries']
Search paths: ['d:\\work\\materialx\\ILM_materialx', 'd:\\work\\materialx\\ILM_materialx\\libraries']
Library path: d:\work\materialx\ILM_materialx
-- No implementation for nodedef: ND_volumematerial

Results 1.39.5:

python ng_test.py -ip d:\work\materialx\ILM_materialx
Library folders ['libraries']
Search paths: ['d:\\work\\materialx\\ILM_materialx', 'd:\\work\\materialx\\ILM_materialx\\libraries']
Library path: d:\work\materialx\ILM_materialx
FAILED to find nodegraph. Scan manually: NG_standard_surface_surfaceshader_100
<implementation name="IMPL_standard_surface_surfaceshader_101" nodedef="ND_standard_surface_surfaceshader" nodegraph="NG_standard_surface_surfaceshader_100">

FAILED to find nodegraph. Scan manually: NG_standard_surface_surfaceshader_100
<implementation name="IMPL_standard_surface_surfaceshader_100" nodedef="ND_standard_surface_surfaceshader_100" nodegraph="NG_standard_surface_surfaceshader_100">

[kwokcb]

@ld-kerley , the cause is the change in logic for this check-in:

https://github.com/AcademySoftwareFoundation/MaterialX/commit/efd5fe591bea091f3f283320034d305306a24d4c

Can you comment as to why this was done ?

[kwokcb]

Note that the historical precendence is:

1. Standard surface would return a nodegraph as implementation when there was 1 version
2. It still returned a nodegraph with multiple versions
3. Now it returns the implementation "glue" element.

[ld-kerley]

I changed this because I needed a way to access the corresponding <implementation> name for each given <nodedef>. In the standard surface case we have two different node definitions, and this to build corresponding OSOs for the OslNetworkShaderGenerator, I needed API access to the associated implementations.

It feels important that the Core API allows direct access to the full data model - without "hiding" intermediate pieces.

Perhaps a middle-ground compromise here is to introduce a new API call, something like getActiveImplementation(). Which parallels the getActiveInputs() which resolves any inheritance?

[kwokcb]

The implementations and nodedef association logic already hides structural details. The issue I see is when getting an implementation it is historically expected to return the actual implementation. Now the api user needs to know that there may be a nodegraph attribute which will redirect to the actual implementation.

I guess this could be hidden behind a new Api, but existing external code will still break, and any internal code needs to be remapped to assure the same behaviour.

For the Oso id, can you not use the version number + existing name to disambiguate?

[ld-kerley]

I think any number of different things could be used for the OSO name - but the OSO really is the implementation for the OSLNetwork generator - so it felt the most natural (and future proof) to derive the name from the corresponding OSL source implementation name. If we have multiple OSONetwork generators in the future - we're going to need something that includes the _getoslnetwork target token.

If we're worried about breaking existing API usage - then what about if we invert the logic and keep the existing function calling what I was proposing calling getActiveImplementation() and introduce a new API call to get the actual implementation element? Though I'm not totally clear what we would call that function. Doing this would have the benefit that we can revert some changes made in the shader generation code, and restore the document level caching of the actual implementation (even if its the nodegraph thats been indirected from the implementation element).

[kwokcb]

Hi @ld-kerley, Sorry about being a stickler on this but I think your last proposal would be best.

• How about getUnmappedImplementation() for the name of the new function.
• The cache can store both these "mappers" so there is no performance penalty for accessing either above some tiny additional cache storage. ( BTW, for functional nodedef utilities, I'd like to add the ability to find graph reuse -- will ping offline. )

[kwokcb]

Here's some prototype code that I think should work. direct_mapped_nodefs is what's currently stored. indirect_mapped_nodedefs would be the additional cached mapping. If you don't want to do this, then the OSL reference generator could build this locally.

indirect_mapped_nodedefs = {}
direct_mapped_nodefs = {}

for ndef in stdlib.getNodeDefs():
    impl = ndef.getImplementation()
    if impl:
        if impl.isA(mx.NodeGraph) :
            direct_mapped_nodefs[ndef.getName()] = impl
            continue
        elif impl.hasAttribute("nodegraph"):
            indirect_mapped_nodedefs[ndef.getName()] = impl    

print("Directly mapped nodedefs count:", len(direct_mapped_nodefs))
print("Indirectly mapped nodedefs count:", len(indirect_mapped_nodedefs))
print("Indirectly mapped nodedefs:")
for ndef_name, impl in indirect_mapped_nodedefs.items():
    print(f"- nodedef: {ndef_name} -> implementation: {impl.getNamePath()}")

def getUnappedImplementation(find_nodedef_name, indirect_mapped_nodedefs):
    if find_nodedef_name in indirect_mapped_nodedefs:
        return indirect_mapped_nodedefs[find_nodedef_name]
    return None

impls = stdlib.getImplementations()
for ndef in stdlib.getNodeDefs():
    ndef_name = ndef.getName()
    unmapped_impl = getUnappedImplementation(ndef_name, indirect_mapped_nodedefs)
    if unmapped_impl:
        print(f"- found unmapped implementation {unmapped_impl.getNamePath()} for nodedef: {ndef_name}")

Result:

Directly mapped nodedefs count: 264
Indirectly mapped nodedefs count: 2
Indirectly mapped nodedefs:
- nodedef: ND_standard_surface_surfaceshader -> implementation: IMPL_standard_surface_surfaceshader_101
- nodedef: ND_standard_surface_surfaceshader_100 -> implementation: IMPL_standard_surface_surfaceshader_100
- found unmapped implementation IMPL_standard_surface_surfaceshader_101 for nodedef: ND_standard_surface_surfaceshader
- found unmapped implementation IMPL_standard_surface_surfaceshader_100 for nodedef: ND_standard_surface_surfaceshader_100