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

Handle precedence of generic and target-specific implementations #1430

Conversation

kwokcb
Copy link
Contributor

@kwokcb kwokcb commented Jul 26, 2023

Fixes #1409

Scenario

It is possible to have a implementation in a derived target which overrides a non-target implementation. The
logic did not try to find implementations with non-empty targets before defaulting to returning an implementation
without a target.

Fix

When dealing with an explicit target on definition search. First test for implementations which have a non-empty target string.
If not found then fallback to non-target implementations. This should fix returning non-target implementations before trying to find implementations with targets.

Test

import MaterialX as mx

# Create a document.
doc = mx.createDocument()

# Create a base and derived targetdef
base_targetDef = doc.addTargetDef("base")
derived_targetDef = doc.addTargetDef("derived")
derived_targetDef.setInheritString('base')

# Create an implementation for the base targetdef
base_impl = doc.addImplementation("IM_function_base")
base_impl.setTarget("base")
base_impl.setNodeDefString("ND_function")

# Create an implementation for the derived targetdef
derived_impl = doc.addImplementation("IM_functon_derived")
derived_impl.setTarget("derived")
derived_impl.setNodeDefString("ND_function")

# Create a nodedef
nodeDef = doc.addNodeDef("ND_function", "float3", "function")

which creates this documet:

<materialx version="1.38">
  <targetdef name="base">
  <targetdef name="derived" inherit="base">
  <implementation name="IM_function_base" target="base" nodedef="ND_function">
  <implementation name="IM_functon_derived" target="derived" nodedef="ND_function">
  <nodedef name="ND_function" node="function">
    <output name="out" type="float3">
</materialx>

The first test returns "derived" and "base", the second before the change returns "" and "". The derived result is incorrect.
After the change, the derived result is "derived" as expected.

# Test with targets specified for base and derived
impl = nodeDef.getImplementation('derived')
print('Got derived target:', impl.getTarget())
impl = nodeDef.getImplementation('base')
print('Got base target: ', impl.getTarget())

# Test with base without a target
base_impl.setTarget('')
impl = nodeDef.getImplementation('derived')
print('Want derived, but got target: "%s"' % impl.getTarget())
impl = nodeDef.getImplementation()
print('Got no target: "%s"' % impl.getTarget())

@dcourtois. Can you see if this will fix your issue? Thanks.

@niklasharrysson, @jstone-lucasfilm : This is the least intrusive change I could think of which does not change
the logic of targetStringsMatch as that method is used in other places.

… for implemtnations which have a non-empty target string.

If not found then fallback to non-target implementations.
This should fix returning non-target implementations before trying to find implementations with targets.
@dcourtois
Copy link

@kwokcb Hi, sorry for the delay in my answer! I finally got to test this, and yes, it fixes my issue and I can finally get rid of my uggly workaround when I use this pull request :)

@kwokcb
Copy link
Contributor Author

kwokcb commented Aug 3, 2023

Hi @dcourtois,
No problem -- everyone is busy :).
Good to hear this fixes things. I'll await review on this.

@jstone-lucasfilm jstone-lucasfilm changed the title Handle finding implementations with a target which override implementations without a target Handle precedence of generic and target-specific implementations Sep 1, 2023
Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks @kwokcb!

@jstone-lucasfilm jstone-lucasfilm merged commit 5a781fa into AcademySoftwareFoundation:main Sep 1, 2023
13 checks passed
@kwokcb kwokcb deleted the inherited_target_impl_search branch September 6, 2023 20:32
Michaelredaa pushed a commit to Michaelredaa/MaterialX that referenced this pull request Oct 21, 2023
…demySoftwareFoundation#1430)

When dealing with an explicit target on definition search. First test for implementations which have a non-empty target string.
If not found then fallback to non-target implementations. This should fix returning non-target implementations before trying to find implementations with targets.
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

Successfully merging this pull request may close these issues.

Issues overriding OSL shader nodes implementations.
3 participants