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

OptiX matrix fixes #1485

Merged
merged 7 commits into from
Apr 29, 2022
Merged

Conversation

tgrant-nv
Copy link
Contributor

Description

This PR adds very basic support for named transforms to testrender and testshade. It's a linear scan through a list of transform names to find the corresponding matrix. Although this is a far-from-ideal solution for real world scenarios with many named transforms, it is sufficient to enable further testing and development.

Tests

No new tests have been added, but all previously-passing tests still pass. It would be possible to enable the matrix tests in the testsuite if not for some small differences in error reporting:

CPU:

Testing getmatrix for unknown space name:
ERROR: Unknown transformation "foobar"
  'foobar' matrix not found, as expected

GPU:

Testing getmatrix for unknown space name:
  'foobar' matrix not found, as expected

Checklist:

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

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 12, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@tgrant-nv tgrant-nv force-pushed the optix-fix-matrix branch 2 times, most recently from 8dce1ea to efc8d06 Compare April 22, 2022 22:35
@tgrant-nv
Copy link
Contributor Author

Hopefully this does not conflict too drastically with #1494.

@AlexMWells
Copy link
Contributor

AlexMWells commented Apr 29, 2022

I don't think they have any conflicts, I think testshade and its CMakeList.txt are only files in common, so a merge/rebase might be necessary.

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

@tgrant-nv
Copy link
Contributor Author

Thanks for taking a look, Alex.

@lgritz
Copy link
Collaborator

lgritz commented Apr 29, 2022

I just verified that those two PRs can be merged atop each other without conflict. Will merge this now.

@lgritz lgritz merged commit b0d8cb8 into AcademySoftwareFoundation:main Apr 29, 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