Fix energy loss in multi-scattering term#23203
Conversation
alice-i-cecile
added
A-Rendering
alice-i-cecile
added
D-Straightforward
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
alice-i-cecile
added
the
M-Release-Note
|
It looks like your PR has been selected for a highlight in the next release blog post, but you didn't provide a release note. Please review the instructions for writing release notes, then expand or revise the content in the release notes directory to showcase your changes. |
|
Successfully passing the white furnace test is an important milestone, and a neat thing to teach people about. Can you add a release note for this PR + #23194 as part of this PR? If you're comfortable explaining the background of the test, why it's significant and what we were doing wrong, please do so, but just a stub is fine :) |
alice-i-cecile
added
the
M-Deliberate-Rendering-Change
JMS55
left a comment
JMS55
left a comment
There was a problem hiding this comment.
Awesome PR!
About passing the white furnace test; that's only true for the EnvMapLight code. Directional lights, point lights, spot lights, irradiance volumes, and probably other things I'm forgetting all go through their own paths, which are not tested in the current example.
|
I'm especially suspicious about several places where we directly add specular and diffuse contributions together, rather than mixing them based on the fresnel factor. I attempted to solve this in Solari by switching to OpenPBR's BRDF spec, but it didn't seem to make much of a difference, and I ultimately decided to prioritize other work. @dylansechet if you're interested in discussing this (or Solari's other BRDF issues), please reach out on discord in #rendering-dev! |
|
merging main in so that it triggers the white furnace in CI check |
mate-h
left a comment
mate-h
left a comment
There was a problem hiding this comment.
Amazing work! This is all mathematically sound and I don't really have any blocking comments, just some general feedback on the readability. Ship it!
|
|
||
| After fixing those, Bevy passes the test. That means your materials will behave more correctly under image-based lighting. | ||
|
|
||
| A gray image has never been so exciting! No newline at end of file |
alice-i-cecile
added
S-Ready-For-Final-Review
|
I'm going to hold off a bit so you can review the non-blocking feedback @dylansechet. Let me know if you'd like me to just merge it though! |
| authors: ["@dylansechet"] | ||
| pull_requests: [23194, 23203] | ||
| --- | ||
| The [white furnace test](https://lousodrome.net/blog/light/2023/10/21/the-white-furnace-test/) is a classic sanity check for physically-based renderers. Place a perfectly reflective object inside a uniform white environment, and it should be indistinguishable from the background, no matter how metallic and rough. Any object that remains visible is a sign that the shader is creating or absorbing energy it shouldn't. |
There was a problem hiding this comment.
Doesn't have to be perfectly reflective - all BRDFs should obey this.
Otherwise, great writing!
There was a problem hiding this comment.
Thanks!
I meant "perfectly reflective" in the sense that we need albedo = 1 for the object to disappear into. But I agree, it's a clumsy choice of words and should be changed in the final veersion.
# Objective This PR is split off from bevyengine#23818. It depends on bevyengine#24313, which is integrated as the first commit here. There's an issue in the solari BRDF which causes partially metallic objects to lose energy in the white furnace test. ## Solution The current BRDF computes a blended F0 between metallic and dielectric materials, and then uses it it for calculations. It should be evaluating both the metallic and dielectric BRDFs separately and only blend them as a last step. A similar issue used to be present for IBL and was fixed in the same way, see bevyengine#23203. ## Showcase ### White furnace test Main: <img width="1280" height="720" alt="layering" src="https://github.com/user-attachments/assets/5c7abaac-93ac-4a35-be76-e81681094232" /> This PR: <img width="1280" height="720" alt="mixing" src="https://github.com/user-attachments/assets/d0c32bab-c880-4222-a255-1f9a5c2e3f6a" />
5 participants
Objective
With #23194 applied, the white furnace test passes for pure metals and dielectrics, but fails for anything in between.
Solution
The issue seems to be the multi-scattering term used in
environment_map_light.The current code computes
FmsEms(mix(F0_dielectric, F0_metal, metalness))when it should be computingmix(FmsEms(F0_dielectric), FmsEms(F0_metal), metalness), which causes an issue as FmsEms is non-linear in F0.The bug is also present in the blogpost that was used as inspiration for the implementation, where the author mentions that the results with multi-scattering are darker than they should be.
Testing
cargo run --example testbed_white_furnace. I've never been so happy to see a gray image :)cargo run --example pbr.Showcase
White furnace test (with #23194 also applied):
Before:
After:
PBR test also on imgsli
Before:
After:
