Hacker News Reader

Comment by pixelesque | Hacker News Reader

JFYI: Your inverse ray direction calculation is not NaN-safe: if rays are completely axis-parallel in one dimension, so the direction value is 0.0 for that axis, you'll be doing the val / 0.0 which results in a NaN...

Also, as you're using full double/f64-precision all the time, you're leaving a fair bit of performance on the table: transcendentals (sin(), cos(), etc) in particular - can be a lot slower than when using f32, and generally double precision can be special-cased to particular areas of the renderer that need it (curve, sphere intersection, and some situations where volume scattering produces very small distances).

View on HN

> Also, as you're using full double/f64-precision all the time, you're leaving a fair bit of performance on the table

There's another issue that popped up on my quick naive profiling run: std::shared_ptr<Material> in the HitRecord/HittableLightSample is assigned/copied and destroyed a lot, and somehow these refcount operations show up as half of all samples on my profile (presumably because even if there's no hit and the pointer stays nullptr, the smart pointer still must check if there's anything to deallocate).

Yeah, passing std::shared_ptr by value in a multi-threaded setup can have a lot over overhead due to them being copied and destroyed a lot, and the fact that the atomic ref count value modifications effectively cause a write back to cache and can cause contention.

Should pass them by const refs really to avoid this.

Or for a better alternative, just use plain old indices rather than shared pointers.

The scene is only going to be loaded / unloaded all at once, you can just load the data into contiguous arrays and index from them. No need to use shared_ptr since lifetimes aren't that complex.

Or just raw pointers, indeed.

std::shared_ptrs can also (because they're implicitly for sharing) alias, so the compiler has to assume the worst and emit loads in other cases, and there's no way (unless a newer C++ version has introduced it and I haven't noticed?) to use '__restrict__' with shared ptrs.

Implemented this and the other improvements suggested on the thread. Performance up by 20%! Thanks for the feedback. https://github.com/themartiano/luz/pull/3

What's the proper way to handle a zero in the direction vector when calculating the reciprocal direction? Should it evaluate to infinity?

Inverse is still 0.0 technically, but yes, there is a trick you can use with Inf and SIMD to mask them out, so Inf is sometimes used.

However, I'd just condition it for the moment.

so:

invDirX = dirX != 0.0 ? 1.0 / dirX : 0.0; etc, etc for each dimension.

Obviously doing the != 0.0 comparison is not great, as it suffers from potential issues again (especially if you have denormals), but you can generally get away with it I've found in most cases.

Reply on news.ycombinator.com