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

testrender: Implement basic displacement shader support #1898

Merged

Conversation

fpsunflower
Copy link
Contributor

Description

Associate an (optional) displacement shader with each material. On startup, we execute the displacement for each face-vertex that has a valid displacement shader assigned.

Tests

Added a new test with a displaced sphere to show how this works.

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.

@fpsunflower
Copy link
Contributor Author

Looks like I need to grab updated reference images from the CI machine.

In any case, this PR should be reviewed after we get #1897 merged.

@lgritz
Copy link
Collaborator

lgritz commented Nov 13, 2024

Update: I've merged #1897 now, so you ought to be able to rebase on top of it in main.

@fpsunflower fpsunflower force-pushed the testrender-displacement branch 2 times, most recently from fc3c72b to 663e1fb Compare November 13, 2024 20:15
@lgritz lgritz force-pushed the testrender-displacement branch from 663e1fb to 9f18135 Compare January 15, 2025 20:24
@lgritz
Copy link
Collaborator

lgritz commented Jan 15, 2025

I took the liberty of rebasing this on top of the current main, fixing the formatting problems, and adding one more reference image, and now we are passing all CI but the icx (I think that will need another reference image, checking...) and the gpu test.

The GPU one is tricky -- this PR breaks the build when OptiX is enabled. It's because of the change of SimpleRaytracer::m_shaders from a ShaderGroupRef to a vector<Material> (and a Material is now a ShaderGroupRef for each of surf and disp). I think you made changes in simpleraytracer.cpp related to this, but didn't make the corresponding changes to optixraytracer.cpp, and so now it's confused.

@lgritz
Copy link
Collaborator

lgritz commented Jan 15, 2025

I believe I've updated now with a fixed icx reference image. So the only remaining problem with this PR is the build break when enabling OptiX. Is this something you're able to look at, @fpsunflower ?

@fpsunflower
Copy link
Contributor Author

Yes, sorry I had started to look into this over the break and then got distracted. I'll pick up where you left off.

@lgritz
Copy link
Collaborator

lgritz commented Jan 16, 2025

Thanks, Chris. I think I at least got a bunch of other distractions out of your way.

fpsunflower and others added 6 commits January 19, 2025 12:46
Associate an (optional) displacement shader with each material. On startup, we execute the displacement for each face-vertex
that has a valid displacement shader assigned.

Signed-off-by: Chris Kulla <ckulla@gmail.com>
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Signed-off-by: Larry Gritz <lg@larrygritz.com>

testrender: Implement basic displacement shader support

Associate an (optional) displacement shader with each material. On startup, we execute the displacement for each face-vertex
that has a valid displacement shader assigned.

Signed-off-by: Chris Kulla <ckulla@gmail.com>
Signed-off-by: Chris Kulla <ckulla@gmail.com>
Signed-off-by: Chris Kulla <ckulla@gmail.com>
@fpsunflower fpsunflower force-pushed the testrender-displacement branch from 6c2f2dd to a2d6c5e Compare January 19, 2025 21:05
Signed-off-by: Chris Kulla <ckulla@gmail.com>
Signed-off-by: Chris Kulla <ckulla@gmail.com>
@fpsunflower
Copy link
Contributor Author

Alright this should be good to go now. I believe the OptiX side is hooked up correctly, though I don't have a setup to run it here.

@lgritz
Copy link
Collaborator

lgritz commented Jan 21, 2025

It's really unfortunate that our CI can't (yet) do OptiX tests. Turns out that we had two somewhat recent changes that broke the CI tests on OptiX and needed minor updates. I have fixes in #1926 (posted yesterday and merged this morning) and #1927 (just posted and awaiting review). I can confirm that with the addition of those, your patch here fully build and passes all tests with OptiX, so it's ready to merge!

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.

Looks great!

There were still some remaining failed tests when I tried it, but they were unrelated to your changes and I have separate patches that address them.

@lgritz lgritz merged commit 907ddce into AcademySoftwareFoundation:main Jan 21, 2025
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.

2 participants