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

Fix testrender OptiX build #1869

Merged

Conversation

tgrant-nv
Copy link
Contributor

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:

  • I have read the contribution guidelines.
  • 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. If I haven't
    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.

 * 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]>
@tgrant-nv tgrant-nv force-pushed the testrender-triangles-optix-fix branch from 205bdbf to ecba4d5 Compare September 21, 2024 02:09
@tgrant-nv tgrant-nv requested a review from lgritz September 21, 2024 21:42
Copy link
Contributor

@fpsunflower fpsunflower left a comment

Choose a reason for hiding this comment

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

LGTM!

@lgritz
Copy link
Collaborator

lgritz commented Sep 22, 2024

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.

@lgritz
Copy link
Collaborator

lgritz commented Sep 24, 2024

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?

@lgritz
Copy link
Collaborator

lgritz commented Sep 24, 2024

This doesn't actually work. Try:

cd testsuite/render-microfacet
testrender -r 320 240 -aa 8 -O2 --optix scene.xml out.exr

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?

@tgrant-nv
Copy link
Contributor Author

If you run ctest -R testoptix, it should run a handful of tests that exercise the OptiX path in testrender.

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.

@tgrant-nv
Copy link
Contributor Author

None of the render-* test cases will currently work in OptiX mode. As things stand now, shading in OptiX is limited to returning the albedo color (at best). #1829 adds full path tracing and BSDF support, but that will probably have to wait until after Dev Days. It's a sprawling change.

@lgritz
Copy link
Collaborator

lgritz commented Sep 24, 2024

OK, great, just wanted to be sure this PR had the functionality we expected. I'll merge!

@lgritz lgritz merged commit 185a3a8 into AcademySoftwareFoundation:main Sep 24, 2024
21 of 22 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.

3 participants