Force-update BVHs when updating query engine

[doyubkim]

This revision fixes the issue where internal BVH structures are not being updated even when updateQueryEngine is invoked. This issue can cause emitters or colliders to fetch old bounding boxes when one of the surface set types is used for geometry representation as reported in Issue #268.

[kentbarber]

Wouldn't invalidating the bvh of the TriangleMesh3 cause it to rebuild every frame? That may be quite expensive. Currently I am only manually invalidating the bvh for only those emitters and colliders that are dynamic. Any static ones are not invalidated. And I am not allowing moving of TriangleMesh3 objects since I had concerns with this as well, but I don't remember the exact details. I haven't checked the code yet to see if my concerns are valid, will do so this weekend.

[doyubkim]

Actually trimesh doesn’t need such an aggressive cache invalidation as it can figure out when the mesh has changed. I will revert that particular change.

[kentbarber]

The changes you added work for me. Although I haven't included invalidateCache() for the TriangleMesh. Note that I just manually moved across the changes for each file for this fix. I didn't use the full branch.

I will test Triangle Meshes at later date and make a note of the invalidateCache() issue. If the mesh itself doesn't change then it should be able to use its transform to update the boundbox directly without having to recalculate the BBox for every triangle or calling _bvh.build() at all. Perhaps a special case for TriangleMesh3 where it has a special flag to set if you want to recalculate if the points have changed, otherwise just transform the last calculated BBox using the new Transform. updateQueryEngine(DIRTYFLAGS::POINTS | DIRTYFLAGS::MATRIX) or perhaps just a SetDirtyPoints() method for a mesh, since I carry around a pointer to the mesh anyway so it doesn't need to be generic as a parameter to updateQueryEngine.

[codecov-io]

Codecov Report

Merging #271 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #271      +/-   ##
==========================================
+ Coverage   87.89%   87.92%   +0.03%     
==========================================
  Files         549      549              
  Lines       37010    37082      +72     
==========================================
+ Hits        32529    32605      +76     
+ Misses       4481     4477       -4

Impacted Files	Coverage Δ
src/jet/surface_set2.cpp	86.92% <100%> (+0.98%)	⬆️
include/jet/detail/bvh3-inl.h	86.74% <100%> (+0.05%)	⬆️
src/tests/unit_tests/surface_set3_tests.cpp	100% <100%> (ø)	⬆️
...c/tests/unit_tests/implicit_surface_set3_tests.cpp	100% <100%> (ø)	⬆️
...c/tests/unit_tests/implicit_surface_set2_tests.cpp	100% <100%> (ø)	⬆️
src/jet/implicit_surface_set3.cpp	86.39% <100%> (+0.87%)	⬆️
src/tests/unit_tests/surface_set2_tests.cpp	100% <100%> (ø)	⬆️
src/jet/implicit_surface_set2.cpp	86.39% <100%> (+0.87%)	⬆️
include/jet/detail/bvh2-inl.h	86.74% <100%> (+0.05%)	⬆️
src/jet/surface_set3.cpp	86.92% <100%> (+0.98%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d298e3...676d8ab. Read the comment docs.

[doyubkim]

Thanks for your suggestion, @kentbarber! Yes, dirty flag is one way to go. It does have similar flags, but not with such a finer level. For this particular fix/PR, I will focus on addressing surface set issues and think about a more generic way of controlling BVHs within the codebase.

Anyway, I also found a bug where BVH can be an ever-growing box without properly resetting root to null. I also added unit tests. Thus taking off the WIP label and making it mergeable.

[utilForever]

Sorry, I was very busy. I'll review it today. 🙏

[utilForever]

I checked and verified all unit test code. LGTM.
Thanks for the fix! 🎉

[doyubkim]

Thanks @utilForever !