Skip to content

Removed bounding_box calls with magic numbers #770

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Removed bounding_box calls with magic numbers #770

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d7a0375
Removed bounding_box calls with magic numbers
trevordblack Oct 20, 2020
65d55e1
Forgot to update CHANGELOG
trevordblack Oct 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions books/RayTracingTheNextWeek.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -989,6 +989,13 @@
) {
auto objects = src_objects; // Create a modifiable array of the source scene objects

aabb box_check;
for (size_t i = start; i < end; i++) {
if (!objects[i]->bounding_box(time0,time1,box_check)) {
std::cerr << "No bounding box in " << i << " bvh_node constructor.\n";
}
}

int axis = random_int(0,2);
auto comparator = (axis == 0) ? box_x_compare
: (axis == 1) ? box_y_compare
Expand Down Expand Up @@ -1056,10 +1063,6 @@
inline bool box_compare(const shared_ptr<hittable> a, const shared_ptr<hittable> b, int axis) {
aabb box_a;
aabb box_b;

if (!a->bounding_box(0,0, box_a) || !b->bounding_box(0,0, box_b))
std::cerr << "No bounding box in bvh_node constructor.\n";

return box_a.min().e[axis] < box_b.min().e[axis];
}

Expand Down Expand Up @@ -2929,14 +2932,13 @@

virtual bool bounding_box(double time0, double time1, aabb& output_box) const override {
output_box = bbox;
return hasbox;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@trevordblack trevordblack Oct 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@hollasch hollasch Oct 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

return ptr->bounding_box(time0, time1, output_box);
}

public:
shared_ptr<hittable> ptr;
double sin_theta;
double cos_theta;
bool hasbox;
aabb bbox;
};
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Expand All @@ -2951,7 +2953,6 @@
auto radians = degrees_to_radians(angle);
sin_theta = sin(radians);
cos_theta = cos(radians);
hasbox = ptr->bounding_box(0, 1, bbox);

point3 min( infinity, infinity, infinity);
point3 max(-infinity, -infinity, -infinity);
Expand Down
11 changes: 7 additions & 4 deletions src/TheNextWeek/bvh.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,6 @@ class bvh_node : public hittable {
inline bool box_compare(const shared_ptr<hittable> a, const shared_ptr<hittable> b, int axis) {
aabb box_a;
aabb box_b;

if (!a->bounding_box(0,0, box_a) || !b->bounding_box(0,0, box_b))
std::cerr << "No bounding box in bvh_node constructor.\n";

return box_a.min().e[axis] < box_b.min().e[axis];
}

Expand All @@ -73,6 +69,13 @@ bvh_node::bvh_node(
) {
auto objects = src_objects; // Create a modifiable array of the source scene objects

aabb box_check;
for (size_t i = start; i < end; i++) {
if (!objects[i]->bounding_box(time0,time1,box_check)) {
std::cerr << "No bounding box in " << i << " bvh_node constructor.\n";
}
}

int axis = random_int(0,2);
auto comparator = (axis == 0) ? box_x_compare
: (axis == 1) ? box_y_compare
Expand Down
4 changes: 1 addition & 3 deletions src/TheNextWeek/hittable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,14 +90,13 @@ class rotate_y : public hittable {

virtual bool bounding_box(double time0, double time1, aabb& output_box) const override {
output_box = bbox;
return hasbox;
return ptr->bounding_box(time0, time1, output_box);
}

public:
shared_ptr<hittable> ptr;
double sin_theta;
double cos_theta;
bool hasbox;
aabb bbox;
};

Expand All @@ -106,7 +105,6 @@ rotate_y::rotate_y(shared_ptr<hittable> p, double angle) : ptr(p) {
auto radians = degrees_to_radians(angle);
sin_theta = sin(radians);
cos_theta = cos(radians);
hasbox = ptr->bounding_box(0, 1, bbox);

point3 min( infinity, infinity, infinity);
point3 max(-infinity, -infinity, -infinity);
Expand Down
11 changes: 7 additions & 4 deletions src/TheRestOfYourLife/bvh.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,6 @@ class bvh_node : public hittable {
inline bool box_compare(const shared_ptr<hittable> a, const shared_ptr<hittable> b, int axis) {
aabb box_a;
aabb box_b;

if (!a->bounding_box(0,0, box_a) || !b->bounding_box(0,0, box_b))
std::cerr << "No bounding box in bvh_node constructor.\n";

return box_a.min().e[axis] < box_b.min().e[axis];
}

Expand All @@ -73,6 +69,13 @@ bvh_node::bvh_node(
) {
auto objects = src_objects; // Create a modifiable array of the source scene objects

aabb box_check;
for (size_t i = start; i < end; i++) {
if (!objects[i]->bounding_box(time0,time1,box_check)) {
std::cerr << "No bounding box in " << i << " bvh_node constructor.\n";
}
}

int axis = random_int(0,2);
auto comparator = (axis == 0) ? box_x_compare
: (axis == 1) ? box_y_compare
Expand Down
4 changes: 1 addition & 3 deletions src/TheRestOfYourLife/hittable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,14 +129,13 @@ class rotate_y : public hittable {

virtual bool bounding_box(double time0, double time1, aabb& output_box) const override {
output_box = bbox;
return hasbox;
return ptr->bounding_box(time0, time1, output_box);
}

public:
shared_ptr<hittable> ptr;
double sin_theta;
double cos_theta;
bool hasbox;
aabb bbox;
};

Expand All @@ -145,7 +144,6 @@ rotate_y::rotate_y(shared_ptr<hittable> p, double angle) : ptr(p) {
auto radians = degrees_to_radians(angle);
sin_theta = sin(radians);
cos_theta = cos(radians);
hasbox = ptr->bounding_box(0, 1, bbox);

point3 min( infinity, infinity, infinity);
point3 max(-infinity, -infinity, -infinity);
Expand Down