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

Modernize sampler in testrender #1534

Merged
merged 1 commit into from
Jul 19, 2022

Conversation

fpsunflower
Copy link
Contributor

The sampler implementation in testrender was very close to the 2020 article from Brent Burley. This patch
catches up to best practices, including a faster owen scrambler from Nathan Vegdahl and recognizing that
the index scrambling can be achived by the same owen scrambler method (which saves a bunch of logic and runs
faster). Finally, avoid the redundant bit reversals and use compiler intrinsics when available to benefit
from hardware instructions (for ARM or GPUs). Finally the integer hash constants were updated to the latest
from the hash prospector project.

While more optimizations are certainly possible, this is a reasonable balance between quality and speed and saves a few lines of code over the previous implementation.

On my laptop, running the microfacet sample from the testsuite at 1024x768 with 4x4 samples goes from 10.2s down to 8.9s (which is roughly 12% faster).

@lgritz
Copy link
Collaborator

lgritz commented Jul 17, 2022

Wow, the sample scrambler itself was accounting for 10% of runtime??

@fpsunflower
Copy link
Contributor Author

Yep! Now that the faster methods are well documented in the literature, might as well use them :)

@lgritz
Copy link
Collaborator

lgritz commented Jul 18, 2022

It's failing one of the unit tests. I know you're looking into that separately, but in the mean time, should we update the ref images (you can have more than one ref image for a test -- the test passes if it matches any of the choices, so sometimes we have multiple that we consider "correct" if we just can't make all platforms match exactly)? Or if it's just one or two pixels we can adjust the amount of allowable difference? Or would you like to simply hold off on merging this until you fully solve #1535?

@fpsunflower
Copy link
Contributor Author

It seems its always the same test giving us trouble. I have one or two more things to try, otherwise I'll just commit this with an extra reference image.

The sampler implementation in testrender was very close to the 2020 article from Brent Burley. This patch
catches up to best practices, including a faster owen scrambler from Nathan Vegdahl and recognizing that
the index scrambling can be achived by the same owen scrambler method (which saves a bunch of logic and runs
faster). Finally, avoid the redundant bit reversals and use compiler intrinsics when available to benefit
from hardware instructions (for ARM or GPUs). Finally the integer hash constants were updated to the latest
from the hash prospector project.

While more optimizations are certainly possible, this is a reasonable balance between quality and speed.

Update reference images for new sampler

Signed-off-by: Chris Kulla <[email protected]>
@lgritz
Copy link
Collaborator

lgritz commented Jul 19, 2022

I've had trouble with that test before, too. I was never able to figure out why that one, in particular, was so brittle to minor platform differences. And not just a couple LSB errors, it was something that was changing where they rays were going.

@fpsunflower fpsunflower merged commit 95fa78d into AcademySoftwareFoundation:main Jul 19, 2022
lgritz pushed a commit to lgritz/OpenShadingLanguage that referenced this pull request Jul 23, 2022
The sampler implementation in testrender was very close to the 2020 article from Brent Burley. This patch
catches up to best practices, including a faster owen scrambler from Nathan Vegdahl and recognizing that
the index scrambling can be achived by the same owen scrambler method (which saves a bunch of logic and runs
faster). Finally, avoid the redundant bit reversals and use compiler intrinsics when available to benefit
from hardware instructions (for ARM or GPUs). Finally the integer hash constants were updated to the latest
from the hash prospector project.

While more optimizations are certainly possible, this is a reasonable balance between quality and speed.

Update reference images for new sampler

Signed-off-by: Chris Kulla <[email protected]>
chellmuth pushed a commit to chellmuth/OpenShadingLanguage that referenced this pull request Sep 6, 2024
Relevant changes:

  * GPU string fix -- needed missing cast (AcademySoftwareFoundation#1553)
  * Fix typo error that prevented correct typecheck of ternary (AcademySoftwareFoundation#1552)
  * testrender now supports the standard MaterialX closure:
  * testrender improvements: modernize sampler (AcademySoftwareFoundation#1534), switch to cone
    tracing (AcademySoftwareFoundation#1543), support MaterialX closures (AcademySoftwareFoundation#1533, AcademySoftwareFoundation#1536, AcademySoftwareFoundation#1537,
    AcademySoftwareFoundation#1538, AcademySoftwareFoundation#1541, AcademySoftwareFoundation#1542, AcademySoftwareFoundation#1547)
  * Bump LLVM to 14.0
  * Help reduce PTX nondeterminism (AcademySoftwareFoundation#1570)

See merge request spi/dev/3rd-party/osl-feedstock!30
@fpsunflower fpsunflower deleted the new-sampler branch October 28, 2024 03:43
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