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 Two: [hittable_list.h] Hittable list with bounding box #435

Closed
dafhi opened this issue Apr 2, 2020 · 2 comments
Closed

Book Two: [hittable_list.h] Hittable list with bounding box #435

dafhi opened this issue Apr 2, 2020 · 2 comments

Comments

@dafhi
Copy link
Contributor

dafhi commented Apr 2, 2020

Master branch

'i' was not declared ..
if (!objects[i]->bounding_box(t0, t1, temp_box))

@hollasch
Copy link
Collaborator

hollasch commented Apr 3, 2020

for (const auto& object : objects) {
    if (!objects[i]->bounding_box(t0, t1, temp_box))
        return false;
    output_box = surrounding_box(output_box, temp_box);
}

should be

for (const auto& object : objects) {
    if (!object.bounding_box(t0, t1, temp_box))
        return false;
    output_box = surrounding_box(output_box, temp_box);
}

Reviewing this code, I don't understand why this method doesn't ignore false returns, and just return false only if none of the objects has a bounding box.

@hollasch hollasch added this to the v3.0.2 milestone Apr 3, 2020
@hollasch hollasch self-assigned this Apr 3, 2020
@trevordblack
Copy link
Collaborator

trevordblack commented Apr 3, 2020

It looks like it's correct in the source, but not in the html.

as for:

I don't understand why this method doesn't ignore false
returns, and just return false only if none of the objects
has a bounding box

I'm not entirely sure. A return of false for bounding_box means that an aabb isn't defined for that object.

I think returning false is for two reasons

  1. Strictness. An underdefined hittable is not allowed
  2. We don't have a way to combine no aabb and 1 aabb, instead of false you would need to keep track of if temp_box has been defined, if the current hittable has an aabb, and when to combine the two. It's actually a bit complicated

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

3 participants