Change hittable::bounding_box method signature

[hollasch]

Current signature is bool bounding_box(aabb& output_box) const.

Currently, the only way it can return false is from a hittable_list object with no children.

That means that ever caller much check the return value before processing the bounding box, an operation that's frequently unnecessary, and a speed bump.

There are two ways to approach this:

1. Use the new Interval::empty capability

For cases (or the current single case) where the bounding box is empty, return an empty aabb. This can be implemented with a aabb::is_empty() method that returns true iff any of its dimension intervals are empty. Alternatively, we could just skip the check and work with the box normally. Generally, computing the hull of bounding boxes with an empty box should just computationally yield the correct result (like adding zero to a sum). One tricky challenge is the transform classes (like translate and rotate_y). These may need to check the box first, but that's easily done, and only such cases need to inspect the return from bounding_box.

2. Empty bounding boxes just bound a single arbitrary point

For example, an empty hittable_list could just return the box around the origin. Everything would still work, it's just that in rare occasions you might find the box enlarged considerably when it contributes to other faraway bounding boxes. The code would still work, it just might be less optimized than it otherwise could be. Also note that we wouldn't normally encounter a hittable_list with no children.

Generally, I lean toward option 1. It's quite useful to have a AABB class that can handle empty and universe boxes, and in the end you still end up with code that's incrementally simpler than what we have today.

[johannmeyer]

@hollasch Could you not create a default constructor for AABB() that creates the object using the opposite extremes? I noticed something similar was done in hittable.h with min and max.

aabb() : minimum(infinity*Point3(1,1,1)), maximum(-infinity*Point3(1,1,1)) {}

The hit for an AABB checks the following:

if (t_max <= t_min)
    return false;

When you are using the extremes it will return false. This would clean up the hittable_list bounding_box although I see now that in dev-major it has already been changed. Although, I also noticed that the hittable_list never has its bounding_box function called. surrounding_box handles this correctly as aabb's min and max return the value stored in minimum and maximum and not the actual min or max of both values stored in the AABB.

aabb bbox = aabb();
if (objects.empty())
   return bbox; // unhittable

aabb temp_box;

for (const auto& object : objects)
{
    object->bounding_box(temp_box);
    output_box = surrounding_box(output_box, temp_box);
}
return bbox;

[hollasch]

I'm having difficulty understanding what exactly you're proposing. Are you reporting a bug in the new hittable::bounding_box() implementation, or are you suggesting an improvement? If the new code is buggy, I'll reopen this issue, otherwise we should create a new one.

Taking your comments one-by-one, the new AABB() constructor effectively already does what you're suggesting, because interval defaults to [+∞,-∞].

The t_max <= t_min probably is a bug, and I think that we should add a new issue to create aabb::is_empty() and use that at the beginning of the aabb::hit() method (true ⇒ return false).

The current implementation of hittable_list appears to properly aggregate bounding boxes for all cases, but I'm not sure if you're indicating that we should create a second new issue to add an early out to hittable_list::hit().