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

Book 3.10.1: Probability density functions could cause NaN's? #772

Open
Walther opened this issue Oct 21, 2020 · 4 comments
Open

Book 3.10.1: Probability density functions could cause NaN's? #772

Walther opened this issue Oct 21, 2020 · 4 comments

Comments

@Walther
Copy link

Walther commented Oct 21, 2020

Sorry - I'm not 100% sure if this should be an issue, feel free to discard if considered not applicable!

[The following refers to cosine_pdf::value()@hollasch]

Consider the following:

virtual double value(const vec3& direction) const override {
auto cosine = dot(unit_vector(direction), uvw.w());
return (cosine <= 0) ? 0 : cosine/pi;
}

and
auto pdf_val = p.value(scattered.direction());
return emitted
+ srec.attenuation * rec.mat_ptr->scattering_pdf(r, rec, scattered)
* ray_color(scattered, background, world, lights, depth-1)
/ pdf_val;

a cosine_pdf.value returning zero can result in a division by zero in the main loop, and cause a NaN.

When adding some debug prints to my own implementation, on a conditional of pdf_val == 0, I got plenty of those, and they were causing NaNs.

However, for some reason, adding the similar debug prints to the example scene in this repo, I was unable to reproduce it. Wonder if that is just due to some implementation bug on my part, random luck, or something else.


Should there be some additional logic somewhere? E.g.

  • make sure the pdf's don't return zeroes?
  • make sure that we don't divide by zero at the end of ray_color ?
    • return something else conditionally; what?
  • other, what?
@hollasch hollasch added this to the v4.0.0 milestone Oct 24, 2020
@hollasch hollasch self-assigned this Oct 29, 2020
@LeFou-k
Copy link

LeFou-k commented Dec 7, 2020

I also have the same problem. In order to solve this, I add a EPSILON (like 0.0001 or so), so the pdf would be pdf + EPSILON but the output was bad. The spp100 output was worse than the one that had not been done importance sampling. I wonder if there's some better method to solve this.

@FelixAntonelli
Copy link

I have been following this book series for a uni project and as such have had to do some research into PDFs, in most places I have seen the Cosine Density function described as 1/(2pi)(1+cos(x)) but the book uses cos(x) / pi. Having done some testing it seems that using 1/(2pi)(1+cos(x)) removes all black pixels from my renders and im wondering if this is what is causing the issue? I plotted both versions on Desmos, the version used in the book (red) dips below zero at the extremities whereas the other version (blue) does not. Here's a render I did with the Cosine PDF from the book gyazo and another one I did with the other version gyazo, the black pixels in the center of the image are almost completely gone.

Maths is not my strong suit so there's a distinct possibility that I am way off here but i'd like to be corrected if so!

@BenediktKrieger
Copy link

BenediktKrieger commented Jan 28, 2024

thank you so much! I was searching for the reason my code was producing NaNs when mixing pdfs. Its quite surprising that if I render only with the lamber/cosine-pdf or the light-pdf I get no NaNs. Only when mixing the potentially very small pdf-values I get NaNs :) Book 3 is no running with Importance sampling at a whopping 1400fps 1spp 1080p with the cornell-box 🥳

@hollasch hollasch removed their assignment Mar 24, 2024
@hollasch hollasch changed the title Probability density functions could cause NaN's? Book 3.10.1: Probability density functions could cause NaN's? Apr 19, 2024
@9mm
Copy link

9mm commented Jun 22, 2024

Did anyone sort this out? This is the best I got 💀 banging my head on this one

image

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

No branches or pull requests

7 participants