Improve layering in the solari BRDF#24243
Conversation
kfc35
added
A-Rendering
|
Btw would love to know the code you used for the white furnace test for solari - I lost mine. |
|
Thanks for the review! I think I've addressed everything, with the only logic change being the mirror pdf handling. |
| if material.roughness <= MIRROR_ROUGHNESS_THRESHOLD { | ||
| return EvaluateAndSampleBrdfResult( | ||
| wi, | ||
| rho.specular / specular_weight, |
There was a problem hiding this comment.
I don't think rho.specular / specular_weight is right here. You need to evaluate the BRDF.
There was a problem hiding this comment.
It should be the same thing, as the total reflected energy for a mirror is the same as the brdf evaluated for the reflected ray.
Doing a brdf evaluation does feel more readable though, and the lut at 0 roughness probably isn't that great.
JMS55
left a comment
JMS55
left a comment
There was a problem hiding this comment.
One last potential issues (not sure which it should be, my brain is a little fried).
Also, we have a couple usages of F_AB, which samples a texture. Can we hoist that out now?
| if material.roughness <= MIRROR_ROUGHNESS_THRESHOLD { | ||
| return EvaluateAndSampleBrdfResult( | ||
| wi, | ||
| evaluate_specular_brdf(wo, wi, world_normal, material) / specular_weight, |
There was a problem hiding this comment.
Are we sure that we should be using evaluate_specular_brdf and not evaluate_brdf here?
There was a problem hiding this comment.
Old code evaluated the full BRDF, but not sure if that was right.
There was a problem hiding this comment.
That's what I tested first, and the white furnace has excess energy if we try and evaluate the full brdf here.
There was a problem hiding this comment.
Weird. I don't understand why that is!
There was a problem hiding this comment.
Does it still break if this thresholding is removed?
JMS55
added
S-Ready-For-Final-Review
# Objective Improve the solari BRDF. Split off from bevyengine#23818. ## Solution The layering approach used in the current brdf to distribute energy between the specular and diffuse lobes doesn't work, for reasons I honestly don't quite understand. The suggested changes are roughly inspired by [OpenPBR's equation 42](https://academysoftwarefoundation.github.io/OpenPBR/#model/basesubstrate/glossy-diffuse). We're not using EON though, so this uses Filament's multiscattering correction instead of equation 39. This formulation has the downside of breaking reciprocity. As far as I can tell the path tracer is unidirectional, so this shouldn't cause issues. --- ## Showcase ### White furnace Main: <img width="1280" height="720" alt="main" src="https://github.com/user-attachments/assets/c5fe02c7-1e3e-41ee-99af-e73e0fc8ae5c" /> This PR: <img width="1280" height="720" alt="layering" src="https://github.com/user-attachments/assets/8d28097f-572e-4a9e-aa66-875730357856" /> --- ### PICA PICA pathtraced <img width="1280" height="720" alt="animated" src="https://github.com/user-attachments/assets/98716de5-1215-413a-a985-7d3fb0638dc0" /> Main: <img width="1280" height="720" alt="main" src="https://github.com/user-attachments/assets/32da9aa1-5235-4200-8c2a-33fd78b15b8b" /> This PR: <img width="1280" height="720" alt="pr" src="https://github.com/user-attachments/assets/a160eb3d-1c94-4d14-b769-0a981c7dc3dd" />
5 participants
Objective
Improve the solari BRDF. Split off from #23818.
Solution
The layering approach used in the current brdf to distribute energy between the specular and diffuse lobes doesn't work, for reasons I honestly don't quite understand.
The suggested changes are roughly inspired by OpenPBR's equation 42. We're not using EON though, so this uses Filament's multiscattering correction instead of equation 39.
This formulation has the downside of breaking reciprocity. As far as I can tell the path tracer is unidirectional, so this shouldn't cause issues.
Showcase
White furnace
Main:
This PR:
PICA PICA pathtraced
Main:
This PR:
