Cycles: Remove MultiGGX code, replace with albedo scaling #107958

Lukas Stockner · 2023-05-16T02:16:44+02:00

Lukas Stockner commented

2023-05-16 02:16:44 +02:00

While the multiscattering GGX code is cool and solves the darkening problem at higher roughnesses, it's also currently buggy, hard to maintain and often impractical to use due to the higher noise and render time.

In practice, though, having the exact correct directional distribution is not that important as long as the overall albedo is correct and we a) don't get the darkening effect and b) do get the saturation effect at higher roughnesses.

This can simply be achieved by adding a second lobe or scaling the single-scattering GGX lobe. Both approaches require the same precomputation and produce outputs of comparable quality, so I went for the simple albedo scaling since it's easier to implement and more efficient.

Overall, the results are pretty good: All scenarios that I tested (Glossy BSDF, Glass BSDF, Principled BSDF with metallic or transmissive = 1) pass the white furnace test (a material with pure-white color in front of a pure-white background should be indistinguishable from the background if it preserves energy), and the overall albedo for non-white materials matches that produced by the real multi-scattering code (with the expected saturation increase as the roughness increases).

In order to produce the precomputed tables, the PR also includes a utility that computes them. This is not built by default, since there's no reason for a user to run it (it only makes sense for documentation/reproducibility purposes and when making changes to the microfacet models).

The code is generally ready for review, there's just one TODO: Currently, the weight increase due to the albedo scaling affects the Glossy/Transmission Color passes. It shouldn't.

This also brings up a UX issue that we should consider for Principled v2: The saturation increase at high roughness is physically correct, but do we actually want it? From a user perspective, it generally would probably be better if the albedo always matched the Base Color input, that direct artist control with no interdependency between inputs is the entire point of the "principled" models after all. Maybe we should apply it for the Glossy and Glass BSDF nodes, but not for Principled BSDF.

While the multiscattering GGX code is cool and solves the darkening problem at higher roughnesses, it's also currently buggy, hard to maintain and often impractical to use due to the higher noise and render time. In practice, though, having the exact correct directional distribution is not that important as long as the overall albedo is correct and we a) don't get the darkening effect and b) do get the saturation effect at higher roughnesses. This can simply be achieved by [adding a second lobe](https://blog.selfshadow.com/publications/s2017-shading-course/imageworks/s2017_pbs_imageworks_slides_v2.pdf) or [scaling the single-scattering GGX lobe](https://blog.selfshadow.com/publications/turquin/ms_comp_final.pdf). Both approaches require the same precomputation and produce outputs of comparable quality, so I went for the simple albedo scaling since it's easier to implement and more efficient. Overall, the results are pretty good: All scenarios that I tested (Glossy BSDF, Glass BSDF, Principled BSDF with metallic or transmissive = 1) pass the white furnace test (a material with pure-white color in front of a pure-white background should be indistinguishable from the background if it preserves energy), and the overall albedo for non-white materials matches that produced by the real multi-scattering code (with the expected saturation increase as the roughness increases). In order to produce the precomputed tables, the PR also includes a utility that computes them. This is not built by default, since there's no reason for a user to run it (it only makes sense for documentation/reproducibility purposes and when making changes to the microfacet models). The code is generally ready for review, there's just one TODO: Currently, the weight increase due to the albedo scaling affects the Glossy/Transmission Color passes. It shouldn't. This also brings up a UX issue that we should consider for Principled v2: The saturation increase at high roughness is physically correct, but do we actually want it? From a user perspective, it generally would probably be better if the albedo always matched the Base Color input, that direct artist control with no interdependency between inputs is the entire point of the "principled" models after all. Maybe we should apply it for the Glossy and Glass BSDF nodes, but not for Principled BSDF.

👀 2 ❤️ 4

Lukas Stockner added this to the 4.0 milestone 2023-05-16 02:16:44 +02:00

Lukas Stockner added the

Module

Render & Cycles

label 2023-05-16 02:16:44 +02:00

Brecht Van Lommel was assigned by Lukas Stockner

2023-05-16 02:16:45 +02:00

Weizhen Huang was assigned by Lukas Stockner

2023-05-16 02:16:45 +02:00

Lukas Stockner added this to the Render & Cycles project 2023-05-16 02:17:09 +02:00

Brecht Van Lommel was unassigned by Lukas Stockner

2023-05-16 02:17:20 +02:00

Weizhen Huang was unassigned by Lukas Stockner

2023-05-16 02:17:20 +02:00

Lukas Stockner requested review from Brecht Van Lommel 2023-05-16 02:17:28 +02:00

Lukas Stockner requested review from Weizhen Huang 2023-05-16 02:17:28 +02:00

Weizhen Huang commented

2023-05-16 11:38:27 +02:00

Seems that you didn't include some changes in osl? I couldn't compile.

Lukas Stockner commented

2023-05-16 12:56:12 +02:00

Seems that you didn't include some changes in osl? I couldn't compile.

Ah yes, I was building without OSL, that still need to be updated. Will do, thanks.

> Seems that you didn't include some changes in osl? I couldn't compile. Ah yes, I was building without OSL, that still need to be updated. Will do, thanks.

Lukas Stockner force-pushed remove-multiggx from ae171addb5 to 8fa1654ae9

2023-05-19 04:33:11 +02:00

Compare

Lukas Stockner commented

2023-05-19 04:34:55 +02:00

Updated to include OSL support.

I also ended up kicking out most of the Cycles-specific OSL microfacet closures, we only need three special ones now (and one can be removed along with Principled v1 later).

If we really want to keep those for user's OSL scripts, we could implement them as wrappers around the standard functions in the header, but I don't think that's needed.

Updated to include OSL support. I also ended up kicking out most of the Cycles-specific OSL microfacet closures, we only need three special ones now (and one can be removed along with Principled v1 later). If we really want to keep those for user's OSL scripts, we could implement them as wrappers around the standard functions in the header, but I don't think that's needed.

Brecht Van Lommel commented

2023-05-19 16:02:14 +02:00

We can break OSL closure compatibility in 4.0.

Lukas Stockner force-pushed remove-multiggx from 8fa1654ae9 to d21029b029

2023-05-22 03:05:35 +02:00

Compare

Lukas Stockner commented

2023-05-22 03:08:07 +02:00

Updated to fix the color passes. They now even include the high-roughness saturation increase, so that with a white background the "glossy direct" channel appears white regardless of the roughness and object's color.

The match is not perfect at the edges at high roughness, but that was already the same before, and I don't see a way of improving it without increasing the size of ShaderClosures.

Updated to fix the color passes. They now even include the high-roughness saturation increase, so that with a white background the "glossy direct" channel appears white regardless of the roughness and object's color. The match is not perfect at the edges at high roughness, but that was already the same before, and I don't see a way of improving it without increasing the size of `ShaderClosure`s.

Brecht Van Lommel approved these changes 2023-05-23 14:15:19 +02:00

Weizhen Huang commented

2023-05-24 18:06:18 +02:00

Now that the new Multi GGX is much faster, wouldn't it be good to remove the "Slower than GGX" from the description? I guess it's still slightly slower but doesn't seem very obvious.
Also maybe mentioning it's energy-preserving, I'm thinking of something like "Gives energy-preserving results by compensating for multiple scattering, preventing excessively darkening especially at high roughness values."

Now that the new Multi GGX is much faster, wouldn't it be good to remove the "Slower than GGX" from the description? I guess it's still slightly slower but doesn't seem very obvious. Also maybe mentioning it's energy-preserving, I'm thinking of something like "Gives energy-preserving results by compensating for multiple scattering, preventing excessively darkening especially at high roughness values."

Weizhen Huang approved these changes 2023-05-24 18:07:02 +02:00

Lukas Stockner commented

2023-05-30 01:01:23 +02:00

Good point about the description, thanks! I'll need to update that.

In general, this will need an extensive entry in the release notes, so I'll wait with merging until Thursday when I'm back home so I can update those properly. Also, the tests need to be updated, and I'd like to add a few more tests.

For now, I've rebased the branch since it turned out that part of the color pass fix here is also the fix for #108211, so I already merged that bit.

Good point about the description, thanks! I'll need to update that. In general, this will need an extensive entry in the release notes, so I'll wait with merging until Thursday when I'm back home so I can update those properly. Also, the tests need to be updated, and I'd like to add a few more tests. For now, I've rebased the branch since it turned out that part of the color pass fix here is also the fix for #108211, so I already merged that bit.

Lukas Stockner force-pushed remove-multiggx from d21029b029 to 270a4c9bdb

2023-05-30 01:02:06 +02:00

Compare

Lukas Stockner force-pushed remove-multiggx from 270a4c9bdb to 9b4058c3b3

2023-06-05 02:19:05 +02:00

Compare

Lukas Stockner referenced this issue from a commit

2023-06-05 02:21:03 +02:00

Cycles: Remove MultiGGX code, replace with albedo scaling

Lukas Stockner merged commit 888bdc1419 into main

2023-06-05 02:21:03 +02:00

Lukas Stockner deleted branch remove-multiggx