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 missing members to ShaderGlobals in rend_lib.h #1721

Merged

Conversation

tgrant-nv
Copy link
Contributor

Description

There is a separate definition of ShaderGlobals in testrender/cuda/rend_lib.h that has drifted away from the definition in include/OSL/shaderglobals.h. Two new members were added in #1702 which changed the byte offset of the output closure Ci, which leads to incorrect shader output since the offset is baked into the PTX.

Adding the new members fixes the shading issue. But this should be regarded as a short-term fix, since the two ShaderGlobals definitions have diverged in other ways that might be significant.

Tests

All OptiX tests passing with the exception of testoptix.optix.opt, which is a preexisting failure.

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.

…haderGlobals in testshade/cuda/rend_lib.h.

Signed-off-by: Tim Grant <tgrant@nvidia.com>
@lgritz
Copy link
Collaborator

lgritz commented Aug 25, 2023

should we have a static_assert somewhere to ensure that these structs don't drift apart inadvertently again?

@tgrant-nv
Copy link
Contributor Author

There are some other differences that I'm not quite sure how to sort out. Such as shaderID being tacked on to the end of the CUDA version. The order of some members is different as well. It would definitely be nice to synchronize them and keep them in sync, though.

@tgrant-nv
Copy link
Contributor Author

I'll reorder the CUDA version to match the "real" version.

…on in shaderglobals.h.

Signed-off-by: Tim Grant <tgrant@nvidia.com>
@tgrant-nv
Copy link
Contributor Author

It was just shadingStateUniform that was a bit out of place. I've rearranged the members so the layout matches shaderglobals.h, with the exception of shaderID. shaderID could probably be tucked into one of the existing pointees, but that would be a bit of a refactor.

Copy link
Collaborator

@lgritz lgritz left a comment

Choose a reason for hiding this comment

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

LGTM, good catch!

@lgritz lgritz merged commit 05f4ace into AcademySoftwareFoundation:main Aug 25, 2023
22 of 23 checks passed
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.

None yet

2 participants