PointCloudLibrary / pcl

Point Cloud Library (PCL)
https://pointclouds.org/
Other
10.01k stars 4.62k forks source link

pcl::ConvexHull crashes #410

Closed madduci closed 10 years ago

madduci commented 10 years ago

Hi

I've successfully compiled PCL from trunk on ArchLinux 64 bit with the following libraries:

Boost 1.55 Eigen 3.2 stable Flann 1.8.4 VTK 5.10 QHull 2012 OpenNI 1.5.4 CUDA 5.5

When i run the convex hull algorithm, the program quits because of Segmentation fault signal. On another machine, with Ubuntu 13.10 and all the latest libraries, the software runs perfectly. What could be the cause of the segmentation fault on that specific point?

rbrusu commented 10 years ago

QHull 2012 broke the QHull ABI and internal logic, so we need to change our code. The other machine that you're running on has an older version of QHull (2009 or 2011) which is why everything works ok. We need someone to submit a pull request with a patch.

madduci commented 10 years ago

any new on this bug? In Ubuntu 14.04, which will be a LTS, now only qhull2012 is available in the repository.

jspricke commented 10 years ago

I've just tested QHull 2012 without problems (unit test and pcl_compute_hull bun01.pcd). Can you come up with a minimal example for me to reproduce?

v4hn commented 10 years ago
#include <iostream>

#include <pcl/point_types.h>
#include <pcl/point_cloud.h>

#include <pcl/surface/convex_hull.h>
//#include <pcl/surface/impl/convex_hull.hpp>

typedef pcl::PointXYZ Point;
typedef pcl::PointCloud<Point> PointCloud;

int main(){
    PointCloud::Ptr cloud(new PointCloud);

    cloud->push_back(Point(0,0,0));
    cloud->push_back(Point(1,0,0));
    cloud->push_back(Point(0,1,0));
    cloud->push_back(Point(0,0,1));
    cloud->push_back(Point(0.1,0.1,0.1));

    pcl::ConvexHull<Point> chull;
    chull.setInputCloud(cloud);

    PointCloud hull_cloud;
    std::vector<pcl::Vertices> hull_polygons;

    chull.reconstruct(hull_cloud, hull_polygons);

    std::cout << "reconstruction succeeded. received hull with " << hull_cloud.size() << " points." << std::endl;

    return 0;
}

Interestingly the code does work with the implementation of ConvexHull included directly.

v4hn commented 10 years ago

ok

It's pretty simple actually. Although the qhull dev(s) made it pretty hard to debug this due to obfuscated code and design flaws.. At least they put a note about the solution onto their NEWS page http://www.qhull.org/news/qhull-news.html . QHull now provides qh_QHpointer and global struct versions of the library as static and shared libraries each and the new libqhull.so is compiled without qh_QHpointer in 2012. PCL detects libqhull.so and uses it as if it was compiled with qh_QHpointer aaand voila, you're in the middle of a jungle of undetected code incompatibilities..

I changed the library to the new "libqhull_p.so" and things work as expected. Would be great to see this fixed in 1.7.2 :)

This brings us to politics though... It looks like ubuntu/debian didn't care to include these qh_QHpointer shared libs in their libqhull6 packages until now. I therefore assume pcl will only work with the static libs they provide.

Also, it's important to fix PCLConfig.cmake.in:141. Not everyone's on Windows (why is this even "most likely" the case?) and this shared/wrong static mixup makes it even harder to see through this linking mess.

I should get around to create a pull-request for this over the weekend..

madduci commented 10 years ago

Unfortunately, this doesn't work on Windows (MSVC2012 here): linking qhullstatic.lib or qhull.lib or qhullstaticlib_p.lib or qhull_p.lib produces always linking errors on ConvexHull. I think this patch broke somehow the Windows compatibility. See #952

v4hn commented 10 years ago

Could you please provide details? What are the exact linking errors?

If none of the libraries work this change didn't break it. It only restricts the required qhull for projects using pcl to the specific qhull that was used to build pcl itself.

jspricke commented 10 years ago

As commented in #952 by blackibiza4, there is no problem with the patch.

madduci commented 10 years ago

I've solved it by myself. It wasn't your patch, it was a QHull fault at all. They provide a ready MSVC solution file for 2012.1, which doesn't contain the -Dqh_QHpointer flag. Using CMake to generate the correct solution file, works definitively, but on Windows, qhullstatic_p and qhullstatic_pd must be linked, instead of qhull_p and qhull_pd, as suggested, otherwise an undefined reference on 'qh_qh' is given. The patch solved the issue at all with qhull, it is all about this funny "Windows game" to find and use the correct libs.

tkircher commented 4 years ago

Ran into this issue building a recent version of PCL (1.11) with a recent version of qhull (8.0). Tried linking PCL with libqhull_r and of course ended up with an undefined reference to qh_qh. There's nothing in the PCL documentation about this. Linking to libqhullstatic.a solved it, as described above. As far as I can tell, this thread is the only explanation of this issue anywhere.

jiapei100 commented 4 years ago

Yeah... found the same issue... Does this belong to a PCL issue ? or a qhull issue?? Check my issue at https://github.com/PointCloudLibrary/pcl/issues/4394 .

kunaltyagi commented 4 years ago

Please keep conversation in the thread that you started.

@tkircher (or anyone else) Please feel free to create a PR to add this in the documentation

tkircher commented 4 years ago

This might be the best place to leave it. At the moment, getting a working build of libpcl with dependencies that aren't literally years and years out of date takes an experienced developer, and that wouldn't really change even with good documentation. But people who run into these same issues will find these threads and either give up or puzzle it out.

kunaltyagi commented 4 years ago

dependencies that aren't literally years and years out of date

IMO, that's a false statement based on following observations:

  1. PCL works with all the latest releases of its dependencies (except VTK-9) on ubuntu (sadly, we can't track every single distro). Even for VTK 9, there's a patch in progress.
  2. PCL CI shows how absurdly simple setting up PCL for a "normal build" is.

If you're interested in making PCL work with the unreleased (on ubuntu-latest) version of libraries or abnormal build setups, please feel free to contribute too. For open source projects, it is vital that the community contributes actively, else the project goes out of sync with it