Several inconsistencies in Cycles BSDF #111941

New Issue

Weizhen Huang · 2023-09-04T23:24:42+02:00

Weizhen Huang commented

2023-09-04 23:24:42 +02:00

Defensive sampling
in bsdf_microfacet_sample() it is checked against (path_flag & PATH_RAY_CAMERA), but bsdf_microfacet_eval() is always using the pdf of defensive sampling, except when fresnel is one. Also, it's worth verifying if defensive sampling is still practical. (bf82f9442c)
sd->Ng or sd->N
both bsdf_microfacet_sample() and bsdf_microfacet_eval() have an argument Ng, but we pass sd->Ng to the former and sd->N to the later, so checks like dot(Ng, wo) > 0 don't give the same result. (063a9e8964)
bsdf_is_transmission(sc, wo) checks using smoothed normal sc->N, but in BSDF sampling and evaluation (sd->type & PRIMITIVE_CURVE) ? sc->N : sd->Ng is used.
ClosureLabel
- In surface_shader_bsdf_sample_closure(), we accumulate the result of all the closures, but label is solely determined by the closure chosen by bsdf_sample(). Maybe this is fine but the logic seems weird.
- For CLOSURE_BSDF_HAIR_CHIANG_ID, the label is determined by the wo direction in bsdf_label(), but determined by the interaction number in bsdf_sample(). Maybe label should always be LABEL_REFLECT because hair closure handles internal absorption itself.
Anisotropy
Behaves differently for Glossy and Principled BSDF. Principled BSDF uses remapping from Disney and only allow positive anisotropy. Glossy BSDF does not use remapping, allow both negative and positive anisotropy, and treat the distribution as isotropic if anisotropy < 1e-4f. The last piece of logic is repeated in svm, osl and shader_nodes.cpp
eta and label required by path guiding
is inconsistent in BSDF sampling and evaluation.

1. Defensive sampling ~~in `bsdf_microfacet_sample()` it is checked against `(path_flag & PATH_RAY_CAMERA)`, but `bsdf_microfacet_eval()` is always using the pdf of defensive sampling, except when fresnel is one. Also, it's worth verifying if defensive sampling is still practical.~~ (bf82f9442c) 2. `sd->Ng` or `sd->N` ~~both `bsdf_microfacet_sample()` and `bsdf_microfacet_eval()` have an argument `Ng`, but we pass `sd->Ng` to the former and `sd->N` to the later, so checks like `dot(Ng, wo) > 0` don't give the same result.~~ (063a9e8964) `bsdf_is_transmission(sc, wo)` checks using smoothed normal `sc->N`, but in BSDF sampling and evaluation `(sd->type & PRIMITIVE_CURVE) ? sc->N : sd->Ng` is used. 3. `ClosureLabel` * In `surface_shader_bsdf_sample_closure()`, we accumulate the result of all the closures, but `label` is solely determined by the closure chosen by `bsdf_sample()`. Maybe this is fine but the logic seems weird. * For `CLOSURE_BSDF_HAIR_CHIANG_ID`, the label is determined by the `wo` direction in `bsdf_label()`, but determined by the interaction number in `bsdf_sample()`. Maybe `label` should always be `LABEL_REFLECT` because hair closure handles internal absorption itself. 4. Anisotropy Behaves differently for Glossy and Principled BSDF. Principled BSDF uses remapping from [Disney](https://www.disneyanimation.com/publications/physically-based-shading-at-disney/) and only allow positive anisotropy. Glossy BSDF does not use remapping, allow both negative and positive anisotropy, and treat the distribution as isotropic if `anisotropy < 1e-4f`. The last piece of logic is repeated in svm, osl and `shader_nodes.cpp` 5. `eta` and `label` required by path guiding is inconsistent in BSDF sampling and evaluation.

Weizhen Huang added the

labels 2023-09-04 23:24:42 +02:00

Weizhen Huang changed title from ~~Several inconsistencies in `bsdf_sample()` and `bsdf_eval()`~~ to Several inconsistencies in Cycles BSDF

2023-09-05 11:54:21 +02:00

Weizhen Huang self-assigned this 2023-09-08 19:18:34 +02:00

Sign in to join this conversation.