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

No PDF cutoff for light samples with a zero BxDF probability #329

Open
BradleyMarie opened this issue Jul 23, 2023 · 2 comments
Open

No PDF cutoff for light samples with a zero BxDF probability #329

BradleyMarie opened this issue Jul 23, 2023 · 2 comments

Comments

@BradleyMarie
Copy link

BradleyMarie commented Jul 23, 2023

Hi Matt,

First off, huge thank you for providing pbrt textbook, source code, and the scene library. They have been invaluable resources and points of comparison for my own renderer.

While working on my own renderer and debugging a small visual diff I had against pbrt, I discovered that the diff was caused by differences in how light samples with a zero bxdf probability are treated. On my renderer, I discard them on the principal that a zero probability sample should not contribute to the scene while pbrt-v3 seems to include them. I assume this behavior is intentional; however, I couldn't find any details about it in the book.

Normally, the visual impact of the difference in behavior is very small unless you have a scene that is intentionally set up to make them obvious.

I've done that with these renders from the ecosys scene (drop-in pbrt file). The one that looks visually correct is the one from an unmodified copy of pbrt while the one with the artifacting is from a copy of pdf where I modified EstimateDirect to cut off light samples where scatteringPdf is zero. It feels intuitively wrong to say that the render that looks visually incorrect is actually the "correct" render, so I think I'll just instead say that my expectation was that these two renders should be identical and they are not.

From unmodified copy of pbrt
With cutoff for paths with zero BxDF PDF

I believe this behavior would also exist in pbrt-v4 based on my read of PathIntegrator::SampleLd but I have not verified it explicitly.

Also, just for completeness, the zero pdf paths themselves are caused by the interpolated vertex normals. The issue goes away if the surface normals are used instead.

@mmp
Copy link
Owner

mmp commented Jul 25, 2023

Hi, Brad--

I'm glad the book has been useful to you, and nice debugging there digging into this!

I think what is going on is related to pbrt's handling of shading normals (discussed here.) So a Lambertian BRDF for example always returns R/pi regardless of whether w_i and w_o are in the same hemisphere and reflection/transmission is selected in BSDF::f based on the geometric normal.

However, BSDF:Pdf doesn't do that and just evaluates the PDFs, which do return 0 (for example) for a Lambertian BRDF with w_i and w_o in different hemispheres.

So, I think those 0 PDF values are arising due to that. I think there may be an argument that it's all correct as written (but am not sure about that!). There is no chance of sampling those directions from BSDF sampling, yet the BSDF has a non-zero value. The light sample is still included, the MIS weight ends up being 1, and everything is ok 🤷 . Though I don't remember that as an intentional decision; my assumption looking at that code now (and presumably the assumption when it was written) was that f.IsBlack()==true <---> scatteringPdf==0, other than in rare floating-point edge cases.)

@BradleyMarie
Copy link
Author

That explanation lines up with my own understanding of the behavior.

With the non-geometric nature of shading normals, the definition of correctness here is a bit pliable. In the limit case with an infinite number of samples for surfaces like this one the code as written will end up with a value somewhere between the value we’d get from just sampling the BRDF and just sampling the lights. If this in–between value has a useful interpretation, then I think it’s arguable that the value could be considered correct; however, my current feeling is that if a path returns light when it’s sampled by the light source then that same path should be samplable and light-yielding by the BxDF.

Another reason to consider treating the current behavior as incorrect is because the same issue repeats itself with indirect light paths. In this render I modified the Path integrator to ignore direct lighting on the first bounce, but to otherwise render the scene normally. Some of the same areas of darkness show up; however, in this case it feels more bug-like to me since if these areas also lack direct lighting then it means the final image can end up with dark spots in places where there geometrically should have been indirect lighting.

ecosys

I also glanced over the pbrt-v4 code and it looks like this particular issue involving indirect light no longer exists since BSDF::Sample_f uses the pdf returned from BxDF::Sample_f. That said, I think there may be a different issue for diffuse reflective paths where the pbrt-v4 code can instead sample and return light from transmissive paths since it does not seem to contain the pbrt-v3 logic which re-evaluates if a path is reflective or transmissive after it is sampled.

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