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

BVH leaf's hit() function may be called twice #804

Closed
kyamant opened this issue Nov 15, 2020 · 2 comments
Closed

BVH leaf's hit() function may be called twice #804

kyamant opened this issue Nov 15, 2020 · 2 comments

Comments

@kyamant
Copy link

kyamant commented Nov 15, 2020

During the construction of the BVH tree, the following case will create a leaf's parent node whose left and right pointers being equal:

if (object_span == 1) {
        left = right = objects[start];

Which will result in the leaf object's hit() function getting invoked twice in bvh_node::hit():

bool hit_left = left->hit(r, t_min, t_max, rec);
bool hit_right = right->hit(r, t_min, hit_left ? rec.t : t_max, rec);

This can be remedied by checking if right is not equal to left before executing right->hit(...).

@trevordblack
Copy link
Collaborator

trevordblack commented Nov 16, 2020

The bvh_node::hit function is designed to be as fast as possible.

We expect (and want) the BVH to be filled as evenly as possible.
A node with only a single object_span should either be a scene hittable_list with a single object, or a leaf of a much bigger tree.

Optimizing a bvh with 1 node, or the leaves of a BVH, is an O(1) optimization.
We would make the O(1) case faster.

We would unfortunately be adding more checks to every other node. So we would have a O(LogN) loss.

This is a bad trade off.
Furthermore, checking the same node twice in a row won't actually be twice as long. With caching it'll take meaningfully less than that, maybe ~1.5-1.7x

If you're familiar with profiling code, I strongly suggest profiling your own code and getting an intuition for where the optimization and bottlenecks are in the code.

This was an awesome catch.
It's just not necessarily a performance bug.

@kyamant
Copy link
Author

kyamant commented Nov 16, 2020

Thanks for the clarification.
Closing the issue...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants