Updates Qhull to 2015.2 using the C++ interface by fran6co · Pull Request #1565 · PointCloudLibrary/pcl

fran6co · 2016-03-06T17:45:03Z

The old qhull had problems with multi threading (reentrant variant of the library) and outputing stuff to the stderr/stdout. This fixes all that and updates the interface to something a bit more usable.

Removed support to previous versions, not sure what's the best way to keep those. I was thinking of creating a different version of the impl files and include them depending on the version.

Depends on #1567

Closes #1618

fran6co · 2016-03-07T23:09:03Z

The C++ interface needs the library to be linked statically, it crashes otherwise.

fran6co · 2016-03-08T15:57:01Z

The method computeMeanAndCovarianceMatrix does the wrong covariance calculation for some reason. I'm investigating what's wrong with it.

SergioRAgostinho · 2017-07-06T12:09:24Z

I don't know what to do with this PR.

Things changed a little bit on our CI we don't manually build dependencies any more (for now), because there's not enough time. You would need to precache it in a different stage.

The reentrant version of qHull was introduced only in 2015.
Ubuntu Precise 12.04 - is running 2009
Ubuntu Trusty 14.04 - is running 2012
Ubuntu Xenial 16.04 - is running 2015.2
Homebrew (OS X) - is running 2015.2 (no surprises there :p)
Windows - ~~@UnaNancyOwen which version of QHull are you bundling in your All-In-One?~~ is being bundled with 2015.2 in the all in one

I also cant' find ppas with backport for Precise and Trust :/

UnaNancyOwen · 2017-07-06T12:18:50Z

@SergioRAgostinho The all-in-one installer for Windows bundles QHull 2015.2.

jspricke · 2017-07-06T12:34:45Z

I can try to do backports as ppa this evening.

SergioRAgostinho · 2017-07-06T14:36:51Z

Thanks for that Jochen. But even so, are we committing to stop supporting qhull prior to 2015?

jspricke · 2017-07-06T21:19:59Z

I made some for precise i386 and trusty: https://launchpad.net/~v-launchpad-jochen-sprickerhof-de/+archive/ubuntu/pcl18. precise amd64 is failing.

Also I'm fine with updateing to the newer qhull version

taketwo · 2017-07-07T07:57:04Z

But even so, are we committing to stop supporting qhull prior to 2015?

This is a tough one. We do not have an official baseline for our dependencies and we do not have a clear procedure how to lift it, if needed. We do not even have a "mission" statement that would make clear what is of more priority for the library, backward compatibility or new features. (As a matter of fact, I remember sometime back in 2013 when Radu was still active, he was against requiring C++11 in any foreseeable future.)

SergioRAgostinho · 2017-07-07T20:04:40Z

We do not even have a "mission" statement that would make clear what is of more priority for the library, backward compatibility or new features.

Given the relatively lax constraints we have in terms of API changes, I would implicitly assume it's more focused towards new features.

Well it's gonna be a new PCL version so why not. Canonical already dropped support for 12.04 and we can provide a backport for 14.04 so...

I remember sometime back in 2013 when Radu was still active, he was against requiring C++11 in any foreseeable future.

So basically the c++11 adoption idea is to optionally provide support for it, but not require it. That's gonna be a whole lotta work 😅

jspricke · 2017-07-09T05:54:58Z

I think the C++ standard needs a little more consideration then a library version.
For qhull, It's on 2015.2 for the mayor Linux distributions since one or two versions, so for me it's fine to update.

Regarding C++11, GCC6 changed to C++14 by default, so that should be fine with most of Linux. For Visual Studio I only found a blog post stating that they support a combined mode included C++11. For Apple, I was not able to find reliable information. Apart from that I don't see a big problem with allowing C++11 features.

SergioRAgostinho · 2017-07-09T13:09:49Z

For Apple, I was not able to find reliable information

Based on these two:
Homebrew/legacy-homebrew#22453 (comment)
https://trac.macports.org/wiki/XcodeVersionInfo

OS X should not be a problem.

jspricke · 2017-07-09T13:12:14Z

Fine with me, although they only talk about C++11 support, not if it's enabled by default ;).

SergioRAgostinho · 2017-07-09T13:13:24Z

It's not. I didn't understand that was the the core argument 😅

taketwo · 2017-07-09T14:06:35Z

So the consensus is that we review/merge this, effectively raising the QHull dependency version starting from 1.9?

jspricke · 2017-07-09T14:07:43Z

Fine with me.

SergioRAgostinho · 2017-07-09T15:32:55Z

Fine by me as well.

SergioRAgostinho · 2018-01-26T10:57:20Z

Hey. Sorry for getting back to you so late. I started looking at this today but I'm facing weird problem in which github is not loading the diff of surface/include/pcl/surface/impl/concave_hull.hpp. Since it's a large one it doesn't load it by default as usual, but this time even when I click it's not loading at all.

Does anyone else have the same problem?

taketwo · 2018-01-26T11:07:30Z

I have the same problem with FF, but with Chromium it works.

SergioRAgostinho

Sorry for taking long to get back to you.

I never used QHull so my feedback is fairly limited.

I have two overall comment which need to be revised and addressed in multiple points:

The first one is regarding the use of int types for storing ids and indexes. By default you should use size_t, unless QHull/PCL actually places a requirement on using another type.
The second one is a request to avoid using the inner PointCloud::points directly. You have a pendent PR in our issue tracker which hides the access to this member, which I personally agree with. Therefore since the PointCloud container provides all resizing and indexing operations you need, I would ask to stick to using those.

SergioRAgostinho · 2018-01-26T09:37:47Z

cmake/Modules/FindQhull.cmake

-else(QHULL_USE_STATIC)
-  set(QHULL_RELEASE_NAME qhull_p qhull${QHULL_MAJOR_VERSION} qhull)
-  set(QHULL_DEBUG_NAME qhull_pd qhull${QHULL_MAJOR_VERSION}_d qhull_d${QHULL_MAJOR_VERSION} qhull_d)
-endif(QHULL_USE_STATIC)


Purrfect 👍

SergioRAgostinho · 2018-01-26T10:01:09Z

surface/include/pcl/surface/qhull.h

+#include "libqhullcpp/QhullFacetList.h"
+#include "libqhullcpp/QhullVertexSet.h"
+#include "libqhullcpp/QhullRidge.h"
+#include "libqhullcpp/QhullError.h"


Since you're already changing this part modify the includes to use < >. Qhull doesn't reside in our build tree.

SergioRAgostinho · 2018-01-26T10:02:28Z

surface/include/pcl/surface/impl/convex_hull.hpp

+    for(orgQhull::QhullFacetList::iterator it = facets.begin (); it != facets.end (); ++it)
    {
-      polygons[dd].vertices.resize (3);
+       polygons[dd].vertices.resize (3);


This one slipped in unnoticed. It's breaking indentation with rest of the code.

SergioRAgostinho · 2018-01-26T10:14:06Z

surface/include/pcl/surface/impl/convex_hull.hpp

  hull_indices_.indices.reserve (num_vertices);

-  FORALLvertices
+  int i = 0;


This feels like it should be a size_t type.

It should be a uint32_t because qhid_to_pcidx the gets used for filling up polygons[dd].vertices that is a vector of uint32_t

SergioRAgostinho · 2018-01-26T10:39:20Z

surface/include/pcl/surface/impl/convex_hull.hpp

-
-  int num_vertices = qh num_vertices;
+  int num_facets = qhull.facetCount();
+  int num_vertices = qhull.vertexCount();


These feel like they should be a size_t type.

same countT issue

SergioRAgostinho · 2018-01-28T10:56:06Z

surface/include/pcl/surface/impl/concave_hull.hpp

    if (dim_ > 2)
-      points[i * dim_ + 2] = static_cast<coordT> (cloud_transformed.points[i].z);
+      points.push_back(cloud_transformed.points[i].z);
  }


Seems like you can preallocate points from the beginning.

SergioRAgostinho · 2018-01-28T11:03:21Z

surface/include/pcl/surface/impl/concave_hull.hpp

+
+    qh_setvoronoi_all(qhull.qh());
+
+    int num_vertices = qhull.vertexCount();


qhull.vertexCount uses countT that is an int

SergioRAgostinho · 2018-01-28T11:04:23Z

surface/include/pcl/surface/impl/concave_hull.hpp

+    alpha_shape.points.resize (num_vertices);
+
+    // Max vertex id
+    int max_vertex_id = 0;


Is Vertex->id() returning int?

yes, if you check the qhull headers they use countT and that countT is defined as an int.

fran6co · 2018-01-28T13:58:52Z

@SergioRAgostinho About using PointCloud::points directly, I haven't made the change in this PR because I felt it would make the reviewing harder as it's not directly related to the current issue. I can make another PR after this one gets merged or add another commit to this one that fixes the PointCloud::points use in this files.

SergioRAgostinho

I don't have anything else from my side then. :)

I think it's now a matter of deciding when to apply this given the ubuntu issue with qhull packaging.

barcode · 2019-08-13T14:14:56Z

What is the status of this?
As far as i can see, the qhullcpp lib is missing for the most recent Ubuntu LTS (18) and this is stopping the PR from being merged. Is this correct, or are there different problems?

Would you be interested in an implementation with the qhull_r c-library?
It could be used as a solution of the non parallel issue until qhullcpp is available on the primary target systems.
I have a version using qhull_r and could create a PR. I also could change it to conditionally use the _r version if it is available.

stale · 2020-02-22T08:22:39Z

This pull request has been automatically marked as stale because it hasn't had
any activity in the past 60 days. Commenting or adding a new commit to the
pull request will revert this.

Come back whenever you have time. We look forward to your contribution.

kunaltyagi · 2020-06-13T21:07:07Z

@PointCloudLibrary/maintainers What's the status on this? The diff appears to have been approved.

With 16.04 going EOL soon, and bionic distributing libqhull-r (link), we can effectively set a merge-by date for this PR

If we need the cpp lib, then it's available in groovy only

kunaltyagi · 2021-07-28T00:48:47Z

@larshg @mvieth Let's close this?

mvieth · 2021-07-28T12:42:34Z

Let's close this?

Since the other pull request was merged, there is now support for multithreading via the re-entrant interface of QHull. Not the C++ interface, but I don't see a major advantage in using that. So yes, I am ok with closing this.

fran6co force-pushed the qhull branch 3 times, most recently from 331d309 to 7ffbf6c Compare March 7, 2016 22:51

fran6co force-pushed the qhull branch 4 times, most recently from 73be4dd to ebd3468 Compare March 8, 2016 15:51

fran6co force-pushed the qhull branch from ebd3468 to 5c1baa8 Compare March 8, 2016 16:14

fran6co mentioned this pull request Mar 8, 2016

Convex and concave surfaces wrong principal component analysis #1567

Merged

SergioRAgostinho added this to the pcl-1.8.1 milestone Jul 14, 2016

SergioRAgostinho added the needs: code review Specify why not closed/merged yet label Jul 14, 2016

SergioRAgostinho modified the milestones: pcl-1.9.0, pcl-1.8.1 Aug 19, 2016

SergioRAgostinho mentioned this pull request Jul 17, 2017

Moving to C++14 #1638

Closed

3 tasks

fran6co force-pushed the qhull branch 2 times, most recently from 1f78dae to 6760cee Compare December 11, 2017 14:28

taketwo mentioned this pull request Dec 15, 2017

Use reentrant interface of qhull (2015+) when available #2154

Closed

SergioRAgostinho reviewed Jan 28, 2018

View reviewed changes

SergioRAgostinho added needs: author reply Specify why not closed/merged yet and removed needs: code review Specify why not closed/merged yet labels Jan 28, 2018

fran6co force-pushed the qhull branch from 6760cee to f12bad1 Compare January 28, 2018 13:51

Updates Qhull to 2015.2 using the C++ interface

113a268

fran6co force-pushed the qhull branch from f12bad1 to 113a268 Compare January 28, 2018 13:52

SergioRAgostinho approved these changes Jan 28, 2018

View reviewed changes

SergioRAgostinho added status: needs decision and removed needs: author reply Specify why not closed/merged yet labels Jan 28, 2018

taketwo mentioned this pull request Feb 23, 2018

pcl::ConvexHull::reconstruct() not thread safe - qh_qh already defined #1073

Closed

kunaltyagi added kind: proposal Type of issue and removed status: needs decision labels Feb 22, 2020

stale bot added the status: stale label Feb 22, 2020

stale bot removed the status: stale label Jun 13, 2020

kunaltyagi added the needs: feedback Specify why not closed/merged yet label Jun 13, 2020

tkircher mentioned this pull request Nov 18, 2020

PCL can't build with qhull 8.0.2 #4517

Closed

al-tu mentioned this pull request Dec 10, 2020

Add support for reentrant qhull #4540

Merged

kunaltyagi added the status: stale label Jul 28, 2021

kunaltyagi closed this Jul 28, 2021


		qh_setvoronoi_all(qhull.qh());

		int num_vertices = qhull.vertexCount();

Uh oh!

Conversation

fran6co commented Mar 6, 2016 • edited by kunaltyagi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fran6co commented Mar 7, 2016

Uh oh!

fran6co commented Mar 8, 2016

Uh oh!

SergioRAgostinho commented Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UnaNancyOwen commented Jul 6, 2017

Uh oh!

jspricke commented Jul 6, 2017

Uh oh!

SergioRAgostinho commented Jul 6, 2017

Uh oh!

jspricke commented Jul 6, 2017

Uh oh!

taketwo commented Jul 7, 2017

Uh oh!

SergioRAgostinho commented Jul 7, 2017

Uh oh!

jspricke commented Jul 9, 2017

Uh oh!

SergioRAgostinho commented Jul 9, 2017

Uh oh!

jspricke commented Jul 9, 2017

Uh oh!

SergioRAgostinho commented Jul 9, 2017

Uh oh!

taketwo commented Jul 9, 2017

Uh oh!

jspricke commented Jul 9, 2017 via email

Uh oh!

SergioRAgostinho commented Jul 9, 2017

Uh oh!

SergioRAgostinho commented Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taketwo commented Jan 26, 2018

Uh oh!

SergioRAgostinho left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fran6co commented Jan 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SergioRAgostinho left a comment

Choose a reason for hiding this comment

Uh oh!

barcode commented Aug 13, 2019

Uh oh!

stale bot commented Feb 22, 2020

fran6co commented Mar 6, 2016 •

edited by kunaltyagi

Loading

SergioRAgostinho commented Jul 6, 2017 •

edited

Loading

SergioRAgostinho commented Jan 26, 2018 •

edited

Loading

SergioRAgostinho left a comment •

edited

Loading

fran6co commented Jan 28, 2018 •

edited

Loading

kunaltyagi commented Jun 13, 2020 •

edited

Loading