Trying to understand hit function in Chapter 8 Volumes #22

KrisYu · 2019-08-21T14:21:43Z

This code snippest from Chapter 8 Volumes:

bool constant_medium::hit(const ray &r, float t_min, float t_max, hit_record& rec) const{
  // I don't know about this part, first we define db, it may be either true or false
  bool db = (drand48() < 0.00001);
  // then we define it false;
  db = false;
  // then why would be bother to write this : bool db = (drand48() < 0.00001);
  // why don't we just write : bool db = false; 
  ...

And if db = false, then why would bother to write those if (db)s later, thanks.

The text was updated successfully, but these errors were encountered:

hollasch · 2019-08-21T18:05:26Z

Man, that whole function needs some love. The formatting is totally mangled, and there are several if statements with the body on the same line, but with indented lines following, which makes it look like they're controlling statements they're not.

Existing Code

bool constant_medium::hit(const ray& r, float t_min, float t_max, hit_record& rec) const {
    bool db = (drand48() < 0.00001);
    db = false;
    hit_record rec1, rec2;
    if (boundary->hit(r, -FLT_MAX, FLT_MAX, rec1)) { 
        if (boundary->hit(r, rec1.t+0.0001, FLT_MAX, rec2)) {
    if (db) std::cerr << "\nt0 t1 " << rec1.t << " " << rec2.t << "\n";
            if (rec1.t < t_min)
                rec1.t = t_min;
            if (rec2.t > t_max)
                rec2.t = t_max;
            if (rec1.t >= rec2.t)
                return false;
            if (rec1.t < 0)
                rec1.t = 0;
            float distance_inside_boundary = (rec2.t - rec1.t)*r.direction().length();
            float hit_distance = -(1/density)*log(drand48()); 
            if (hit_distance < distance_inside_boundary) {
            if (db) std::cerr << "hit_distance = " <<  hit_distance << "\n";
                rec.t = rec1.t + hit_distance / r.direction().length(); 
            if (db) std::cerr << "rec.t = " <<  rec.t << "\n";
                rec.p = r.point_at_parameter(rec.t);
            if (db) std::cerr << "rec.p = " <<  rec.p << "\n";
                rec.normal = vec3(1,0,0);  // arbitrary
                rec.mat_ptr = phase_function;
                return true;
            }
        }
    }
    return false;
}

Properly Formatted Code

bool constant_medium::hit(const ray& r, float t_min, float t_max, hit_record& rec) const {
    bool db = (drand48() < 0.00001);
    db = false; // ??
    hit_record rec1, rec2;

    if (boundary->hit(r, -FLT_MAX, FLT_MAX, rec1)) {
        if (boundary->hit(r, rec1.t+0.0001, FLT_MAX, rec2)) {
            if (db) std::cerr << "\nt0 t1 " << rec1.t << " " << rec2.t << "\n";

            if (rec1.t < t_min)
                rec1.t = t_min;
            if (rec2.t > t_max)
                rec2.t = t_max;
            if (rec1.t >= rec2.t)
                return false;

            if (rec1.t < 0)
                rec1.t = 0;

            float distance_inside_boundary = (rec2.t - rec1.t)*r.direction().length();
            float hit_distance = -(1/density)*log(drand48());

            if (hit_distance < distance_inside_boundary) {
                if (db) std::cerr << "hit_distance = " <<  hit_distance << "\n";

                rec.t = rec1.t + hit_distance / r.direction().length();
                if (db) std::cerr << "rec.t = " <<  rec.t << "\n";

                rec.p = r.point_at_parameter(rec.t);
                if (db) std::cerr << "rec.p = " <<  rec.p << "\n";

                rec.normal = vec3(1,0,0);  // arbitrary
                rec.mat_ptr = phase_function;

                return true;
            }
        }
    }
    return false;
}

In conjunction with addressing Kris's original question, the code needs to be cleaned up.

trevordblack · 2019-08-21T19:15:00Z

The bool db controls whether the hit info is printed out to stderr. The code operates within a volume. Any given path could bounce dozens of time before completion. If this bool is always true, then every one of those rays will print it's information. This is 100,000s of data points.

The code here means that for every ray in a volume, it creates a random number. If that random number is 1/10000 then it will print that rays data.

In this way, 10s of ray data will dumped rather than 100,000s.

However it was set to always false with the assumption that reader would comment out one of the two instantiations.

This is probably worth keeping, as volumes are crazy hard to debug, but it should be removed from the "beauty code block" and placed into its own code block.

hollasch · 2019-08-21T20:16:11Z

Hah! I was puzzled by the meaning, then realized that the formatting was totally bogus, then reformatted it, then forgot to ask the original question while reading the reformatted code. Duh.

Yes, seems obvious now in retrospect.

What do you mean by "beauty code block"?

Also, a simple periodic sampling would accomplish the same throttling without incurring the hit of generating a new random number, and would be easier to understand (and of course, fix the name in the process):

const  int samplesPerDebug = 10000; // Set to 0 to disable debug sampling.
static int iteration = 0;

bool debug = false;
if (samplesPerDebug > 0) {
    iteration = ++iteration % samplesPerDebug;
    debug = iteration == 0;
}

trevordblack · 2019-08-21T20:40:22Z

I was tongue-in-cheekily referring to the fact that final raytracing images are referred to as "Beauty Images".

One potential problem with the above code is that it samples exactly every 10,000 samples. You could potentially get into problems of aliasing the data, versus the random sampling above.

The sample rate is so outrageously sparse that this shouldn't be a problem is practice.

Controlling how often samples are printed is that bit much more intuitive. So, this gets my vote

hollasch · 2019-08-21T22:39:34Z

Thinking about this a bit more, this would be better:

#if 0                                         // 0 = debug off, 1 = debug on
    bool debug = drand48() < 0.00001;
#else
    const bool debug = false;
#endif

...
if (debug) std::cerr << "hit_distance = " <<  hit_distance << "\n";
...

The fastest code is code that's not there. :D (The if (debug) statement should be optimized away entirely.)

trevordblack · 2019-08-21T23:41:51Z

Since when have we cared about speed? I'm not a fan of this method. We don't use the preprocessor in this manner any where else in the codebase

…

On Wed, Aug 21, 2019, 15:39 Steve Hollasch ***@***.***> wrote: Thinking about this a bit more, this would be better: #if 0 // 0 = debug off, 1 = debug on bool debug = drand48() < 0.00001; #else const bool debug = false; #endif ... if (debug) std::cerr << "hit_distance = " << hit_distance << "\n"; ... The fastest code is code that's not there. :D — You are receiving this because you commented. Reply to this email directly, view it on GitHub <https://github.com/RayTracing/TheNextWeek/issues/44?email_source=notifications&email_token=ACNZVKGTHCOBWO6W6UE2EQTQFW72PA5CNFSM4IOHUW2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD43KZBQ#issuecomment-523676806>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACNZVKDB5KIZE274RRV2ED3QFW72PANCNFSM4IOHUW2A> .

hollasch · 2019-08-22T17:30:22Z

The point was that we certainly could illustrate debugging methods and code alongside the raytracing code more, since debugging support helps the inevitable cases where something "looks off". I care about speed because the development iteration cycle is crucial.

If you're objecting because it's non-conventional to the codebase, I can fix that. :D

If you're objecting because preprocessor control is ugly and too much of a C++ language quirk, I could debate (indeed, I like Simonyi's original idea of moving languages up to ASTs and metalanguages).

Despite all that, I think my rebuttals don't have much weight in this context, and all of my alternatives seem to add more complexity than is warranted.

Certainly, the name must be fixed. The code would also be more clear as

const enableDebug = true;
bool debug = enableDebug && drand48() < 0.00001; // For debug, print occasional samples

if (debug) ...

trevordblack · 2019-08-22T18:36:50Z

I was objecting to the break from convention. We could also hook into the existing compiler defines i.e. _DEBUG If we decided to do this we could go through and add more debug blocks to other parts of NextWeek. Not sure how to feel demanding knowledge of compiler defines to the audience. I think your solution of adding a debug variable in the code is not a bad idea. For the purposes of speed, any compiler will deadcode eliminate for a const bool.

…

On Thu, Aug 22, 2019, 10:30 Steve Hollasch ***@***.***> wrote: The point was that we certainly could illustrate debugging methods and code alongside the raytracing code more, since debugging support helps the inevitable cases where something "looks off". I care about speed because the development iteration cycle is crucial. If you're objecting because it's non-conventional to the codebase, I can fix that. :D If you're objecting because preprocessor control is ugly and too much of a C++ language quirk, I could debate (indeed, I like Simonyi's original idea of moving languages up to ASTs and metalanguages). Despite all that, I think my rebuttals don't have much weight in this context, and all of my alternatives seem to add more complexity than is warranted. Certainly, the name *must* be fixed. The code would also be more clear as const enableDebug = true; bool debug = enableDebug && drand48() < 0.00001; // For debug, print occasional samples if (debug) ... — You are receiving this because you commented. Reply to this email directly, view it on GitHub <https://github.com/RayTracing/TheNextWeek/issues/44?email_source=notifications&email_token=ACNZVKFD2EKFEWIN4ZUZ4SDQF3EK7A5CNFSM4IOHUW2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD452B7Q#issuecomment-524001534>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACNZVKEYROXFVN5L3VRW4SDQF3EK7ANCNFSM4IOHUW2A> .

- Improve naming of debug control variable. - Improve debug logic. - Fix whack indentation. - Slight code refactor. Changes to both code & book. Resolves #44

hollasch assigned petershirley Aug 21, 2019

hollasch referenced this issue in RayTracing/TheNextWeek Aug 23, 2019

Clean up constant_medium::hit()

ec7eb9e

- Improve naming of debug control variable. - Improve debug logic. - Fix whack indentation. - Slight code refactor. Changes to both code & book. Resolves #44

hollasch referenced this issue in RayTracing/TheNextWeek Aug 23, 2019

Clean up constant_medium::hit()

54be5f5

- Improve naming of debug control variable. - Improve debug logic. - Fix whack indentation. - Slight code refactor. Changes to both code & book. Resolves #44

hollasch closed this as completed in RayTracing/TheNextWeek#45 Aug 26, 2019

hollasch transferred this issue from RayTracing/TheNextWeek Aug 29, 2019

hollasch added the book 2: Next Week label Aug 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trying to understand hit function in Chapter 8 Volumes #22

Trying to understand hit function in Chapter 8 Volumes #22

KrisYu commented Aug 21, 2019

hollasch commented Aug 21, 2019

trevordblack commented Aug 21, 2019 •

edited

Loading

hollasch commented Aug 21, 2019 •

edited

Loading

trevordblack commented Aug 21, 2019

hollasch commented Aug 21, 2019 •

edited

Loading

trevordblack commented Aug 21, 2019 via email

hollasch commented Aug 22, 2019

trevordblack commented Aug 22, 2019 via email

Trying to understand hit function in Chapter 8 Volumes #22

Trying to understand hit function in Chapter 8 Volumes #22

Comments

KrisYu commented Aug 21, 2019

hollasch commented Aug 21, 2019

Existing Code

Properly Formatted Code

trevordblack commented Aug 21, 2019 • edited Loading

hollasch commented Aug 21, 2019 • edited Loading

trevordblack commented Aug 21, 2019

hollasch commented Aug 21, 2019 • edited Loading

trevordblack commented Aug 21, 2019 via email

hollasch commented Aug 22, 2019

trevordblack commented Aug 22, 2019 via email

trevordblack commented Aug 21, 2019 •

edited

Loading

hollasch commented Aug 21, 2019 •

edited

Loading

hollasch commented Aug 21, 2019 •

edited

Loading