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

Add null check in getShaderNodes #2228

Merged

Conversation

jstone-lucasfilm
Copy link
Member

This changelist adds a missing null check in getShaderNodes, handling the edge case of an invalid output string. Previously, this edge case would trigger a crash in MaterialXCore, and now it correctly generates validation warnings and proceeds.

This changelist adds a missing null check in getShaderNodes, handling the edge case of an invalid output string.  Previously, this edge case would trigger a crash in MaterialXCore, and now it correctly generates validation warnings and proceeds.
@jstone-lucasfilm jstone-lucasfilm merged commit e13344b into AcademySoftwareFoundation:main Feb 14, 2025
34 checks passed
@ld-kerley
Copy link
Contributor

In cases like this where we find a bug I think we should add a unit test to exercise the fix at the same time as submitting the fix - that way we will try and ensure that we don't regress. Do you think we can add a small unit test here to simulate the bad input data?

@jstone-lucasfilm
Copy link
Member Author

I was thinking along the same lines, @ld-kerley, though we'll need to create a bit more infrastructure to allow for "invalid" documents in our TestSuite. Two potential paths forward come to mind:

  1. We could create a new "invalid" folder in our TestSuite, which would be loaded along with other examples during testing, but would not require a passing result in validation. As of today, we require all of our TestSuite examples to pass validation, so adding even a single invalid file would be a breaking change.
  2. We could extend our fuzz testing system so that it consistently generates this category of error procedurally. This would require more work, but would potentially catch a much wider set of error conditions, not just the invalid output string that inspired this fix.

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.

2 participants