Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump mapping is broken for BDPT and lightpath #385

Open
Vutshi opened this issue Sep 26, 2023 · 3 comments
Open

Bump mapping is broken for BDPT and lightpath #385

Vutshi opened this issue Sep 26, 2023 · 3 comments

Comments

@Vutshi
Copy link

Vutshi commented Sep 26, 2023

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

Lightpath:
cornell_lightpath_v4

BDPT:
cornell_bdpt_v4

Path:
cornell_path_v4

Ground truth (pbrt-v3 BDPT):
cornell_bdpt_v3

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

@mmp
Copy link
Owner

mmp commented Oct 18, 2023

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:

PBRT_DBG("%s\n",
.

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
Copy link
Author

Vutshi commented Oct 19, 2023

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 Vutshi changed the title Displacement is broken for BDPT and lightpath Bump mapping is broken for BDPT and lightpath Oct 19, 2023
@Vutshi
Copy link
Author

Vutshi commented Oct 19, 2023

@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
cornell_bdpt_new

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

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());

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants