lighttransport / nanort

NanoRT, single header only modern ray tracing kernel.
MIT License
1.07k stars 89 forks source link

Possible bug - intersector.Intersect(...) overrides uvs without ensuring local_t > 0 #16

Closed Domenicobrz closed 6 years ago

Domenicobrz commented 6 years ago

I'm reporting a possible bug related to the uv coordinates modified by the function

template <typename T = float, class H = TriangleIntersection<T>>
class TriangleIntersector {
...
bool Intersect(T *t_inout, unsigned int prim_index) { ... }    ln. 765
...
}

called by

inline bool BVHAccel<T>::TestLeafNode(const BVHNode<T> &node, const Ray<T> &ray,
 const I &intersector) const { ... }        ln. 1823

After a careful review of my current project and after ensuring none of my code was interfering with texture sampling, I started to dig inside the ray-intersection routine and found a possible bug in those functions

In my current project, I'm using a textured light source whose emissive radiance is sampled from a texture, and in certain scenes the returned barycentric uvs for the emissive primitives is wrong, since apparently TestLeafNode lets the intersector change the uv values before ensuring the local_t is greater than 0 ( at ln.1848)

Here's a detailed explanation and an example of what is going wrong:

After the bool BVHAccel<T>::Traverse ln.1936

function is called, the normal ray-intersection routine inside the while loop finds the correct leaf node at:

if (TestLeafNode(node, ray, intersector)) {
   hit_t = intersector.GetT();
}  // ln.1992

and stores the correct results for the primitive as e.g. t = 200.0, u = 0.88, v = 0.06

Since the while loop isn't yet complete, it keeps iterating the rest of the AABB stack until another Leaf gets hit and tested. As soon as TestLeafNode( ... ) is called, two primitives needs to be tested. Both primitives will end up having a negative t value, since these objects are behind the ray direction, but here's the problem

ln. 1847
if (intersector.Intersect(&local_t, prim_idx)) {
   if (local_t > ray.min_t) {
        // Update isect state
       t = local_t;
       intersector.Update(t, prim_idx);

       hit = true;
   }
}

intersector.Intersect(...) will test those primitives and override the uv values previously stored u = 0.88 and v = 0.06 before ensuring the local_t of those primitives is greater than the ray.min_t

This way, I end up with the correct hit_t value (e.g. at 200.0) and incorrect uv values (the values of those primitives whose hit_t was negative)

Fixing the above issue completely solved the problem in my project

syoyo commented 6 years ago

Thank you for detailed report, @Domenicobrz!

I understand the situation and I need to rethink API design to solve the issue. Let me give some time for figuring out the best & elegant solution.

syoyo commented 6 years ago

I have added t_min check in Intersect method in fix-intersect branch.

https://github.com/lighttransport/nanort/commit/d4723a8904dcf94ab728854e657340e60962f45c#diff-6ae3ef545df31bcbb8d6f3814304e805R822

Could you please confirm this fix solves the issue, @Domenicobrz ?

Domenicobrz commented 6 years ago

@syoyo I'll try tomorrow to test this fix and see if it works for that specific scene I had trouble with

Domenicobrz commented 6 years ago

I can confirm the fix solved the problem, switching to the old build caused the bug to recur

syoyo commented 6 years ago

Thanks!

syoyo commented 6 years ago

I have merged fix-intersect branch into master.

syoyo commented 6 years ago

And thank you for finding a bug. You are our hero!

Domenicobrz commented 6 years ago

Thanks for your work! happy raytracing