bvh_node: no more const qualifier of objects array, axis sorting "in place"

[relief7]

based on @armansito's and @hollasch's suggestions in #1391 (see also further explanation and tests in #1388 )

-removed const qualifier for objects vector in bvh_node constructor so sorting is done "in place"
-hopefully ok because we don't need the scene objects array after being consumed by the bvh
-removed 2nd unnecessary branch within condition "object_span == 2"
-did not change for loop of bbox calculation because using for (auto& obj : objects) instead of for (size_t object_index=0; object_index < size; object_index++) made everything a lot slower
-updated bvh source code in book2 and book3
-modified listings in book2
-added note to changelog
-added names to acknowledgements (Arman was already there 😄)

[hollasch]

We cannot mutate the input list. Any outstanding references into the original hittable list array would be invalidated.

[hollasch]

Indent error.

[hollasch]

You can pretty much achieve all this by going with my original suggestion: the bvh_node constructor that takes a (const) hittable list constructs a vector of raw hittable pointers, and then all recursions modify sections of that copied array.

[armansito]

You can pretty much achieve all this by going with my original suggestion: the bvh_node constructor that takes a (const) hittable list constructs a vector of raw hittable pointers, and then all recursions modify sections of that copied array.

Agreed. The cost of the initial copy should be negligible and you should still see a similar performance improvement by removing the intermediate per-node allocations and copies.

[relief7]

OK thank you for the feedback! Will take another stab at it.

[armansito]

OK thank you for the feedback! Will take another stab at it.

Thank you for bearing with the back and forth and the detailed performance data you have posted. I'm eager to see the new measurements.

[relief7]

Hi! After looking at these changes a bit closer, I am running into a few issues...

Currently, everything in the bvh is based on the type std::shared_ptr<hittable>. This means that left anywhere in the bvh tree could point to different objects.

std::shared_ptr<hittable> left points to:
-bvh
-sphere
-quad
-hittable_list
-...

The whole beauty and simplicity of the bvh currently is based on the fact that we only need to deal with one data type and therefore can treat all these items the same at all levels. We probably agree that we don't want to introduce heterogeneous data types, so if our input is now an array of raw hittable * pointers, it means we have to switch the internal bvh data type to raw pointers as well. So our new left / right nodes become:

hittable * left points to:
-bvh
-sphere
-quad
-hittable_list
-...

Otherwise we cannot store hittables and bvh nodes in the same left / right pointers.

Creation of new bvh nodes used to be:
left = make_shared<bvh_node>(objects, start, mid);

and it becomes:
left = new bvh_node(objects, start, mid);

We have to switch to new() to create new instances of bvh_node, otherwise they will get destroyed when running out of scope. So in order to prevent memory leaks, we would also have to manually manage lifetime with delete.

Issues:

1. Since my experience with raw pointers is very limited, I don't really feel fit enough to revamp all internals of the bvh class since it requires not only storing hittable raw pointers but also managing lifetime of bvh pointers. It would probably take me a very long time to ensure safety and proper handling/releasing of resources.
2. IMHO, having to manually manage resources may be overkill if this tutorial is aimed at beginner levels. If we say that from a didactic perspective, the primary focus of this tutorial is learning the concepts and not speed at any cost, I feel this may not be necessary and further complicates the code.
3. I like the beauty and simplicity of having shared_ptr<hittable> as a reference to hittables throughout the whole tutorial as it provides a common interface from one chapter to the next. Changing the bvh in this way would break this paradigm.

For these reasons, I am afraid I won't be able to do these code changes unless you have tips on how to solve this elegantly.

An easy solution to the problem about the mutated input list (without converting the bvh to raw pointers) would be to remove the const reference and copy input list by value:
old:
bvh_node(const hittable_list& list) : bvh_node(list.objects, 0, list.objects.size()) {}
new:
bvh_node(hittable_list list) : bvh_node(list.objects, 0, list.objects.size()) {}

That's an easy fix that I could provide if you want me to.

[hollasch]

https://www.youtube.com/watch?v=7FPELc1wEvk

Gotcha. I agree with your conclusion. Might have been nice to simplify things, but let's keep everything as std::shared_ptr<hittable>.

[relief7]

Ok, thank you for the feedback and the hilarious video! 🤣

I updated pull request with:
-removed pass-by-ref in bvh constructor (so the input hittable_list is copied once and not mutated inside the method):
bvh_node(hittable_list list) : bvh_node(list.objects, 0, list.objects.size()) {}
-updated book listings
-fixed indentations in books/RayTracingTheNextWeek.html

speed wise, the time for copying the input once seems negligible:

• 100,000 spheres

  • before:
    1.78006
  • after:
    1.83080

• 1,000,000 spheres

  • before:
    26.7226
  • after:
    27.9355

[hollasch]

Holy cow; that took me longer to grok than it should have! I couldn't figure out why this wasn't just a reversion of everything. Finally realized that the constructor without indices creates an implicit copy of the source list, and the constructor with the indices just uses the reference to the copy.

It makes me want to explain this subtlety somehow, mostly because it's due to subtle aspects of C++ that would be easy to mistakenly drop when implementing in other languages. I think I'll just squash+merge this in, and then issue a follow-up PR to explain this subtlety.

[hollasch]

I'll add some more clarification in a subsequent change.

[hollasch]

(Also, please keep the first lines of commits to 50 characters, and the remainder of commit comments to 72 characters.)