implementation of ball pivoting for surface reconstruction #1950

t-lou · 2017-07-26T00:07:39Z

algorithm from

@article{bernardini1999ball,
   title={The ball-pivoting algorithm for surface reconstruction},
   author={Bernardini, Fausto and Mittleman, Joshua and Rushmeier, Holly and Silva, Cl{\'a}udio and Taubin, Gabriel},
   journal={IEEE transactions on visualization and computer graphics},
   volume={5},
   number={4},
   pages={349--359},
   year={1999},
   publisher={IEEE}
 }

general structure from https://github.com/rodschulz/BPA
fitted from https://github.com/t-lou/bpa_remake

example

input point cloud

output surface

SergioRAgostinho · 2017-07-26T00:33:16Z

Thanks for contributing this. It looks pretty neat 👍

Some preliminary comments:

You haven't submitted any unit tests. How do you know your implementation is doing exactly what it's supposed to? The reconstructed image you posted is definitely a good sign but the question is still valid. I'm bringing this up because we need a mechanism to ensure us that if we have to modify your implementation we're not breaking it's intended functionality, constraints, assumptions, etc...
Can provide you submit also an example? Something like the images you posted, if possible with pointclouds we already have in repo's test folder.
Can you also submit a tutorial explaining a little bit what's the primer behind this method? What are its strong points? What made you use it/implement it/need it instead of the other surface reconstruction methods we provide for instance?

t-lou · 2017-07-26T07:47:25Z

I'll do unit test soon.

But where should I write the example and tutorial?

taketwo · 2017-07-26T07:54:24Z

All our tutorials are contained in this same repository under the "doc" directory. You can take any of the existing tutorials as an example. But I would say let's first concentrate on adding unit tests, reviewing and merging this. The tutorial can be added in a separate PR later on.

Unfortunately I don't have any time today/tomorrow, but I will go through the code and leave comments later this week.

t-lou · 2017-07-26T20:57:53Z

The unit test I wrote failed. But it is successful locally. Can you help me?

SergioRAgostinho · 2017-07-26T21:26:37Z

Unfortunately there's not much I can do on my side other than run it on a Darwin platform.

You need to think about a synthetic small test case, in which you can theoretically predict the outcome, without having to run the algorithm. You should be able to fully understand what are the algorithm's capabilities and limitations, and come up with test scenarios which exploit success and failure conditions.

Loading the bun.pcd and then performing reconstruction, gives me the impression you're doing the exactly reverse process which is better than not having a unit test, but it's not a good unit test.

My advice is to rewrite the unit test following the guidelines I just gave you.

t-lou · 2017-07-26T22:16:01Z

Do you mean composing a hand-made simple cloud and doing unit test with it?

But aren't many other unit tests done with the reverse way?

SergioRAgostinho · 2017-07-26T22:27:50Z

Do you mean composing a hand-made simple cloud and doing unit test with it?

Exactly

But aren't many other unit tests done with the reverse way?

In our repo? Definitely. But it doesn't necessarily make them good unit tests. :)

taketwo

Thanks for contributing. I think we will need several rounds of reviews to get this merged.

My first request will be on the structural side of things. The Edge and Front classes are currently members of the BPA class and, as such, are implicitly templated on the point type. However, their implementation is virtually independent of the point type. Only exception is the Edge::center_ member field of type PointNT. However, only the XYZ coordinates of it are used, so it can be replaced with Eigen::Vector3f.

With this is mind, I'd propose to strip these classes off from BPA. They can remain in the same header file, maybe renamed to BallPivotingEdge and BallPivotingFront to make their scope clear. The implementation can be moved to the "cpp" file because it is template-free.

This change will decrease compilation time and the binary size, because useless instantiations of virtually the same classes will be avoided.

taketwo · 2017-07-27T16:19:51Z

surface/include/pcl/surface/ball_pivoting.h

+#define PCL_BALL_PIVOTING_H_
+
+/**
+ * This class is an implementation of ball pivoting algorithm with some different details


This Doxygen comment should be moved to be directly above the class declaration.
We typically have just authors/title/journal instead of a full BiBTeX entry, i.e. in this case

Bernardini F., Mittleman J., Rushmeier H., Silva C., Taubin G., "The ball-pivoting algorithm for surface reconstruction", TVCG'99

Also, please add \ingroup surface tag.
Finally, it would be great to have a sentence or two with high-level usage guideline. For example (just copied from the paper, assume these claims are actually true):

BPA exhibits linear time complexity and robustness to noise in real scanned data. The output mesh is a manifold of an alpha-shape of the point set.
BPA requires that the points are distributed over the entire surface with a spatial frequency greater than or equal to a certain minimum value. An estimate of surface normal should be available for every point.

…tions

taketwo

I think that Edge should be a plain data struct. In its current form it has no "business logic", i.e. methods besides from member field accessors. And the accessors are trivial, there are no invariants to be maintained (besides from the size of id_vertices_ vector, which as I commented, should be just an array).

taketwo · 2017-07-30T10:24:25Z

surface/CMakeLists.txt

@@ -121,6 +122,7 @@ if(build)
        "include/pcl/${SUBSYS_NAME}/marching_cubes.h"
        "include/pcl/${SUBSYS_NAME}/marching_cubes_hoppe.h"
        "include/pcl/${SUBSYS_NAME}/marching_cubes_rbf.h"
+        "include/pcl/${SUBSYS_NAME}/ball_pivoting.h"


Don't forget that the header with helper structures should also be installed.

Is this issue still open?

No, it's not an issue, I see you added "ball_pivoting_front.h", thanks.

taketwo · 2017-07-30T10:24:38Z

surface/include/pcl/surface/ball_pivoting.h

+ * Software License Agreement (BSD License)
+ *
+ *  Point Cloud Library (PCL) - www.pointclouds.org
+ *  Copyright (c) 2014-, Open Perception, Inc.


is it necessary? not the starting time of the license?

This is a brand new file, created in 2017. You can not claim copyright for the time before it existed!

taketwo · 2017-07-30T10:25:36Z

surface/include/pcl/surface/ball_pivoting.h

+#ifndef PCL_BALL_PIVOTING_H_
+#define PCL_BALL_PIVOTING_H_
+
+#include <memory>


Isn't this a c++11 header? Why do we need it?

for pair, not right?

No, std::pair comes from <utility> header. But you don't need to include it explicitly, for example map already has it.

taketwo · 2017-07-30T10:27:29Z

surface/include/pcl/surface/ball_pivoting.h

+
+#include <memory>
+#include <vector>
+#include <map>


map and set includes should be in the "ball_pivoting_front.h", that's where they are needed.

taketwo · 2017-07-30T10:27:45Z

surface/include/pcl/surface/ball_pivoting_front.h

+ * Software License Agreement (BSD License)
+ *
+ *  Point Cloud Library (PCL) - www.pointclouds.org
+ *  Copyright (c) 2014-, Open Perception, Inc.


taketwo · 2017-07-30T10:33:33Z

surface/include/pcl/surface/ball_pivoting_front.h

+         * returns the center of ball
+         * @return
+         */
+        Eigen::Vector3f


Better return by const reference to avoid copies.

taketwo · 2017-07-30T10:35:26Z

surface/include/pcl/surface/ball_pivoting_front.h

+      class Edge
+      {
+      protected:
+        /** list of vertice index, should have length 2 */


If this list should have a fixed length, why not using good ol' array? I.e. uint32_t id_vertices_[2];

taketwo · 2017-07-30T10:38:40Z

surface/include/pcl/surface/ball_pivoting_front.h

+        Edge ();
+
+        /**
+         * a fake constructor with only vertice index on edge


I'm not sure if "fake" is a good terminology, it's not clear what is implied. What is the state of the other variables? Is an edge constructed this way valid?

taketwo · 2017-07-30T10:39:42Z

surface/include/pcl/surface/ball_pivoting_front.h

+         * @return
+         */
+        uint32_t
+        getIdVertice (const size_t id) const


Should be getIdVertex

taketwo · 2017-07-30T18:47:07Z

Speaking of the Edge structure, since it's just a data container, I think there is no need to have getter functions (e.g. getCenter, isBackBall, etc.). The member fields can simply be public. Also, there is no need to define an empty destructor.

t-lou · 2017-07-31T06:28:24Z

Buenas mis amigos. Is the code and unit test better now?

taketwo

Thanks for the effort so far, I think we are getting close.
A general comment: we should not have free functions inside the "hpp" file. They are an implementation detail of BPA, but by keeping them free you "leak" them into the rest of PCL. Please turn them into private class methods, maybe static if you want. Also, please follow the PCL style guide regarding the function naming, not snake_case(), but camelCase().

taketwo · 2017-07-31T07:07:56Z

surface/src/ball_pivoting.cpp

+/*
+ * Software License Agreement (BSD License)
+ *
+ *  Copyright (c) 2017, Willow Garage, Inc.


This should be Open Perception

taketwo · 2017-07-31T07:08:19Z

surface/src/ball_pivoting.cpp

+ *     copyright notice, this list of conditions and the following
+ *     disclaimer in the documentation and/or other materials provided
+ *     with the distribution.
+ *   * Neither the name of Willow Garage, Inc. nor the names of its


Here also, no Willow Garage. Please just copy the license from your other files.

taketwo · 2017-07-31T07:15:53Z

surface/src/ball_pivoting.cpp

+} // namespace pcl
+
+// Instantiations of specific point types
+PCL_INSTANTIATE(BallPivoting, (pcl::PointNormal)(pcl::PointXYZRGBNormal)(pcl::PointXYZINormal))


This should be conditionally compiled depending on the PCL_NO_PRECOMPILE macro. Please take this as an example.

Both XYZ and Normal are necessary, which macro should I use? I didn't find "PCL_XYZ_NORMAL_POINT_TYPES" or "PCL_XYZNORMAL_POINT_TYPES"

I did not mean to change the point types, my comment was about conditional compilation. I.e. something like:

#ifndef PCL_NO_PRECOMPILE #include <pcl/point_types.h> #include <pcl/impl/instantiate.hpp> PCL_INSTANTIATE(BallPivoting, (pcl::PointNormal)(pcl::PointXYZRGBNormal)(pcl::PointXYZINormal)) #endif

now there is no problem, i though you meant compilation with different range of types

taketwo · 2017-07-31T07:16:50Z

surface/src/ball_pivoting.cpp

+
+#include <boost/make_shared.hpp>
+
+#include <pcl/impl/instantiate.hpp>


This include should be inside the conditional compilation branch, see my comment in the end of this file.

taketwo · 2017-07-31T07:18:14Z

surface/include/pcl/surface/ball_pivoting.h

+     * @param num_point_in_radius
+     * @param ratio_success
+     */
+    virtual void


Why is this virtual? Do we expect other algorithms deriving from BPA that will want to modify this behavior?

taketwo · 2017-07-31T07:22:12Z

surface/include/pcl/surface/ball_pivoting.h

+     * @param ratio_success
+     */
+    virtual void
+    setEstimatedRadius (const int num_sample_point, const int num_point_in_radius, const float ratio_success);


I think this is not a clear name. What about setSearchRadiusAutomatic()? This will make connection with the setSearchRadius() function, thus making clear for the user that these two functions have the same purpose of setting the search radius. Further, I think we should provide some reasonable default values for the arguments.

taketwo · 2017-07-31T07:25:31Z

surface/include/pcl/surface/impl/ball_pivoting.hpp

+   */
+  template<typename PointNT>
+  double
+  guess_radius (const typename PCLSurfaceBase<PointNT>::KdTreePtr &kdtree,


I do not see a reason to have this as a free function. This simply provides an implementation for setEstimatedRadius() and is only used there.

BTW, these are the default parameters I asked for.

taketwo · 2017-07-31T07:41:38Z

surface/include/pcl/surface/impl/ball_pivoting.hpp

+   * @param point0
+   * @param point1
+   * @param center
+   * @param plane the rotation is along the normal vector of plane


Why do we need to explicitly pass the plane? Isn't it implicitly determined by the three points?

The direction can be inferred in normal vector of the plane, either alpha in one direction or 360-alpha in the other

Hm, I still not sure if I get this. The angle between these two vectors is simply

alpha = acos(vc0.dot(vc1));

And then, based on direction of the plane normal, you want to change it to 2pi - alpha?

taketwo · 2017-08-01T06:11:27Z

surface/include/pcl/surface/ball_pivoting.h

-    virtual void
-    setEstimatedRadius (const int num_sample_point, const int num_point_in_radius, const float ratio_success);
+    void
+    setSearchRadiusAutomatically (const int num_sample_point = 0, 


Can this be a typo? Before when the function was called guess_radius the default was 500.

should not be, now non-positive parameter would be changed to one ratio of point cloud size, 10% or 20% i remember

This definitely needs to be documented. What if instead of number of sample points this function take the fraction of sample points, between 0 and 1? And the default value will be explicitly 0.2?

taketwo · 2017-08-01T06:20:00Z

surface/include/pcl/surface/impl/ball_pivoting.hpp

+   * @param point0
+   * @param point1
+   * @param center
+   * @param plane the rotation is along the normal vector of plane


Hm, I still not sure if I get this. The angle between these two vectors is simply

alpha = acos(vc0.dot(vc1));

And then, based on direction of the plane normal, you want to change it to 2pi - alpha?

…archRadiusAutomatically's parameters, rectification for rate_success

t-lou · 2018-01-16T21:26:03Z

Hi, sorry for the miss-operations. After some reading, I think geometry.h is better as a container for the first two function: plane in between and circle centre, how do you think? I can also correct this odd thing, that the functions are defined in .h file.

Maybe also the case that angle.hpp should be a cpp file, as there is no template. How do you like?

t-lou · 2018-04-23T19:27:54Z

Hello, the CI seems to fail because time went out, what could I do about it? Thx :)

SergioRAgostinho · 2018-05-02T15:33:21Z

Rebase with master. The tests were split in two jobs IIRC and you should no longer have timeouts.

t-lou · 2018-05-05T11:05:51Z

Hello, thank you for your suggestion, it looks like the VTK cannot be found.. What should I do?

stale · 2020-02-21T19:43:41Z

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.

fitted bpa to pcl class

6d7eef5

SergioRAgostinho added this to the pcl-1.9.0 milestone Jul 26, 2017

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

SergioRAgostinho added the needs: author reply Specify why not closed/merged yet label Jul 26, 2017

taketwo self-assigned this Jul 26, 2017

Tongxi Lou added 2 commits July 26, 2017 15:59

unit test for ball pivoting

a91882b

avoid unnecessary test on point position

1a7d393

taketwo reviewed Jul 27, 2017

View reviewed changes

Tongxi Lou added 3 commits July 29, 2017 19:12

comment and tag

36d9ceb

move front and edge out of BallPivoting, remove some unnecessary func…

7478a66

…tions

correct bad naming of classes and namespace. comment unit test for now

463bc2c

taketwo reviewed Jul 30, 2017

View reviewed changes

Tongxi Lou added 2 commits July 30, 2017 17:07

fix an error from yesterday. try test which works locally again

19cab1d

some suggested changes, incl defining Edge as struct

4b240ed

Tongxi Lou added 3 commits July 30, 2017 21:15

try custom point cloud

42c4e55

remove getters from Front::Edge

47ece16

clean unit test and fix one logic error

734c80a

taketwo reviewed Jul 31, 2017

View reviewed changes

Tongxi Lou added 2 commits July 31, 2017 11:33

move free functions to member functions

0ddcffd

conditional compiltion

e268df8

taketwo reviewed Aug 1, 2017

View reviewed changes

Tongxi Lou added 6 commits January 16, 2018 22:03

try custom point cloud

98d3d1e

remove getters from Front::Edge

dbe2239

clean unit test and fix one logic error

2530b40

move free functions to member functions

fb681ed

conditional compiltion

d1331de

comment for math for calculation of rotation angle. comment for setSe…

0582a91

…archRadiusAutomatically's parameters, rectification for rate_success

t-lou closed this Jan 16, 2018

t-lou deleted the bpa branch January 16, 2018 21:06

t-lou restored the bpa branch January 16, 2018 21:06

t-lou reopened this Jan 16, 2018

tongxi.lou added 2 commits January 16, 2018 22:40

Merge branch 'bpa' of github.com:t-lou/pcl into bpa

af907b1

move non-template functions to common

2ac4938

t-lou force-pushed the bpa branch from cbba7bb to 2ac4938 Compare January 23, 2018 06:55

SergioRAgostinho added the module: surface label Feb 14, 2018

SergioRAgostinho self-assigned this Feb 14, 2018

Merge branch 'master' into bpa

3003d77

t-lou added 2 commits June 18, 2018 08:02

Merge branch 'master' into bpa

a4a0a8a

Merge remote-tracking branch 'upstream/master' into bpa

f5eb4e8

SergioRAgostinho removed this from the pcl-1.9.0 milestone Aug 26, 2018

t-lou added 3 commits September 8, 2018 13:37

Merge remote-tracking branch 'upstream/master' into bpa

f1b0d6c

Merge branch 'master' into bpa

19e6b7d

merge conflict

c2b8906

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

larshg added the changelog: new feature Meta-information for changelog generation label May 30, 2023

stale bot removed the status: stale label May 30, 2023


		#include <boost/make_shared.hpp>

		#include <pcl/impl/instantiate.hpp>

implementation of ball pivoting for surface reconstruction #1950

Are you sure you want to change the base?

implementation of ball pivoting for surface reconstruction #1950

Conversation

t-lou commented Jul 26, 2017 • edited by SergioRAgostinho

SergioRAgostinho commented Jul 26, 2017

t-lou commented Jul 26, 2017

taketwo commented Jul 26, 2017

t-lou commented Jul 26, 2017

SergioRAgostinho commented Jul 26, 2017

t-lou commented Jul 26, 2017

SergioRAgostinho commented Jul 26, 2017

taketwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taketwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taketwo commented Jul 30, 2017

t-lou commented Jul 31, 2017 • edited

taketwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-lou commented Jan 16, 2018 • edited

t-lou commented Apr 23, 2018

SergioRAgostinho commented May 2, 2018

t-lou commented May 5, 2018

stale bot commented Feb 21, 2020

t-lou commented Jul 26, 2017 •

edited by SergioRAgostinho

t-lou commented Jul 31, 2017 •

edited

t-lou commented Jan 16, 2018 •

edited