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

Bug: Image corruption if lambertian scattered ray direction is (0,0,0). #619

Closed
Teledhil opened this issue May 24, 2020 · 2 comments
Closed
Assignees
Milestone

Comments

@Teledhil
Copy link

There is a small chance on lambertian materials that the scattered ray has direction (0,0,0):

        virtual bool scatter(
            const ray& r_in, const hit_record& rec, color& attenuation, ray& scattered
        ) const {
            vec3 scatter_direction = rec.normal + random_unit_vector();
            scattered = ray(rec.p, scatter_direction);
            attenuation = albedo;
            return true;
        }

In line:

vec3 scatter_direction = rec.normal + random_unit_vector();

If the random_unit_vector() is the opposite of the normal.

Later, on the ray_color() function, the scattered ray will be the ray r of the next recursive call:

color ray_color(const ray& r, const hittable& world, int depth) {
    hit_record rec;

    // If we've exceeded the ray bounce limit, no more light is gathered.
    if (depth <= 0)
        return color(0,0,0);

    if (world.hit(r, 0.001, infinity, rec)) {
        ray scattered;
        color attenuation;
        if (rec.mat_ptr->scatter(r, rec, attenuation, scattered))
            return attenuation * ray_color(scattered, world, depth-1);
        return color(0,0,0);
    }

    vec3 unit_direction = unit_vector(r.direction());
    auto t = 0.5*(unit_direction.y() + 1.0);
    return (1.0-t)*color(1.0, 1.0, 1.0) + t*color(0.5, 0.7, 1.0);
}

On the next recursive call, if there is no hit, on this line, unit_direction() will become (-nan, -nan, -nan) since unit_vector() divides by length zero:

vec3 unit_direction = unit_vector(r.direction());

And this will cause ray_color() to return a -nan color.

@Teledhil Teledhil changed the title Bug: Image corruption if scattered ray direction is (0,0,0) and hits the background. Bug: Image corruption if lambertian scattered ray direction is (0,0,0). May 24, 2020
@hollasch
Copy link
Collaborator

hollasch commented May 24, 2020

There would be multiple places to address this (and we might want to fix more than one).

  • What does unit_vector() return for zero vectors? Should it return some unit vector, like (1,0,0)? We should probably shut down code that could produce NaN values when we find it.
  • A scatter function should never set the scattered ray direction to (0,0,0). In such a case, I suppose it should return false and leave the scatter ray unset, or set it to the normal vector and return true.
  • The scatter() method in book 3 is slightly different. It should be addressed here as well.
  • As a side note, instead of computing the unit vector, it should just use r.direction().y() / r.direction().length(). This is a small optimization, but is still susceptible to the 0.0/0.0 problem.

@hollasch hollasch added this to the v3.3.0 milestone Jun 4, 2020
@trevordblack trevordblack modified the milestones: v3.3.0, v3.2.2 Oct 5, 2020
@hollasch hollasch self-assigned this Oct 12, 2020
hollasch added a commit that referenced this issue Oct 13, 2020
In cases where the random unit vector is exactly or very closely equal
to the reversed normal vector, the scatter direction vector will be zero
or close to it. This can lead to infinite or NaN values after the call.

Resolves #619
@hollasch
Copy link
Collaborator

I've decided for now not to add a test in ray_color() to catch inbound rays with near-zero direction. We can do that later if it turns out that the fix for lambertian::scatter() is insufficient or other cases warrant it.

Also, I don't think it makes sense to slow down unit_vector() by adding a non-zero test.

Finally, the optimization above is short-lived as the project develops, and not really worth the additional complexity / explanation.

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

3 participants