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

Fix problems with material::scatter() methods #728

Closed
wants to merge 7 commits into from

Conversation

hollasch
Copy link
Collaborator

  • In book 1, the code for material::dielectric::scatter() divereged from
    the source code.

  • In book 2, we neglected to include the code changes to handle ray time
    for material::metal and material::dielectric.

Resolves #133

- In book 1, the code for material::dielectric::scatter() divereged from
  the source code.

- In book 2, we neglected to include the code changes to handle ray time
  for material::metal and material::dielectric.

Resolves #133
@hollasch hollasch added this to the v3.2.1 milestone Sep 11, 2020
@hollasch hollasch requested a review from a team September 11, 2020 23:22
@hollasch hollasch self-assigned this Sep 11, 2020
@trevordblack
Copy link
Collaborator

The text in the book having

            virtual bool scatter(
                const ray& r_in, const hit_record& rec, color& attenuation, ray& scattered
            ) const override {
                attenuation = color(1.0, 1.0, 1.0);
                double etai_over_etat = rec.front_face ? (1.0 / ref_idx) : ref_idx;

                vec3 unit_direction = unit_vector(r_in.direction());
                double cos_theta = fmin(dot(-unit_direction, rec.normal), 1.0);
                double sin_theta = sqrt(1.0 - cos_theta*cos_theta);
                if (etai_over_etat * sin_theta > 1.0 ) {
                    vec3 reflected = reflect(unit_direction, rec.normal);
                    scattered = ray(rec.p, reflected);
                    return true;
                }
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ C++ highlight
                double reflect_prob = schlick(cos_theta, etai_over_etat);
                if (random_double() < reflect_prob)
                {
                    vec3 reflected = reflect(unit_direction, rec.normal);
                    scattered = ray(rec.p, reflected);
                    return true;
                }
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ C++
                vec3 refracted = refract(unit_direction, rec.normal, etai_over_etat);
                scattered = ray(rec.p, refracted);
                return true;
            }

the "redundant" if statement is to make the code as easy to read as possible. I agree that chunking the two if statements together is certainly "cleaner", I think it's harder to understand.

The src has the cleaner:

                if (  (etai_over_etat * sin_theta > 1.0)
                   || (random_double() < schlick(cos_theta, etai_over_etat))
                   ) {
                    vec3 reflected = reflect(unit_direction, rec.normal);
                    scattered = ray(rec.p, reflected);
                    return true;
                }

The dielectric chapter is already the densest chapter in the book and I wanted the source to be incredibly simple to understand. But, having redundant code makes transcription errors more likely, not less.
I don't like this change, but I tolerate it.

The r_in.time change is perfect.

@trevordblack
Copy link
Collaborator

I would happy with:

double reflect_prob = schlick(cos_theta, etai_over_etat);
if (  (etai_over_etat * sin_theta > 1.0)
       || (random_double() < reflect_prob) )
{

@hollasch
Copy link
Collaborator Author

hollasch commented Sep 27, 2020

@trevordblack -- can you take a look at this latest change? I think my hang-up with the earlier code was that I usually think of indices of refraction, but in our code we're really dealing with a baked-in ratio of indices of refraction. I've updated the naming in the refraction code to reflect that. What do you think?

If it looks good, then I'll go ahead and tie up all the remaining loose ends.

scattered = ray(rec.p, refracted);
return true;
}

public:
double ref_idx;
double refraction_ratio; // Ratio of the two materials' indices of refraction
Copy link
Collaborator

@trevordblack trevordblack Sep 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh no
This member variable is supposed to be the refractive index.

             double etai_over_etat = rec.front_face ? (1.0 / ref_idx) : ref_idx;

the variable etai_over_etat is the refractive ratio. so your change of variable name there is fine.
the 1.0 is because we assume that the ray is coming-from/going-to a region with a refractive index of 1.0 (a vacuum)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're not dealing with a baked in refractive ratio. At least, that's not what our text is saying. The member variable is supposed to be the refractive index of the media.

It just so happens to also be the reciprocal refractive ratio if the outside is a vacuum

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! Of course; now I get it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you change refraction_ratio back to ref_idx I'm otherwise perfectly happy with this commit

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, fixed in next change, I think.

@trevordblack
Copy link
Collaborator

Looks good to me, make sure changes to source make their way back into the inlined source in the text.

@hollasch
Copy link
Collaborator Author

Yup, thanks!

This also adds the changes to the scatter specular updates in book 3
that were omitted.
@hollasch
Copy link
Collaborator Author

Ok @trevordblack , this last change should do it. Also found updates to the PDF specular component for dielectric materials that were missed in book 3.

@hollasch
Copy link
Collaborator Author

All changes now squashed and fixed up in PR #739.

@hollasch hollasch closed this Sep 30, 2020
@hollasch hollasch deleted the ray-time-metal-dielectric branch September 30, 2020 19:23
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

Successfully merging this pull request may close these issues.

None yet

2 participants