-
Notifications
You must be signed in to change notification settings - Fork 360
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 testrender OptiX build #1869
Fix testrender OptiX build #1869
Conversation
* Refactor the accel build and pipeline creation. * Remove the old quad & sphere code. * Change globals_from_hit to use the mesh data. Signed-off-by: Tim Grant <[email protected]>
…test_str_ops, which has been broken since the change to string hashes. Signed-off-by: Tim Grant <[email protected]>
205bdbf
to
ecba4d5
Compare
Signed-off-by: Tim Grant <[email protected]>
Signed-off-by: Tim Grant <[email protected]>
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.
LGTM!
Since our CI only builds OptiX but doesn't test it, I'll need to test it on my work machine (will try later today), and if that works, I will merge. |
Passes my tests. But on the other hand, it looks like the testsuite isn't actually running the "render" tests in optix mode because those files don't have the "OPTIX" file tag within them. So this is certainly building, but it's not really being tested. What's the relationship between this and #1829? Should this be merged first and then the other will be rebased and looked at again? Are there parts of the (much more extensive) description message body for 1829 that should be included here to reflect the refactoring that's present in both? |
This doesn't actually work. Try:
I just see solid cyan for the spheres Is this expected to work with this PR? Or is this only expected to build and it's #1829 that will make it work? |
If you run Yes, I'll need to more or less re-implement #1829 before I can re-open that PR. But that's probably for the best, since this PR takes care of a fair chunk of the host-side refactoring, so the rendering change will be a bit more comprehensible. There's nothing from the other description that needs to be restated here. |
None of the |
OK, great, just wanted to be sure this PR had the functionality we expected. I'll merge! |
185a3a8
into
AcademySoftwareFoundation:main
Description
This PR restores build and run-time functionality for the OptiX path in
testrender
following #1865. It includes much of the host code refactoring from #1829. Having only one geometry type is a nice simplification.Tests
No new tests have been added, but I did update the reference images to reflect slight changes from the tessellated geometry. I also updated the expected output for
test_str_ops
, which has been broken since the change to string hashes.Checklist:
already run clang-format v17 before submitting, I definitely will look at
the CI test that runs clang-format and fix anything that it highlights as
being nonconforming.