Removed bounding_box calls with magic numbers

[trevordblack]

Resolves #749

[hollasch]

This looks wrong to me. In the constructor, we compute the bbox of the rotated object and store it in this->bbox. Here we compute the original (unrotated) geometry bbox and return that instead, clobbering the computed bbox.

[trevordblack]

how about:

virtual bool bounding_box(double time0, double time1, aabb& output_box) const override {
aabb unrotated_box;
if (ptr->bounding_box(time0, time1, unrotated_box) {
    output_box = bbox;
    return true;
}
else {
    return false;
}

[hollasch]

Ah, I see what's going on. The rotated object is constructed with a given rotation angle, so is constant ever after. The rotated object, however, may be time-varying.

Given this, we can't compute the bounding box of the child object in the constructor, since we have no time parameters at construction. Even if we did, there's no point, because the bounding_box method takes some (possibly different) time parameters. So we need to recompute the bounding box on every call.

Thus:

1. Remove the bbox member from the class in addition to the hasbox flag. They're both unusable.
2. We have code in the constructor that computes the bounding box of the rotated child object's box. That code no longer has any use in the constructor, so move that into the rotate_y::bounding_box() method.
3. You'll thus need access to the rotation angle, so you'll have to store that in a member variable.

[hollasch]

The end result will be that the body of rotate_y::bounding_box() will end up looking a lot like what the constructor currently looks like.

[hollasch]

Note that this is pretty much the way that the translate class operates.

[hollasch]

Ugh.
Wait.

There's a discrepancy here. Like, a big one.

A ray has a specific time value. So a hit result samples the object over time. However, bounding_box() methods take a range of times. Thus, they must yield the bounds of the object over that range, as opposed to a tight bounds for a specific instance in time.

That range of time is the range of time values for the entire rendering, and is thus invariant. Therefore, we should know the time range at object construction, and can safely cache the result. So rotate_y is actually the way that things should be, except that it hard-coded the time range.

translate incorrectly recomputes the bounding box over and over for every query — the time range is always the same, and therefore the result should just have been cached at construction and returned.

But that means that logically, every hittable should take the time range as parameters to their constructor. Which would be a major change.

[hollasch]

I think this change is screwed, and should be lumped in with the other bounding-box work slated for v4.0.0.

[hollasch]

There's another possible (major) design approach, which is that instead of every hittable taking a time range in the constructor, they have a set_time_range() method. For most simple primitives, a no-op. For time-varying primitives, this sets the time range, which will be used by bounding_box(). For hittables that contain children, they proxy the set_time_range() call to each child, and possibly compute and cache their bounding box. (The bounding box could be memoized in the bounding_box() method, but no point doing that on each call; bounding_box() might be called many times, while set_time_range() should be called only once.

Just a thought.

[hollasch]

Clarifying the comment above, just prior to scene render, you'd call set_time_interval() (better name than set_time_range()) on the root of the scene, to set up the scene graph once prior to rendering, after the scene time interval is known. Prior to rendering the second frame, you'd call set_time_interval() with arguments for the second frame.

[trevordblack]

We should consider changing the ptr value in rotate_* to something more specific like, something else.

Uhh.

primitive, base, unmoved_hittable_ptr

[hollasch]

Ok, we're going to rethink this for v4.0.0 instead.

[hollasch]

This turns out to be a larger issue than a small patch change. Deferring to v4.0.0.

[hollasch]

See #799