Skip to content

fix(bsdl): Correct colored Fresnel sampling - #2158

Open
tdavidovicNV wants to merge 1 commit into
AcademySoftwareFoundation:mainfrom
tdavidovicNV:fix/mtx-colored-fresnel-sampling
Open

fix(bsdl): Correct colored Fresnel sampling#2158
tdavidovicNV wants to merge 1 commit into
AcademySoftwareFoundation:mainfrom
tdavidovicNV:fix/mtx-colored-fresnel-sampling

Conversation

@tdavidovicNV

Copy link
Copy Markdown
Contributor

Assisted-by: OpenAI Codex / GPT-5.6

Description

The BSDL sampler shared by testrender's MaterialX dielectric and
generalized-Schlick closures selects reflection with probability max(F) and
transmission with the remaining probability. For RGB F = (0.2, 0.5, 0.8),
this gives a reflection probability of 0.8 and a transmission probability of
0.2. The transmission evaluation instead uses max(1 - F) = 0.8 as the
transmission probability when computing the PDF and sample weight. A
transmission sample selected with probability 0.2 is therefore evaluated as
though it had been selected with probability 0.8.

This change restores the arithmetic-average selection used by testrender's
original implementation of the MaterialX generalized-Schlick closure for
neutral reflection and transmission tints:

pR = average(F);
pT = 1 - pR;

For the example above, this gives pR = 0.5 and pT = 0.5.

This is also consistent with testrender's BSDF lobe-selection policy. With
neutral path throughput, path-weighted albedo selection reduces to the
arithmetic average over the active channels.

The same complementary probabilities are used for event selection, the
reported PDF, and the reciprocal sample weight. In particular, once reflection
is selected with probability pR, its returned weight must include 1 / pR
because the full sampling density is pR times the conditional microfacet
density.

The internal dielectric helper receives lambda_0 so that the average covers
three channels in RGB mode and four channels in spectral mode. Fresnel may
return Power::UNIT(), which populates all four lanes, so the inactive RGB
lane is masked before averaging. No separate scatter-mode handling is
introduced.

Open question for review: this change adds a required argument to the
header-only DielectricBSDF constructor. Defaulting it to 0 would preserve
existing RGB call sites but would silently treat existing spectral call sites
as RGB. Should direct constructor users be required to pass lambda_0, or
should an RGB-default compatibility overload be provided?

This is an internal BSDL sampling correction with no OSL language change, so no
documentation update is needed.

Tests

Local validation on Windows:

  • CMake configuration completed successfully.
  • The Release genluts, testrender, and oslc targets built successfully;
    genluts regenerated all BSDL lookup tables.
  • The focused unit_bsdl test passes. It covers RGB and spectral averaging,
    inactive RGB-lane masking, the colored reflection/transmission sampling
    boundary, and the corresponding PDF and reciprocal-weight factors.
  • The existing generalized-Schlick tests compiled their shaders and rendered
    their output images. CTest could not compare the images because the locally
    configured idiff and oiiotool executables are unavailable.
  • git diff --check passes.
  • Fork CI
    completed for commit 717d5aaf: 22 of 23 matrix jobs passed. This includes
    clang-format 17, and unit_bsdl passed across the build/test configurations.
  • The sanitizer job failed during dependency setup, before OSL was built,
    because the cached TIFF::tiff target references a missing WebP::webp
    target. This is unrelated to the change.

Codex was used to trace the sampling/PDF mismatch, compare the correction with
the original MaterialX implementation and other renderer policies, and draft
the fix and PR description. I reviewed the resulting changes and build output.

Checklist

  • I have read the guidelines on contributions and code review procedures.
  • I have read the Policy on AI Coding Assistants
    and if I used AI coding assistants, I have an Assisted-by: TOOL / MODEL
    line in the pull request description above.
  • I have updated the documentation if my PR adds features or changes
    behavior.
  • I am sure that this PR's changes are tested in the testsuite.
  • I have run and passed the testsuite in CI before submitting the
    PR, by pushing the changes to my fork and seeing that the automated CI
    passed there. (Exceptions: If most tests pass and you can't figure out why
    the remaining ones fail, it's ok to submit the PR and ask for help. Or if
    any failures seem entirely unrelated to your change; sometimes things break
    on the GitHub runners.)
  • My code follows the prevailing code style of this project and I
    fixed any problems reported by the clang-format CI test.

Restore the arithmetic-mean proposal used by the original testrender MaterialX sampler, and use its complement consistently for transmission PDFs and sample weights.

Assisted-by: OpenAI Codex / GPT-5.6

Signed-off-by: Tomas Davidovic <tdavidovic@nvidia.com>
@tdavidovicNV

Copy link
Copy Markdown
Contributor Author

The two failures seem completely unrelated. Sanitizer seems to be broken due to some OIIO changes, and the MacOS picked up too new LLVM (23.1.0).

GGXDist(roughness_index, 0),
DielectricFresnel::from_table_index(fresnel_index, false), cosNO,
roughness_index, true)
roughness_index, true, 1)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Don't we need to pass lambda_0 here too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As far as I understand, these two only exist for LUT generation, use scalar DielectricFresnel where all 4 lanes have the same value and therefore the lamba_0 doesn't really matter. But I agree that the 1 isn't the most self-explanatory here.

We could push this whole thing into the Fresnel calculation, so the BSDF wouldn't need to know about the lambda, only the colored SchlickFresnel would, but that would move the responsibility of calculating the probabilities from the BSDF (where it lives now) to Fresnel, which would get a reflection_probability interface. I tried to make the change as minimal as possible, but it is definitely a tradeoff to consider.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I remember now. As a sanity check, can you check if the generated luts changed? Would it be better to use 0?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@aconty so with lambda_0 == 1, the results are bit identical. I have just tried with = 0 and it is actually worse.

Before the proposed change, we just used F.max(), which picked on of the identical lanes. With lambda_0 = 1, we sum all 4 lanes and then divide by 4, and get the identical number. With lambda_0 = 0, we would sum up only first 3 and divide by 3 and, by the magic of powers of 2 being easier to optimize, we get a different result.

Given the standard IEEE754 approach of having Guard, Round, and Sticky bits in the FPUs, you pretty much have 2 bits of extra precision in the arithmetic, so it makes perfect sense that exactly 4x bigger/smaller makes no difference (you don't even hit the Sticky bit).

GGXDist(roughness_index, 0),
DielectricFresnel::from_table_index(fresnel_index, true), cosNO,
roughness_index, true)
roughness_index, true, 1)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and here?

@aconty

aconty commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

First of all, thanks and good catch!

Code LGTM, I left a couple of questions. I don't remember all the details so you may not have to fix anything.

BTW, I just happen to be, at the moment, setting up a separate repo for BSDL with its own testsuite. It is a private repo for now since I didn't know more people were active here. We may have to speed it up so we don't diverge.

@tdavidovicNV

Copy link
Copy Markdown
Contributor Author

@aconty thanks. I am not really doing any development on this, I merely caught it (and the other 2) when reviewing the 1.39.6 MaterialX PRs. So no large changes planned. But it does speak to keeping this more local, rather than pushing it into the Fresnel.

We can also leave this open until you publish the BSDL repo, and then cross-reference it from there and close this one. Depends on your timeline, I guess.

@aconty

aconty commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

We can merge and I'll port it somehow leter. Didn't you get noise changes in the testsuite?

@tdavidovicNV

Copy link
Copy Markdown
Contributor Author

@aconty there are changes in both render-mx-generalized-schlick and render-mx-generalized-schlick-glass, but they are tiny and within thresholds. We might want to update the references to avoid the drift, but I do not know what the policy on that is.

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