Reduce dynamic allocations for AABB queries#2001
Reduce dynamic allocations for AABB queries#2001alecjacobson merged 5 commits intolibigl:mainfrom svenpilz:reduceDynamicAllocations
Conversation
alecjacobson
left a comment
There was a problem hiding this comment.
Thanks! Overall, this is a good change. I left a couple notes, it'd be nice to hear your thoughts on.
include/igl/ray_mesh_intersect.cpp
Outdated
| typename DerivedV, | ||
| typename DerivedF> | ||
| IGL_INLINE bool ray_triangle_intersect( | ||
| Eigen::Vector3d s_d, |
There was a problem hiding this comment.
should these be const references rather than non-const pass-by-copy?
There was a problem hiding this comment.
Yes. I cannot remember why I did it like that. Will fix that.
I was also not sure about your usage of auto and if you like named variables to give some context, even if they are basically just temporaries. Like auto d = v.cast<double>(); foo(d) vs. foo(v.cast<double>()).
There was a problem hiding this comment.
I remember now why it took it by copy, intersect_triangle1 needs non-const arguments. But reworked to hide this from the caller and to be consistent with the other functions.
| template < | ||
| typename DerivedV, | ||
| typename DerivedF> | ||
| IGL_INLINE bool ray_triangle_intersect( |
There was a problem hiding this comment.
should document this in the header.
There was a problem hiding this comment.
Guess it makes sense to also have the vectors's scalar type as a template parameter?
There was a problem hiding this comment.
Added to the header.
| { | ||
| hit = hits.front(); | ||
| return true; | ||
| // Shortcut for AABB based queries. |
There was a problem hiding this comment.
would it make more sense for the AABB code to call this one directly?
There was a problem hiding this comment.
That is a good question. I haven't really looked at the AABB code. Do you think people would use this function here directly, could this be a common use case? If the AABB is the only caller with one triangle, guess adapting there would be the better place.
|
Also added a small unit test. |
|
hopefully the builds and tests work and we can merge this |
|
Could not reproduce the failing test on Linux. Tried GCC 10, and clang 13 in Debug with address and undefined sanitizer. @alecjacobson, do you have an idea here? |
Regarding discussion #1959.
Reduces dynamic allocations which would reduce performance on Emscripten/WebAssembly builds when multi-threading is enabled. Enabling multi-threading on that platform makes
malloc/freeatomic.Checklist