Proposal for new bvh split method (approach Nr. 3) - divide and conquer with std::sort() #1391
Conversation
There was a problem hiding this comment.
If you agree to these changes, then we also need to do the following work:
- Make the same changes to bvh.h in
src/TheRestOfYourLife. - Update the text and any listings for books 2 & 3
- Add an entry to the CHANGELOG.md file.
- Please add your name to
books/acknowledgments.md.
src/TheNextWeek/bvh.h
Outdated
| for (size_t object_index=0; object_index < size; object_index++) | ||
| bbox = aabb(bbox, src_objects[object_index]->bounding_box()); |
There was a problem hiding this comment.
Simplify to:
for (auto& obj : src_objects)
bbox = aabb(bbox, obj->bounding_box());
src/TheNextWeek/bvh.h
Outdated
| // Build the bounding box of the span of source objects. | ||
| bbox = aabb::empty; | ||
| for (size_t object_index=start; object_index < end; object_index++) | ||
|
|
||
| size_t size = src_objects.size (); |
There was a problem hiding this comment.
Move size definition down below the line:
auto objects = src_objects; // A modifiable array of the source scene objects
src/TheNextWeek/bvh.h
Outdated
| } else if (size == 2) { | ||
| if (comparator(objects[0], objects[1])) { | ||
| left = objects[0]; | ||
| right = objects[1]; | ||
| } else { | ||
| left = objects[start+1]; | ||
| right = objects[start]; | ||
| left = objects[1]; | ||
| right = objects[0]; | ||
| } |
There was a problem hiding this comment.
Since we're rewriting this, let's fix issue #1327 in the same commit. Please simplify this case to the following:
...
} else if (size == 2) {
left = objects[0];
right = objects[1];
} else {
...
(Ordering the two child objects in this case accomplishes nothing.)
|
Hi Steve! |
|
You can improve further by writing a helper function to do the job.
This reduce the current |
|
Hi @wlbksy! So a good sort for x in the beginning might not be an optimal sort for z a few levels further down, for example. Or am I misunderstanding this issue? |
I agree, you need to know the spatial subdivision axis and position at each level and the BVH construction code is already handling this. As an alternative, if we really wanted to eliminate all the redundant copies and vector allocations, we could allow the BVH construction to sort the object list in-place without creating any intermediate vectors. The code would look very similar to the original listing but instead remove the @hollasch any thoughts? |
|
Hi @armansito I personally think that this is a great idea! It would probably be super fast and efficient. I always wondered why we are doing the indexing and copying the vector in the first place because doing both kind of negate each other. My proposal works around the issue to simplify the indexing (because it would not be needed) but what you are suggesting is probably the most elegant solution with the smallest RAM and CPU footprint. 😄 |
Oh, you're right. I missed this random axis switching. |
|
Hi! I made some tests with your suggestions, @armansito (see results in my previous bvh thread #1388 ). IMHO, this would be the most elegant solution to most issues, so a new bvh method such as the one from my original proposal would not really be needed anymore. There is also a pull request #1409 with corresponding code and book changes, Steve @hollasch , in case you find it useful. Thanks! |
You didn't force-push the |
No, as you can see in the status message:
I am only modifying the |
|
Phew! Thanks @relief7. |
Hi Steve!
please find proposal for "new bvh split method (approach Nr. 3) - divide and conquer with std::sort()" based on latest dev branch in this pull request (see further info in issue reply).
Thanks,
Markus