-
Notifications
You must be signed in to change notification settings - Fork 373
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
Fix userdata derivatives for interpolated params on GPU #1685
Fix userdata derivatives for interpolated params on GPU #1685
Conversation
|
Pascal, you can fix the DCO by adding a sign-off:
For the EasyCLA, it seems you weren't on the SPI approved list. (How did that happen? Is this really your first OSL PR?) Anyway, I added you, so you should see something to click that will let you accept it and they it will be cleared for this and any future PRs. |
Don't worry about the two test failures for icc and icx -- that's just Intel's servers being flaky and the CI runs unable to download those compilers. It comes and goes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code change LGTM, thanks for the fix.
I need the CLA and DCO fixed before I can merge.
d8c2c60
to
0bc7384
Compare
Yes, very first OSL PR, I only contributed to OIIO so far. For the EasyCLA should I proceed as a Corporate or Individual contributor? |
Corporate, tell it you're with Sony Imageworks, I've already put you on our list. |
I completed the CLA authorization request. Is there something else I need to do for getting the CLA failure resolved? Another push force commit? |
I'm not sure, I'll try to figure it out. |
Hmmm, can you just diddle it and re-push:
maybe that will get it unstuck. |
0bc7384
to
3473d97
Compare
I re-pushed the branch but same thing. The EasyCLA process says this: "The Corporate CLA process requires you to be approved by your organization's CLA Manager. A CLA Manager at your organization will receive your request via email immediately after you submit it. To expedite the process, you can follow up with them directly.". Maybe you (or someone else?) have receive an email for approving the authorization? |
I am the CLA manager for the company. I see you already on the approved list, and there are no additional requests waiting for my approval. If the rest of the CI passes, I may be able to just force a merge and override the check. I just don't know why EasyCLA is still giving me trouble. @jmertic do you have any insights? |
Pascal, is it possible that the email associated with the commit doesn't match the email associated with your github account? |
Oh you're right! I didn't realize that the commit was signed off with my Imageworks email instead of my gmail address. I'll look into it tomorrow. |
I think you should be able to fix with
and then re-push --force But also, I just added your work email to the permissions list, so it may just work with another force push? (Though changing the email to be consistent will make it so that future PRs all look like the same person instead of you appearing under multiple aliases.) |
Signed-off-by: Pascal Lecocq <pascal.lecocq@gmail.com>
3473d97
to
a5342ab
Compare
I setup my personal email for that specific repo in the .git/config using a [user] tag. I just pushed another commit, let's see how it goes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM. I know you're authorized under our CLA, so I'm going to force the merge even if it doesn't work this time. We'll figure it out next time if it's still not working (it may be fine if a PR is first started after you have been added and with the emails consistent).
Highlights: * GPU/OptiX support of ReParameter (AcademySoftwareFoundation#1686) * Fix OptiX userdata derivatives for interpolated params on GPU (AcademySoftwareFoundation#1685) * Fix userdata binding corner case (AcademySoftwareFoundation#1673) * Fix constant float values being converted to ints (AcademySoftwareFoundation#1674) Restock the abi dump file as we push to 1.13.4. See merge request spi/dev/3rd-party/osl-feedstock!48
Description
Following #1657, it turns out we forgot to zero initialize the userdata derivatives for the optix path.
This leads to some rendering glitches caused by the memory garbage left in those derivatives.
Tests
Checklist: