Fix OpenPBR thin-walled subsurface color scaling - #3066
LouisDeconinck wants to merge 3 commits into
Conversation
|
Thanks for this proposal, @LouisDeconinck! Can you add a comment on the GitHub Issue that you're tackling, so that we can assign it to you? Additionally, see the instructions at #3066 (comment) on how to resolve the CLA authorization warnings, so that we can begin reviewing this PR. |
|
Signed the CLA. |
|
Done — Ive commented on #3027 and the issue is now assigned to me. EasyCLA is also passing on this PR now. |
Signed-off-by: Louis Deconinck <louis.dck@gmail.com> Assisted-by: Codex
Signed-off-by: Louis Deconinck <louis.dck@gmail.com> Assisted-by: Codex
264fd51 to
47b8e60
Compare
|
Thanks for taking this on, @LouisDeconinck! This looks very promising, but I see one important issue to address before we merge, along with a suggestion on the unit test. Your diagnosis of the double application is correct, but removing the color from the external factors makes a latent problem in our original graph more visible. In our hardware shading library, Ideally I'd like this PR to address the clamping issue along with the double application, and I believe two changes would get us there:
On the unit test, the new case in Overall this looks like a great first contribution, and with the refinements above, I believe it should be ready to merge. Once it lands in MaterialX, we can propagate the same fix to the reference graph in the OpenPBR repository. |
The 0.5*(1-g) and 0.5*(1+g) weights sum to one, so the OpenPBR equations are exactly a mix of the transmission and reflection lobes. This keeps every weight within [0, 1] across the full anisotropy range, avoids the clamp in the hardware multiply implementations, applies subsurface_color once through the lobe color inputs, and reduces this section of the graph from ten nodes to six. Also updates the functional notation in the PBR spec and replaces the literal-value unit test with a render test material exercising thin-walled subsurface across all shading languages.
|
Thanks for the detailed review — pushed a rework along exactly those lines:
|
Fixes #3027.
The OpenPBR thin-walled subsurface lobes are unit-albedo lobes;
subsurface_colorshould be applied once as each lobe color. The nodegraph also used it in both anisotropy factors, which squared the color contribution. This changes those factors to unit color and adds a focused graph regression test.Validation:
git diff --checkpassedAI assistance: Codex assisted with implementation and test drafting; the patch was reviewed before submission.