griefith/test2
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

Cycles: Fix inconsistency in Ng handling between Microfacets and other closures

Browse Source 
In Cycles, the convention is that reflection vs. refraction are classified
based on the hemisphere defined by the *shading* normal (N).

In general, most closure code uses the shading normal for most operations,
as is expected since using the geometric normal (Ng) would break normal maps
and smooth shading.

However, there are two places that use Ng: On the one hand, BSDF sampling
functions generally reject reflections that fall below the Ng hemisphere, since
they'd intersect the geometry when tracing the bounce. This is required, and
we can't do much about it.
On the other hand, the Microfacet evaluation code also checked that the ray
is in the same hemisphere w.r.t. both shading and geometric normal.

Theoretically, this is the right thing to do, since sampling and evaluation code
are supposed to be consistent. However, doing so breaks smooth shading, since
now direct light evaluation near the terminator will sometimes be rejected.

This didn't cause problems in practice because of another inconsistency: While
the parameter of the eval functions was named Ng, the caller actually provided
N (unclear whether by mistake or as a hacky workaround to the terminator).
When this was fixed in 063a9e89, users quickly reported issues with the shadow
terminator, so it was reverted to the hacky inconsistency in 1c50dd8b.

So, let's clean this mess up properly. If we don't want to do the Ng hemisphere
check in _eval, then instead of passing in a misleading value that ends up
making it a no-op, just remove the check. After all, the other closures don't
perform it either.

This way, we avoid the mislabeled Ng, we get rid of the special case for
microfacets, and the shadow terminator continues to be fine.

Technically, we still have the _sample vs. _eval mismatch. However, this is just
unavoidable, and is irrelevant in practice: For a strongly directional light
that makes the shadow terminator noticeable, the MIS weights will be massively
in favor of eval, to the point that it doesn't really matter what sample does.

To support this argument: You can actually reproduce a broken shadow terminator
in pretty much every Cycles version going back to 2011 by just setting up a
small intense mesh emitter, turning off MIS on it to disable _eval, and then
rendering a diffuse smooth-shaded sphere with >100000 samples so that the
fireflies resolve into somewhat consistent lighting.
If nobody has complained about this affecting all closures for 11 years,
I guess it's fine.

Pull Request: https://projects.blender.org/blender/blender/pulls/138632
This commit is contained in:
Lukas Stockner
2025-05-18 17:20:32 +02:00
parent 111b0764c1
commit a6015e1411
6 changed files with 30 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
intern/cycles/kernel/closure/bsdf.h
View File

@@ -532,18 +532,15 @@ ccl_device_inline
case CLOSURE_BSDF_MICROFACET_GGX_ID:
case CLOSURE_BSDF_MICROFACET_GGX_REFRACTION_ID:
case CLOSURE_BSDF_MICROFACET_GGX_GLASS_ID:
/* For consistency with eval() this should be using sd->Ng, but that causes
* artifacts (see shadow_terminator_metal test). Needs deeper investigation
* for how to solve this. */
eval = bsdf_microfacet_ggx_eval(kg, sc, sd->N, sd->wi, wo, pdf);
eval = bsdf_microfacet_ggx_eval(kg, sc, sd->wi, wo, pdf);
break;
case CLOSURE_BSDF_MICROFACET_BECKMANN_ID:
case CLOSURE_BSDF_MICROFACET_BECKMANN_REFRACTION_ID:
case CLOSURE_BSDF_MICROFACET_BECKMANN_GLASS_ID:
eval = bsdf_microfacet_beckmann_eval(kg, sc, sd->N, sd->wi, wo, pdf);
eval = bsdf_microfacet_beckmann_eval(kg, sc, sd->wi, wo, pdf);
break;
case CLOSURE_BSDF_ASHIKHMIN_SHIRLEY_ID:
eval = bsdf_ashikhmin_shirley_eval(sc, sd->N, sd->wi, wo, pdf);
eval = bsdf_ashikhmin_shirley_eval(sc, sd->wi, wo, pdf);
break;
case CLOSURE_BSDF_ASHIKHMIN_VELVET_ID:
eval = bsdf_ashikhmin_velvet_eval(sc, sd->wi, wo, pdf);

15
intern/cycles/kernel/closure/bsdf_ashikhmin_shirley.h
View File

@@ -46,13 +46,11 @@ ccl_device_inline float bsdf_ashikhmin_shirley_roughness_to_exponent(const float
}
ccl_device_forceinline Spectrum bsdf_ashikhmin_shirley_eval(const ccl_private ShaderClosure *sc,
const float3 Ng,
const float3 wi,
const float3 wo,
ccl_private float *pdf)
{
const ccl_private MicrofacetBsdf *bsdf = (const ccl_private MicrofacetBsdf *)sc;
const float cosNgO = dot(Ng, wo);
const float3 N = bsdf->N;
float NdotI = dot(N, wi);
@@ -60,9 +58,7 @@ ccl_device_forceinline Spectrum bsdf_ashikhmin_shirley_eval(const ccl_private Sh
float out = 0.0f;
if ((cosNgO < 0.0f) || fmaxf(bsdf->alpha_x, bsdf->alpha_y) <= 1e-4f ||
!(NdotI > 0.0f && NdotO > 0.0f))
{
if (fmaxf(bsdf->alpha_x, bsdf->alpha_y) <= 1e-4f || (NdotI < 0.0f) || (NdotO < 0.0f)) {
*pdf = 0.0f;
return zero_spectrum();
}
@@ -208,6 +204,13 @@ ccl_device int bsdf_ashikhmin_shirley_sample(const ccl_private ShaderClosure *sc
/* reflect wi on H to get wo */
*wo = -wi + (2.0f * HdotI) * H;
/* Check hemisphere. */
if (dot(Ng, *wo) < 0.0f) {
*pdf = 0.0f;
*eval = zero_spectrum();
return LABEL_NONE;
}
if (fmaxf(bsdf->alpha_x, bsdf->alpha_y) <= 1e-4f) {
/* Some high number for MIS. */
*pdf = 1e6f;
@@ -216,7 +219,7 @@ ccl_device int bsdf_ashikhmin_shirley_sample(const ccl_private ShaderClosure *sc
}
else {
/* leave the rest to eval */
*eval = bsdf_ashikhmin_shirley_eval(sc, Ng, wi, *wo, pdf);
*eval = bsdf_ashikhmin_shirley_eval(sc, wi, *wo, pdf);
}
return label;

22
intern/cycles/kernel/closure/bsdf_microfacet.h
View File

@@ -564,7 +564,6 @@ ccl_device_forceinline int bsdf_microfacet_eval_flag(const ccl_private Microface
template<MicrofacetType m_type>
ccl_device Spectrum bsdf_microfacet_eval(KernelGlobals kg,
const ccl_private ShaderClosure *sc,
const float3 Ng,
const float3 wi,
const float3 wo,
ccl_private float *pdf)
@@ -578,7 +577,6 @@ ccl_device Spectrum bsdf_microfacet_eval(KernelGlobals kg,
const float3 N = bsdf->N;
const float cos_NI = dot(N, wi);
const float cos_NO = dot(N, wo);
const float cos_NgO = dot(Ng, wo);
const float alpha_x = bsdf->alpha_x;
const float alpha_y = bsdf->alpha_y;
@@ -588,11 +586,10 @@ ccl_device Spectrum bsdf_microfacet_eval(KernelGlobals kg,
/* Check whether the pair of directions is valid for evaluation:
* - Incoming direction has to be in the upper hemisphere (Cycles convention)
* - Specular cases can't be evaluated, only sampled.
* - The outgoing direction has to be the in the same hemisphere w.r.t. both normals.
* - Purely reflective closures can't have refraction.
* - Purely refractive closures can't have reflection.
*/
if ((cos_NI <= 0) || !bsdf_microfacet_eval_flag(bsdf) || ((cos_NgO < 0.0f) != is_transmission) ||
if ((cos_NI <= 0) || !bsdf_microfacet_eval_flag(bsdf) ||
(is_transmission && !has_transmission) || (!is_transmission && !has_reflection))
{
return zero_spectrum();
@@ -732,7 +729,16 @@ ccl_device int bsdf_microfacet_sample(KernelGlobals kg,
/* Compute actual reflected or refracted direction. */
*wo = do_refract ? refract_angle(wi, H, cos_HO, m_inv_eta) : 2.0f * cos_HI * H - wi;
if ((dot(Ng, *wo) < 0) != do_refract) {
/* Ensure that the sampled direction lies in the correct hemisphere.
* Note that the check against Ng is only performed in the sampling code, not the evaluation.
* This is technically inconsistent, but required in order to avoid shadow terminator artifacts
* on smooth geometry (which we'd get if we checked Ng in evaluation) while ensuring that
* sampling doesn't return supposed reflection rays going into the geometry and vice versa.
* The same is done for other closures as well. */
const float cos_NO = dot(N, *wo);
const float cos_NgO = dot(Ng, *wo);
if ((cos_NgO < 0) != do_refract || (cos_NO < 0) != do_refract) {
return LABEL_NONE;
}
@@ -976,13 +982,12 @@ ccl_device void bsdf_microfacet_blur(ccl_private ShaderClosure *sc, const float
ccl_device Spectrum bsdf_microfacet_ggx_eval(KernelGlobals kg,
const ccl_private ShaderClosure *sc,
const float3 Ng,
const float3 wi,
const float3 wo,
ccl_private float *pdf)
{
const ccl_private MicrofacetBsdf *bsdf = (const ccl_private MicrofacetBsdf *)sc;
return bsdf->energy_scale * bsdf_microfacet_eval<MicrofacetType::GGX>(kg, sc, Ng, wi, wo, pdf);
return bsdf->energy_scale * bsdf_microfacet_eval<MicrofacetType::GGX>(kg, sc, wi, wo, pdf);
}
ccl_device int bsdf_microfacet_ggx_sample(KernelGlobals kg,
@@ -1043,12 +1048,11 @@ ccl_device int bsdf_microfacet_beckmann_glass_setup(ccl_private MicrofacetBsdf *
ccl_device Spectrum bsdf_microfacet_beckmann_eval(KernelGlobals kg,
const ccl_private ShaderClosure *sc,
const float3 Ng,
const float3 wi,
const float3 wo,
ccl_private float *pdf)
{
return bsdf_microfacet_eval<MicrofacetType::BECKMANN>(kg, sc, Ng, wi, wo, pdf);
return bsdf_microfacet_eval<MicrofacetType::BECKMANN>(kg, sc, wi, wo, pdf);
}
ccl_device int bsdf_microfacet_beckmann_sample(KernelGlobals kg,

11
intern/cycles/kernel/closure/bsdf_sheen.h
View File

@@ -65,19 +65,10 @@ ccl_device Spectrum bsdf_sheen_eval(const ccl_private ShaderClosure *sc,
const float a = bsdf->transformA;
const float b = bsdf->transformB;
if (dot(N, wo) <= 0.0f) {
*pdf = 0.0f;
return zero_spectrum();
}
const float3 localO = to_local(wo, T, B, N);
if (localO.z <= 0.0f) {
*pdf = 0.0f;
return zero_spectrum();
}
const float lenSqr = sqr(a * localO.x + b * localO.z) + sqr(a * localO.y) + sqr(localO.z);
const float val = M_1_PI_F * localO.z * sqr(a / lenSqr);
const float val = M_1_PI_F * fmaxf(localO.z, 0.0f) * sqr(a / lenSqr);
*pdf = val;
return make_spectrum(val);

BIN
tests/files/render/displacement/cycles_renders/bump_glass.png (Stored with Git LFS)
View File

Binary file not shown.

BIN
tests/files/render/displacement/cycles_renders/bump_glossy.png (Stored with Git LFS)
View File

Binary file not shown.