mmp / pbrt-v4

Source code to pbrt, the ray tracer described in the forthcoming 4th edition of the "Physically Based Rendering: From Theory to Implementation" book.
https://pbrt.org
Apache License 2.0
2.75k stars 407 forks source link

Possible wrong use of non-symmetric BRDF with BDPT #395

Closed rainbow-app closed 8 months ago

rainbow-app commented 8 months ago

Hello.

I'm using v3, but v4 looks affected too.

I do not have images that show a problem. I'm sharing my finding that looks like a bug.


According to Veach PhD, f() should always be computed in the direction of the travel of light. See indexes in arguments of fs in c{s,t} below (10.8).

When you build camera subpath, the "o" in si.wo means what it's for: outgoing light. When you build light subpath, the meaning is reversed: si.wo is the direction of incoming light. When you later connect the subpaths, you're not taking this distinction into account.

Same with wi. Consider t=1 case, light subpath connects to camera.

qs.f(sampled, TransportMode::Importance) here sampled is the camera.

    Spectrum f(const Vertex &next, ...
        Vector3f wi = next.p() - p();
...
            return si.bsdf->f(si.wo, wi) *...
...

wi points from surface (qs) to camera (sampled), and that's direction of outgoing light.

In general (I didn't read how non-symmetric BRDFs are implemented), the first of the two arguments in f() is like you write here -- outgoing light. So you're actually swapping them.

Same in one (of the two) f() call in general connect branch.

PS.

For the same reason, the light subpath itself is also wrong (whenever non-symmetric brdf is evaluated) -- when calling isect.bsdf->Sample_f(wo, &wi,... in RandomWalk().

I am missing a lot here (am not, and never was interested in bump mapping or refraction). Sorry if it's all intentional, and not a bug.

PS2.

My apologies. The key difference between v3 and v4 is that you pass mode as a third argument to f(): v3: return si.bsdf->f(si.wo, wi) v4: return bsdf.f(si.wo, wi, mode); This mode opens lots of possibilities that I didn't notice two days ago. May be somewhere there's a bug in swapping directions, but without specific parts/details in v4 code that look like that, this issue belongs strictly to v3. And since this is v4, I'm closing the issue here. My apologies for unnecessarily taking your attention.