Book 3.12.6: Incorrect solution to Handling Surface Acne

[TheRayTracer]

I have been working through this series for a number of years as a hobbyist. Book 3 has equipped me with the knowledge to upgrade my Whitted-style ray tracer to a path ray tracer. While I am still working through Book 3 I wanted to say a big Thank you!

Onto the topic, I would like to discuss the "Surface Acne" that is produced after introducing the concept of PDFs. Aka, those pesky and ugly black pixels.

After introducing the concept of PDFs in Book 3, my implementation also exhibited NaN pixels as early as chapter 6. I tried the book’s reference implementation and read the accompanying explanation in chapter 12.6 to resolve the issue, but the explanation does not make sense and the proposed solution does not work as intended.

Within the write_colorfunction we have:

https://github.com/RayTracing/raytracing.github.io/blob/ec8b43e81dedc019f751fe37ed020cf97fe3f178/src/common/color.h#L24-L27

However, I believe that it is too late in the process to fix these pixels at the output stage. What the above says is:

If Pixel is Bad then
     Set Pixel = Black

Thus, these bad pixels are still being outputted as black.

What I think the intention of the code and explanation is actually:

Loop over camera height and width pixels:
      pixel_color = empty;
      Loop over samples:
             color_contribution = ray_color(ray, depth);
             If color_contribution is Bad then
                  Set color_contribution = Black;
             pixel_color += color_contribution;

This way, one bad sample does not ruin all the samples and the final pixel.

Thus, I would remove the lines

https://github.com/RayTracing/raytracing.github.io/blob/ec8b43e81dedc019f751fe37ed020cf97fe3f178/src/common/color.h#L24-L27

from the write color_write function, and check for bad/NaN samples in the main rendering loop as outlined above. This would also limit the scope of the fix to Book 3 (where it is introduced).

I could end the issue here as I believe that Chapter 12.6 serves as a learning exercise – in both graphics and floating-point math. However, I don’t want to accept a known bug in my implementation especially when I believe the source of the bug is an easy fix.

We seem to get NaN samples when we divide calculating color contribution sample by zero. Specifically here

https://github.com/RayTracing/raytracing.github.io/blob/ec8b43e81dedc019f751fe37ed020cf97fe3f178/src/TheRestOfYourLife/pdf.h#L33-L36

, where the PDF can be zero. So let’s not allow it to be zero. 😃 Let’s make it as close to zero as possible. We could simply do the following:

  return fmax(DBL_EPSILON, cosine / PI);

Note, that this could also be applied more generally higher up in the ray_color member function too.

I have attached a few rendering plates, although I am unsure if they actually help since they are only showing the known bug and a clean render. But perhaps the root cause of the bug can be left as a exercise for the reader to investigate. 😃

[hollasch]

Note that DBL_EPSILON is not "as close to zero as possible". In fact, something like 47% of all possible floats are closer to zero than DBL_EPSILON (reason 27 why Steve hates epsilons). Nevertheless, a promising location and general tactic.

[TheRayTracer]

Somewhat related: https://github.com/RayTracing/raytracing.github.io/issues/772

[trevordblack]

I think the "right" solution here is to terminate the ray if pdf = 0:

        auto pdf_val = p.value(scattered.direction());

        if (pdf_val == 0) {
            // Scattering is impossible
            return color_from_emission;                         
        }

        double scattering_pdf = rec.mat->scattering_pdf(r, rec, scattered);

        color color_from_scatter =
            (srec.attenuation * scattering_pdf * ray_color(scattered, depth-1)) / pdf_val;

        return color_from_emission + color_from_scatter;            

[trevordblack]

@TheRayTracer What do you think of this?

[TheRayTracer]

Happy with terminating the ray if pdf = 0 in the main rendering loop. This should prevent the NaN's.

However, this addresses the second part of the issue.

The main issue was described in the first part of the issue. That is, detecting a bad pixel still outputs a black pixel.

My bad for not splitting the issue into two.

[trevordblack]

We can definitely move the NaN check from out of the write out to the main render loop, but I'm not sure if there are other sources of NaNs in the code.

Hmm.

I hate having to write the code, then point out the NaNs, then saying we need to rewrite it, then rewriting to remove NaNs. Then show that the NaNs are gone.

Let me think what to do about NaN code in main render loop (versus just removing that chunk of the book altogether)

[TheRayTracer]

"...just removing that chunk of the book altogether"

That would be the ideal option.