Skip to content

Fix interface input resolution in ShaderGen#2369

Merged
jstone-lucasfilm merged 4 commits into
AcademySoftwareFoundation:mainfrom
kwokcb:interfaceinput_resolve
May 20, 2025
Merged

Fix interface input resolution in ShaderGen#2369
jstone-lucasfilm merged 4 commits into
AcademySoftwareFoundation:mainfrom
kwokcb:interfaceinput_resolve

Conversation

@kwokcb

@kwokcb kwokcb commented May 5, 2025

Copy link
Copy Markdown
Contributor

Issue

When a node is created in shader generation the incorrect values may be stored due to:

  1. A node definition's input interface resolved values are not used.
  2. A node instance's input interface resolved values are not used.

Notes

There were past fixes for proper meta-data passing such as colorspace / units and geometry stream bindings on interface inputs. This addresses the missing evaluation of resolved values.

Fix

  1. On node creation query the resolved value on nodedef interface inputs instead of unresolved values
  2. On node creation check for interface input values and stored resolved ones if found.

Note that the logic for token resolution has not changed, it was just not being called for properly.

This helps with #2361 as now you can connect to downstream materials and have it show up. The node editor itself still has issues with switching between shaders to evaluate which is not present in the viewer. This just makes it so that it will switch and evaluation properly when you choose different downstream materials in the node editor and will evaluation connected material configs properly in the viewer (in general).

Test

Add a new variant for token resolution by adding downstream materials to the previous token test.
The previous test resolves properly for some cases if the interface inputs are at the nodegraph level of an unconnected output.

In order, parent, nodedef, top and sibling level token override tests.

image
image
image
image

@kwokcb

kwokcb commented May 5, 2025

Copy link
Copy Markdown
Contributor Author

@niklasharrysson, @jstone-lucasfilm I think both of you were involved in previous interface input resolve issues so pinging you first.

if (nodeInput)
{
ValuePtr value = nodeInput->getResolvedValue();
if (!value)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Seems interface input values were not being checked at all on node instances.

if (interfaceInput)
{
portValue = interfaceInput->getValue();
portValue = interfaceInput->getResolvedValue();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Interface inputs on nodedefs should get the resolved value.

@kwokcb kwokcb changed the title ShaderGen : Fix interface input value for ShaderGen : Fix resolving interface input values May 5, 2025
@niklasharrysson

Copy link
Copy Markdown
Contributor

@niklasharrysson, @jstone-lucasfilm I think both of you were involved in previous interface input resolve issues so pinging you first.

Thanks Bernard, this looks like a good fix!

@kwokcb

kwokcb commented May 20, 2025

Copy link
Copy Markdown
Contributor Author

Thanks @niklasharrysson , @jstone-lucasfilm let me know what you think. I would be nice to get this in as I think it
may also resolve some of the other existing input interface data passing issues but would need to check.

@jstone-lucasfilm jstone-lucasfilm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This change looks good to me, thanks @kwokcb!

@jstone-lucasfilm jstone-lucasfilm changed the title ShaderGen : Fix resolving interface input values Fix interface input resolution in ShaderGen May 20, 2025
@jstone-lucasfilm jstone-lucasfilm merged commit ffc324b into AcademySoftwareFoundation:main May 20, 2025
32 checks passed
@kwokcb kwokcb deleted the interfaceinput_resolve branch May 21, 2025 14:55
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.

3 participants