Skip to content

Conversation

@aconty
Copy link
Contributor

@aconty aconty commented Jul 25, 2025

Description

This moves the dielectric implementation to BSDL and adds energy conservation via Turquin style multiple scattering. That is, we create a microfacet based reflection refraction model where we precumpute the energy loss to a lookup table. Then at render time we renormalize the lobe to remove the energy loss. This is a roughness wedge just using this technique.

mtx_clear_vd8

In addition we use Bounded Visible Normal sampling to avoid rejecting reflection samples. This improves noise significantly at the cost of losing valid refracted directions. This doesn't produce energy loss thanks to the renormalization, but biases the the refraction for high roughness and low IOR. The result is still plausible and very hard to see which one is correct.

mtx2_clear_vd8

Here is with higher IOR and some color:

mtx_plastic_vd8

If you have a spectral render or a hacky backport for dispersion you can get some color aberration, this BSDF can do wavelength dependent IOR.

mtx_cleardisp_vd8

Also keeping the ability to layer a reflection only coating on top of other BSDFs via layer().

mtx_coat_vd8

Tests

Updated existing tests.

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.

This moves the dielectric implementation to BSDL and adds energy
conservation via Turquin style multiple scattering. That is, we create a
microfacet based reflection refraction model where we precumpute the
energy loss to a lookup table. Then at render time we renormalize the
lobe to remove the energy loss.

In addition we use Bounded Visible Normal sampling to avoid rejecting
reflection samples. This improves noise significantly at the cost of
losing valid refracted directions. This doesn't produce energy loss
thanks to the renormalization, but biases the the refraction for high
roughness and low IOR. The result is still plausible and very hard to
see which one is correct.

Also keeping the ability to layer a reflection only coating on top of
other BSDFs via layer().

Signed-off-by: Alejandro Conty <[email protected]>
@fpsunflower
Copy link
Contributor

Great stuff! I hadn't thought of just ignoring the bias from bounded VNF in the refraction case, its true that its hard to tell which image is "correct". At the same time, looking at the images, I would say the difference is noticeable enough that I am worried about compatibility with other renderers (since this is meant to be a reference for MaterialX and others).

Maybe it could be a compile time decision? Or maybe compatibility with other systems if more important overall?

Do you have the comparison of the new approach to the previous one with extra (reciprocal) diffuse lobes?

@lgritz
Copy link
Collaborator

lgritz commented Jul 25, 2025

I took the liberty of pushing a 2-line update to your branch that adjusts the test parameters to allow a small number of pixels to fail in the render-mx-dielectric-glass test. Let's see how it does on the CI run now, it seemed to do the trick in my fork.

I would rather have made it as a code change suggestion as part of a review, which you could then look at and click whether or not to accept without my needing to actually alter your fork, but GH won't let me do that. I really wish that GH allowed one to make code edit suggestions to lines that were not directly part of the PR. Quite often the correct suggestion is to add something that was left out!

@lgritz
Copy link
Collaborator

lgritz commented Jul 25, 2025

The Windows failures are "expected" -- all branches are currently failing the Windows tests for reasons unrelated to your PR.

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.

The code itself LGTM, but I'll leave it to @fpsunflower and @aconty (and any other experts with an opinion) to hash out acceptance of the implementation details.

@aconty
Copy link
Contributor Author

aconty commented Jul 26, 2025

Thanks, Larry!

Chris, do you mean the SPI model? yes, I do have it here

spi2_clear_vd8

Remember this model incurs in energy gain with many bounces + TIR. Our math for the balance is not perfect, specially when using transparent shadows. Take into account both SPI and this pass a furnace test.

I do like the idea of making it a compile time option. I would make it runtime if it wasn't for the duplicated albedo tables + intuition that people will like cleaner best. Let me make it compile option.

Signed-off-by: Alejandro Conty <[email protected]>
@fpsunflower
Copy link
Contributor

That is quite different! I haven't done a comparison on my end to the "ground truth" Heitz model, but it would be interesting to know which is closest.

Given how far apart these two version of multiple scattering are, maybe the smaller difference from the Bounded VNF is not that actually that big ...

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

There is still some CI stuff to sort out, but that should just be a matter of tweaking the reference thresholds or adding additional reference images.

@lgritz
Copy link
Collaborator

lgritz commented Jul 26, 2025

There is still some CI stuff to sort out, but that should just be a matter of tweaking the reference thresholds or adding additional reference images.

No, the CI is totally passing now, except for the Windows builds which are failing for the llvm vs msvs version we discussed last week.

@aconty
Copy link
Contributor Author

aconty commented Jul 27, 2025

Sounds good, ok to merge?

@lgritz lgritz merged commit a76c42d into AcademySoftwareFoundation:main Jul 28, 2025
24 of 26 checks passed
@lgritz
Copy link
Collaborator

lgritz commented Jul 28, 2025

Alex, do you need a backport and internal build?

@aconty
Copy link
Contributor Author

aconty commented Jul 28, 2025

No need, no. Thanks!

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