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 N/I to globals needed list when backfacing() shadeop is used #1462

Merged

Conversation

sfriedmapixar
Copy link
Contributor

Description

Fix a bug where utilizing the backfacing() shadeop would not mark the N and I globals as needed.

During the OSL runtime optimizations, if we find a backfacing() shadeop, we now add both N and I
to the list of needed globals, as the underlying implementation of that shadeop relies on them.

Tests

This is tested by the globals-needed test. Additionally it adds the ability to filter
out.txt in a test before diff'ing by including a regular expression for the filter in the filter_re variable.
This was so that the output from the debug mode of testshade could be limited to only
the line of interest to this test.

Checklist:

  • [ x] I have read the contribution guidelines.
  • [ x] I have previously submitted a Contributor License Agreement.
  • I have updated the documentation, if applicable.
  • [ x] I have ensured that the change is tested somewhere in the testsuite (adding new test cases if necessary).
  • [x ] My code follows the prevailing code style of this project.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 11, 2022

CLA Signed

The committers are authorized under a signed CLA.

@sfriedmapixar
Copy link
Contributor Author

My EasyCLA manager is out on vacation for the next week, so this may be in limbo for a bit.

@lgritz
Copy link
Collaborator

lgritz commented Feb 11, 2022

Its been like this for a LONG time, I don't think there's a rush.

But I like the fixes. And nice trick with the testsuite option to filter the output!

Copy link
Contributor

@AlexMWells AlexMWells left a comment

Choose a reason for hiding this comment

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

LGTM

@sfriedmapixar
Copy link
Contributor Author

Ok, got my EasyCLA approval through. The bleeding-edge CI failure looks like a main/master rename issue. Do I need to rebase this before you can pull it, or is it good-to-go?

@lgritz
Copy link
Collaborator

lgritz commented Feb 24, 2022

The bleeding edge failure was already fixed by #1463, no need to worry about that. I'll just do the merge.

@lgritz lgritz merged commit c2b1d86 into AcademySoftwareFoundation:main Feb 24, 2022
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