Bump mapping is broken for BDPT and lightpath

[Vutshi]

Bump mapping works differently for lightpath, BDPT, and path integrators. Tests are done with pbrt-v4 ffdc4982b34756363a2af2793e8d4fbf43698fee

Lightpath:

BDPT:

Path:

Ground truth (pbrt-v3 BDPT):

Scene is using water-raindrop.png (and Al_eta for v3) from barcelona-pavilion cornell_box.pbrt.txt cornell_box_v3.pbrt.txt

[mmp]

Slightly confusingly, this is actually bump mapping, not displacement mapping. (Which is currently only supported for plymesh shapes--this should be fixed...)

I am almost certain that the issue is due to pbrt-v4 not using the appropriate correction factor for shading normals with adjoint light transport. (See Eric Veach's Non-symmetric Scattering in Light Transport Algorithms paper for details.) In pbrt-v3, the correction is performed here: https://github.com/mmp/pbrt-v3/blob/13d871faae88233b327d04cda24022b8bb0093ee/src/integrators/bdpt.cpp#L206.

But it is not included in pbrt-v4--here is the corresponding part of the code: https://github.com/mmp/pbrt-v4/blob/6d9d64eac7ac8ba381aa0226531c9ec368991fb4/src/pbrt/cpu/integrators.cpp#L2109.

So why not? The problem is that applying that correction can lead to high variance in many cases. So if the shading normals aren't too far off the geometric normal, not applying the correction may be preferable. You might try re-adding that functionality in pbrt-v4 (and finding the right spot in the LightPathIntegrator and applying it there, and see if that makes things match up better.

[Vutshi]

Slightly confusingly, this is actually bump mapping, not displacement mapping.

I agree, the reason I called it displacement is that pbrt-v4 scene format uses “texture displacement” for the bump map. (Fixed the issue title)

Thank you for the suggestion, I will try it.

Best

[Vutshi]

@mmp

I tested the correction for non-symmetric scattering. It does change things slightly, but it does not address the main issue.

BDPT with correction factor

Difference between the new BDPT and old BDPT in pbrt-v4:

UPDATE: here is the diff of the source code

Click to expand

``` diff --git a/src/pbrt/cpu/integrators.cpp b/src/pbrt/cpu/integrators.cpp index a2f15f9..cacab2c 100644 --- a/src/pbrt/cpu/integrators.cpp +++ b/src/pbrt/cpu/integrators.cpp @@ -59,6 +59,21 @@ std::string RandomWalkIntegrator::ToString() const { return StringPrintf("[ RandomWalkIntegrator maxDepth: %d ]", maxDepth); } +// BDPT Utility Functions +Float CorrectShadingNormal(const SurfaceInteraction &isect, const Vector3f &wo, + const Vector3f &wi, TransportMode mode) { + if (mode == TransportMode::Importance) { + Float num = AbsDot(wo, isect.shading.n) * AbsDot(wi, isect.n); + Float denom = AbsDot(wo, isect.n) * AbsDot(wi, isect.shading.n); + // wi is occasionally perpendicular to isect.shading.n; this is + // fine, but we don't want to return an infinite or NaN value in + // that case. + if (denom == 0) return 0; + return num / denom; + } else + return 1; +} + // Integrator Method Definitions Integrator::~Integrator() {} @@ -2106,6 +2121,7 @@ int RandomWalk(const Integrator &integrator, SampledWavelengths &lambda, vertex.delta = true; pdfRev = pdfFwd = 0; } + beta *= CorrectShadingNormal(isect, wo, bs->wi, mode); PBRT_DBG("%s\n", StringPrintf("Random walk beta after shading normal correction %s", beta) .c_str()); ```