Add support for reentrant qhull#4540
Conversation
|
I think this one can be merged, after ci 16.04 is removed. |
cmake/Modules/FindQhull.cmake
Outdated
| set(QHULL_MAJOR_VERSION 8) | ||
|
|
||
| if(QHULL_USE_STATIC) | ||
| set(QHULL_RELEASE_NAME qhullstatic) |
There was a problem hiding this comment.
I think this should be changed to qhullstatic_r (and below qhullstatic_r_d)
|
With the addition of the minor change above, I found this PR works nicely when building pcl on conda-forge: conda-forge/pcl-feedstock#30 |
kunaltyagi
left a comment
kunaltyagi
left a comment
There was a problem hiding this comment.
The CMake along with the HAVE_QHULL_2011 flag needs to be looked at more deeply. Sadly, I'm not an active user of surface, and my testing will not be deeper than what the CI already does
The other changes LGTM (though I'm surprised only these few changes are needed)
|
@al-tu Could you delete these two lines so that our Ubuntu 20.10 check also runs the hull tests? |
Some of this was discussed in #3741. |
|
Based on that discussion, the conditional statements for 2011 shouldn't be needed (since the lowest we support is 18.04) |
|
@al-tu Are you still willing to work on this pull request so that we can hopefully merge it soon? |
done |
|
build failed with "out of disk space" error on runner, should i just retry (i hope some rotation takes place in such cases)? |
I re-ran the failed test, that error happens from time to time, we don't have a solution for that yet. |
kunaltyagi
left a comment
kunaltyagi
left a comment
There was a problem hiding this comment.
All instances of qh macro and 2011 macro have been replaced :)
There are some instances where qh_* functions don't take a qh parameter (eg: concanve_hull.hpp, line 272), but I assume that's not an issue because the CI doesn't complain
PR LGTM otherwise (Will approve after some discussion)
Should we be changing our minimum versions supported after this PR?
Where do you mean? In the tutorials about compiling PCL or somewhere else? |
kunaltyagi
left a comment
kunaltyagi
left a comment
There was a problem hiding this comment.
That's true, tutorials is quite outdated wrt claim of min version
* using reentrant qhull by default, qhull calls edits * using reentrant qhull by default, qhull calls edits * using reentrant qhull by default, qhull calls edits * cmake for qhull fix * cmake for qhull fix * static libs made reentrant as well * turning surface tests on for 20.10 * typos fixes, using errfile for qh_zero second param * removing HAVE_QHULL_2011 and assuming its always true * removing QHULL_MAJOR_VERSION
5 participants
non-reentrant qhull is deprecated
for me turned out to be the only way to deal with "undefined reference to 'qh_qh'" with any qhull/pcl build options combinations i tried
tested with qhull 8.0.2
related issues/PRs:
#1618
#1565