Avoid phantom surfces in marching cubes hoppe #1874

t-lou · 2017-05-30T12:20:35Z

… in complex point clouds.

For indoor reconstruction, the MC Hoppe algorithm tends to form significant phantom surfaces where it should be concave. It is likely to be caused by signed distance functions(SDF) generated by far away points. Thus a spatial threshold is added in my branch to ignore SDFs which are far from points. The structure of marching cube is also optimized(separate common parts)

… in complex point clouds. basically only build surface in cubes near points.

SergioRAgostinho · 2017-05-30T13:00:09Z

Can you add some images exemplifying the visual differences of the method before your changes and after?

Thanks for contributing.

t-lou · 2017-06-02T13:00:49Z

Point cloud

Reconstruction with pcl-1.8

Reconstruction also with spatial restriction

SergioRAgostinho · 2017-06-29T11:52:26Z

surface/include/pcl/surface/impl/marching_cubes.hpp

  // Create grid
-  grid_ = std::vector<float> (res_x_*res_y_*res_z_, 0.0f);
+  // grid_ = std::vector<float> (res_x_*res_y_*res_z_, 0.0f);


Don't submit commented lines

SergioRAgostinho · 2017-06-29T12:03:18Z

surface/include/pcl/surface/impl/marching_cubes.hpp

+      v.vertices[j] = static_cast<int> (i) * 3 + j;
+    polygons[i] = v;
+  }
+}


This section can also be refactored into performReconstructionProc. It will required you to change the signature of that method to include the pointcloud and the polygon vector.

SergioRAgostinho · 2017-06-29T12:03:39Z

surface/include/pcl/surface/impl/marching_cubes.hpp

+}
+
+template <typename PointNT> void
+pcl::MarchingCubes<PointNT>::performReconstructionProc ()


See comment above regarding the signature for this one.

SergioRAgostinho · 2017-06-29T12:06:17Z

surface/include/pcl/surface/impl/marching_cubes.hpp

-  voxelizeData ();
-
+  // real part of marching cube
+  performReconstructionProc();


See my comment below regarding the signature change for this block and the code that can also be included inside.

SergioRAgostinho · 2017-06-29T12:07:04Z

surface/include/pcl/surface/impl/marching_cubes.hpp

-    for (int j = 0; j < 3; ++j)
-      v.vertices[j] = static_cast<int> (i) * 3 + j;
-    polygons[i] = v;
-  }


In conjunction with my other comments, leave this here.

SergioRAgostinho · 2017-06-29T12:14:24Z

surface/include/pcl/surface/impl/marching_cubes_hoppe.hpp

+  std::vector<int> nn_indices(1, 0);
+  std::vector<float> nn_sqr_dists(1, 0.0f);
+  Eigen::Vector3f point;
+  PointNT p;


Comment regarding the two lines above: I'm an apologist of RAII. Does this grant you a super duper performance increase to justify declaring it here?

SergioRAgostinho · 2017-06-29T12:16:10Z

surface/include/pcl/surface/impl/marching_cubes_hoppe.hpp

+  std::vector<float> nn_sqr_dists(1, 0.0f);
+  Eigen::Vector3f point;
+  PointNT p;
+  const Eigen::Vector4f delta = ( max_p_ - min_p_ ).cwiseQuotient( Eigen::Vector4f(


I don't really understand the reason behind declaring this as Vector4f. Was it to make it aligned or something? Note that if you declare it Vector3f (use actually Array3f to use element wise operations later on) you can reduce the expression below, inside the nested for blocks.

Edit: Now I'm definitely sure use you want to perform element wise operations. Read on Eigen about Arrays and block. Create delta as an Array3f for the three top element block of max_p_ min_p_.

SergioRAgostinho · 2017-06-29T12:29:11Z

surface/include/pcl/surface/impl/marching_cubes_hoppe.hpp

-            point - input_->points[nn_indices[0]].getVector3fMap ());
+        if( !is_far_ignored || nn_sqr_dists[0] < dist_ignore_ )
+        {
+          normal = input_->points[nn_indices[0]].getNormalVector3fMap ();


SergioRAgostinho · 2017-06-29T12:31:35Z

surface/include/pcl/surface/marching_cubes.h

+
+      /** \brief the point cloud really generated from Marching Cubes
+       */
+      pcl::PointCloud<PointNT> intermediate_cloud_;


With the changes requested I think this member can be removed and just exist in a temporary scope inside the methods.

t-lou · 2017-06-29T14:41:16Z

Hello, thank you for the review. Should I correct in another commit or collide to this one? Would further commits be added automatically in this pull-request? Thank you

taketwo · 2017-06-29T15:03:57Z

Yes, add more commits to the same branch and push it again, this PR will be updated automatically. In the end, when the review is done, we will squash everything and write a single clean commit to the main repository.

t-lou · 2017-07-03T06:25:22Z

How about now?

taketwo

General comment: please follow the PCL style guide with respect to the spaces before braces.

taketwo · 2017-07-03T06:46:30Z

surface/include/pcl/surface/impl/marching_cubes_hoppe.hpp

+        {
+          const Eigen::Vector3f normal = input_->points[nn_indices[0]].getNormalVector3fMap ();
+
+          if(normal.norm() > 0.5f)


Is this necessary? I thought PCL maintains the invariant normal.norm() == 1.0.

In some point clouds I used (maybe not generated with pcl, but with quite fine normal), some points have normal vector with length 0. Would the normal be [0] if the neighborhood is not consistent?

Hm, at least in NormalEstimation class invalid normals are set to NaN (see here). I wonder how norm() function reacts to that.

taketwo · 2017-07-03T06:47:06Z

surface/include/pcl/surface/impl/marching_cubes_hoppe.hpp

+
+  const Eigen::Vector3f min_p = min_p_.segment(0, 3);
+  const Eigen::Vector3f delta = ( max_p_ - min_p_ ).segment(0, 3).cwiseQuotient(
+        Eigen::Vector3f((float)res_x_, (float)res_y_, (float)res_z_) );


Is explicit cast necessary?

taketwo · 2017-07-03T06:48:42Z

surface/include/pcl/surface/impl/marching_cubes.hpp

+  intermediate_cloud.clear();
+
+  // preallocate memory assuming a hull. suppose 6 point per voxel
+  intermediate_cloud.reserve((size_t)(res_y_*res_z_ + res_x_*res_z_ + res_x_*res_y_) * 2 * 6);


This is potentially a huge number. Do we have a check for overflow somewhere?

I wanted to avoid massive copy of vector. How to check overflow? Could you show me a file with this function? (in voxel grid I have seen similar output, but for overflow of uint32 or int32)

I meant that this product may exceed what size_t can represent. In this case we can actually get some small number as a result and reserve too less. I'd propose to perform this multiplication in double, then clamp to intermediate_cloud.max_size(), and finally reserve.

taketwo · 2017-07-03T06:51:09Z

surface/include/pcl/surface/impl/marching_cubes.hpp

@@ -45,7 +45,7 @@
 //////////////////////////////////////////////////////////////////////////////////////////////
 template <typename PointNT>
 pcl::MarchingCubes<PointNT>::MarchingCubes () 
-: min_p_ (), max_p_ (), percentage_extend_grid_ (), iso_level_ ()
+: min_p_ (), max_p_ (), percentage_extend_grid_ (), iso_level_ (), dist_ignore_ (-1.0f)


Actually, in PCL we often have such simple constructor definitions right in the header file, motivation being that it's easier to see the default values. While we are at it, would you mind to transfer this to the header and also fill in the values of min_p_, max_p_, etc for clarity?

Move the whole constructor into header file? Maybe I could move min_p_ and max_p_ to local variable and change to vector3f, it looks a bit stateful to have them as class members. How do you say?

I had a brief look and they are used in different functions both in parent and deriving class. If you have an idea how to make them local without need to recompute several times, please go ahead!

taketwo · 2017-07-03T10:05:23Z

surface/include/pcl/surface/impl/marching_cubes.hpp

+  Eigen::Vector3f size3_extend = 0.5f * percentage_extend_grid_ * (upper_boundary - lower_boundary);
+
+  lower_boundary -= size3_extend;
+  upper_boundary += size3_extend;


Everything below this line has no effect, right? Should we delete these lines?

I have deleted. Is the constructor now what you want?

…unding box. default parameter for mc

SergioRAgostinho

Good work. Now it's just really some minor things and some questions regarding your thoughts.

SergioRAgostinho · 2017-07-03T11:11:02Z

surface/include/pcl/surface/impl/marching_cubes.hpp

@@ -45,7 +45,7 @@
 //////////////////////////////////////////////////////////////////////////////////////////////
 template <typename PointNT>
 pcl::MarchingCubes<PointNT>::MarchingCubes () 
-: min_p_ (), max_p_ (), percentage_extend_grid_ (), iso_level_ ()
+: percentage_extend_grid_ (), iso_level_ (), dist_ignore_ (-1.0f)
 {
 }


Wasn't this supposed to be moved to the .h file?

SergioRAgostinho · 2017-07-03T11:15:59Z

surface/include/pcl/surface/impl/marching_cubes.hpp

 {
-  pcl::getMinMax3D (*input_, min_p_, max_p_);
+  PointNT max_pt, min_pt;
+  pcl::getMinMax3D (*input_, min_pt, max_pt);


It doesn't make much difference in this case but this looks like the overload you were actually looking for
http://docs.pointclouds.org/trunk/group__common.html#gafd9010977f5e52b35b484be7624df3f8

SergioRAgostinho · 2017-07-03T11:32:44Z

surface/include/pcl/surface/impl/marching_cubes.hpp


  // Run the actual marching cubes algorithm, store it into a point cloud,
  // and copy the point cloud + connectivity into output
-  points.clear ();
+  intermediate_cloud.clear ();


Unnecessary. Intermediate_cloud is always initialized empty.

SergioRAgostinho · 2017-07-03T11:37:40Z

surface/include/pcl/surface/impl/marching_cubes.hpp

+  // the point cloud really generated from Marching Cubes, prev intermediate_cloud_
+  pcl::PointCloud<PointNT> intermediate_cloud;
+
+  Eigen::Vector3f upper_boundary, lower_boundary;


I noticed that every operation on these guys is coefficient wise. You should replace their type to Eigen::Array. Have a look at https://eigen.tuxfamily.org/dox/group__TutorialArrayClass.html

SergioRAgostinho · 2017-07-03T11:44:30Z

surface/include/pcl/surface/impl/marching_cubes_rbf.hpp

-        point[0] = min_p_[0] + (max_p_[0] - min_p_[0]) * float (x) / float (res_x_);
-        point[1] = min_p_[1] + (max_p_[1] - min_p_[1]) * float (y) / float (res_y_);
-        point[2] = min_p_[2] + (max_p_[2] - min_p_[2]) * float (z) / float (res_z_);
+        Eigen::Vector3d point = base + delta.cwiseProduct (Eigen::Vector3d (x, y, z));


Const qualifier

SergioRAgostinho · 2017-07-03T12:03:41Z

surface/include/pcl/surface/marching_cubes.h

    protected:
      /** \brief The data structure storing the 3D grid */
      std::vector<float> grid_;

      /** \brief The grid resolution */
      int res_x_, res_y_, res_z_;

-      /** \brief Min and max data points. */
-      Eigen::Vector4f min_p_, max_p_;


Was this (or the bound named equivalent) worth removing? I saw you compute it only once and then have to pass it around to a number of methods. Its lifetime is equivalent to input_, grid_, res_x_, etc... I really don't understand why you felt they were "stateful", or to be more clear, that they shouldn't belong to the state of the Class. Can you explain a little bit the reasoning?

I also question if delta should not follow the same.

I just have another compelling argument: There's not a single time you invoke the methods (the ones which now have the bounds on the signature) and that these bounds actually change. You don't invoke the method changing the bounds ever. As the long as the input cloud is the same, the bounds will always be the same.

Because I think it's not the property of the class, but of the data. And using them as parameter doesn't make the program too complicated or slow.

Sorry, what do you mean by "if delta should not follow the same"?

Do you mean to do marching cube meshing on the same input cloud for several times with different parameters? But the old code would also invoke "getBoundingBox ()" each time we execute "performReconstruction"

SergioRAgostinho · 2017-07-03T12:06:32Z

surface/include/pcl/surface/impl/marching_cubes.hpp

-  center[0] = min_p_[0] + (max_p_[0] - min_p_[0]) * float (index_3d[0]) / float (res_x_);
-  center[1] = min_p_[1] + (max_p_[1] - min_p_[1]) * float (index_3d[1]) / float (res_y_);
-  center[2] = min_p_[2] + (max_p_[2] - min_p_[2]) * float (index_3d[2]) / float (res_z_);
+  const Eigen::Vector3f size_voxel = (upper_boundary - lower_boundary).cwiseQuotient (


This is similar to delta right? Increases the time you compute this to three time.

excuse me, what do you mean?

https://github.com/t-lou/pcl/blob/10fe1eb3e7e720a4a80f8ae3c304a493eeb77785/surface/include/pcl/surface/impl/marching_cubes.hpp#L127-L128

is similar to this

https://github.com/t-lou/pcl/blob/10fe1eb3e7e720a4a80f8ae3c304a493eeb77785/surface/include/pcl/surface/impl/marching_cubes_hoppe.hpp#L66-L67

and this

https://github.com/t-lou/pcl/blob/10fe1eb3e7e720a4a80f8ae3c304a493eeb77785/surface/include/pcl/surface/impl/marching_cubes_rbf.hpp#L103-L104

You're computing the same property three times. This normally hints me that it should become a member of the base class. What's your opinion?

I would prefer to calculate once and use them as parameters, as they are data-related, not algorithm related. In this way the functions would be more stateless.
Do you think I should put delta (voxel_size), min_p_ and max_p_ all to class definition?

Yes. I'm just trying to understand what are the advantages of your approach and the avantages of mine.

The class has data members (i.e. input_ and grid_) so it's only natural to have other members related to this data.

Currently your protected methods have a parameters in the signature that never changes, as long as the input cloud (which is a member) doesn't change, and they're only invoked following a public call to performReconstruction.

If we add the members to the class definition and simply don't expose setters and getter functions to them we have:

More member which are data related but not accessible to the outside

You don't have them in the signature because you don't need them, since you're anyway you're alway invoking the methods with the same bound data.

You prevent have to recompute delta, because it persists valid throughout a call to performReconstruction

Opinion @taketwo

PCL classes are by design stateful. We use separate function calls to set parameters, input point cloud, input indices. After that we call a compute method, passing a single argument: storage for the output. Therefore I agree with @SergioRAgostinho, there is no reason to pretend there is no state and pass around variables that are computed from the input point cloud, which in fact is the state.

SergioRAgostinho · 2017-07-03T12:06:55Z

surface/include/pcl/surface/impl/marching_cubes.hpp

@@ -90,15 +96,17 @@ pcl::MarchingCubes<PointNT>::interpolateEdge (Eigen::Vector3f &p1,
                                              float val_p2,
                                              Eigen::Vector3f &output)
 {
-  float mu = (iso_level_ - val_p1) / (val_p2-val_p1);
+  float mu = (iso_level_ - val_p1) / (val_p2 - val_p1);


SergioRAgostinho · 2017-07-03T12:17:18Z

surface/include/pcl/surface/impl/marching_cubes.hpp

+                                            const float dist_ignore)
+: percentage_extend_grid_ (percentage_extend_grid), 
+  iso_level_ (iso_level), 
+  dist_ignore_ (dist_ignore)
 {
 }


Move this entire constructor to the .h

…er and Eigen::Array3f instead of Eigen::Vector3f

SergioRAgostinho

Really minor comments. Thanks for all the effort and thanks for contributing this.

SergioRAgostinho · 2017-07-04T07:07:51Z

surface/include/pcl/surface/impl/marching_cubes.hpp

+  pcl::getMinMax3D (*input_, min_pt, max_pt);
+
+  lower_boundary_ = min_pt.getVector3fMap ().array ();
+  upper_boundary_ = max_pt.getVector3fMap ().array ();


You should use getArray3fMap() instead.

SergioRAgostinho · 2017-07-04T07:13:55Z

surface/include/pcl/surface/impl/marching_cubes.hpp

-             min_p_.x (), min_p_.y (), min_p_.z ());
-  PCL_DEBUG ("[pcl::MarchingCubesHoppe::getBoundingBox] Upper left point is [%f, %f, %f]\n",
-             max_p_.x (), max_p_.y (), max_p_.z ());
+  PointNT max_pt, min_pt;


You're just interested in the coordinates so this should be PointXYZ.

SergioRAgostinho · 2017-07-04T07:23:22Z

surface/include/pcl/surface/marching_cubes_rbf.h

-      MarchingCubesRBF ();
+      MarchingCubesRBF (const float percentage_extend_grid = 0.0f,
+                        const float iso_level = 0.0f,
+                        const float off_surface_epsilon = 0.1f) :


Since you're more knowledgeable about this algorithm than I am, does the order in which these parameters appear respect the convention of "more likely to be changed" to "less likely to be changed" (from the default values)?

SergioRAgostinho · 2017-07-04T07:25:22Z

surface/include/pcl/surface/marching_cubes_hoppe.h

+      MarchingCubesHoppe (const float percentage_extend_grid = 0.0f,
+                          const float iso_level = 0.0f,
+                          const float dist_ignore = -1.0f) :
+        MarchingCubes<PointNT> (percentage_extend_grid, iso_level),


Since you're more knowledgeable about this algorithm than I am, does the order in which these parameters appear respect the convention of "more likely to be changed" to "less likely to be changed" (from the default values)?

taketwo · 2017-07-04T07:41:03Z

Beside from the latest comments from Sergio, I think everything is fine. Well, actually there are still some problems with spaces between braces, would be nice to fix them before we merge. Thanks for the effort!

Edit: oh, and I'm afraid that the unit test is failing now.

t-lou · 2017-07-04T08:05:43Z

@taketwo
How should I get the information about unit tests? I only did tests in my project, and tried to compile on my computer.
could you give some examples where the spacing is wrong? http://pointclouds.org/documentation/advanced/pcl_style_guide.php#spacing doesn't give a complete description of where to add/delete spaces. Thanks

@SergioRAgostinho
actually I'm not familiar with MC RBF. The author didn't use parameters for constructor, and instead used constant values in initial list. Should I do that too?

taketwo · 2017-07-04T08:08:24Z

Here is a complete log of the failing test (comes from here):

16: Test command: /home/travis/build/PointCloudLibrary/pcl/build/test/surface/test_marching_cubes "/home/travis/build/PointCloudLibrary/pcl/test/bun0.pcd"
16: Test timeout computed to be: 9.99988e+06
16: [==========] Running 1 test from 1 test case.
16: [----------] Global test environment set-up.
16: [----------] 1 test from PCL
16: [ RUN      ] PCL.MarchingCubesTest
16: /home/travis/build/PointCloudLibrary/pcl/test/surface/test_marching_cubes.cpp:79: Failure
16: The difference between points.points[points.size()/2].x and -0.042528 is 0.0053850042550563845, which exceeds 1e-3, where
16: points.points[points.size()/2].x evaluates to -0.037142995744943619,
16: -0.042528 evaluates to -0.042528000000000003, and
16: 1e-3 evaluates to 0.001.
16: /home/travis/build/PointCloudLibrary/pcl/test/surface/test_marching_cubes.cpp:80: Failure
16: The difference between points.points[points.size()/2].y and 0.080196 is 0.018017069140911099, which exceeds 1e-3, where
16: points.points[points.size()/2].y evaluates to 0.098213069140911102,
16: 0.080196 evaluates to 0.080196000000000003, and
16: 1e-3 evaluates to 0.001.
16: /home/travis/build/PointCloudLibrary/pcl/test/surface/test_marching_cubes.cpp:81: Failure
16: The difference between points.points[points.size()/2].z and 0.043159 is 0.088069751283168796, which exceeds 1e-3, where
16: points.points[points.size()/2].z evaluates to -0.044910751283168793,
16: 0.043159 evaluates to 0.043159000000000003, and
16: 1e-3 evaluates to 0.001.
16: /home/travis/build/PointCloudLibrary/pcl/test/surface/test_marching_cubes.cpp:82: Failure
16: Value of: 10854
16: Expected: vertices[vertices.size ()/2].vertices[0]
16: Which is: 11202
16: /home/travis/build/PointCloudLibrary/pcl/test/surface/test_marching_cubes.cpp:83: Failure
16: Value of: 10855
16: Expected: vertices[vertices.size ()/2].vertices[1]
16: Which is: 11203
16: /home/travis/build/PointCloudLibrary/pcl/test/surface/test_marching_cubes.cpp:84: Failure
16: Value of: 10856
16: Expected: vertices[vertices.size ()/2].vertices[2]
16: Which is: 11204
16: /home/travis/build/PointCloudLibrary/pcl/test/surface/test_marching_cubes.cpp:95: Failure
16: The difference between points.points[points.size()/2].x and -0.033919 is 0.0082885054203271843, which exceeds 1e-3, where
16: points.points[points.size()/2].x evaluates to -0.025630494579672813,
16: -0.033919 evaluates to -0.033918999999999998, and
16: 1e-3 evaluates to 0.001.
16: /home/travis/build/PointCloudLibrary/pcl/test/surface/test_marching_cubes.cpp:96: Failure
16: The difference between points.points[points.size()/2].y and 0.151683 is 0.016454813154220593, which exceeds 1e-3, where
16: points.points[points.size()/2].y evaluates to 0.13522818684577942,
16: 0.151683 evaluates to 0.15168300000000001, and
16: 1e-3 evaluates to 0.001.
16: /home/travis/build/PointCloudLibrary/pcl/test/surface/test_marching_cubes.cpp:97: Failure
16: The difference between points.points[points.size()/2].z and -0.000086 is 0.035851919834375384, which exceeds 1e-3, where
16: points.points[points.size()/2].z evaluates to 0.035765919834375381,
16: -0.000086 evaluates to -8.6000000000000003e-05, and
16: 1e-3 evaluates to 0.001.
16: /home/travis/build/PointCloudLibrary/pcl/test/surface/test_marching_cubes.cpp:98: Failure
16: Value of: 4284
16: Expected: vertices[vertices.size ()/2].vertices[0]
16: Which is: 4275
16: /home/travis/build/PointCloudLibrary/pcl/test/surface/test_marching_cubes.cpp:99: Failure
16: Value of: 4285
16: Expected: vertices[vertices.size ()/2].vertices[1]
16: Which is: 4276
16: /home/travis/build/PointCloudLibrary/pcl/test/surface/test_marching_cubes.cpp:100: Failure
16: Value of: 4286
16: Expected: vertices[vertices.size ()/2].vertices[2]
16: Which is: 4277
16: [  FAILED  ] PCL.MarchingCubesTest (331 ms)
16: [----------] 1 test from PCL (331 ms total)
16: 
16: [----------] Global test environment tear-down
16: [==========] 1 test from 1 test case ran. (331 ms total)
16: [  PASSED  ] 0 tests.
16: [  FAILED  ] 1 test, listed below:
16: [  FAILED  ] PCL.MarchingCubesTest
16: 
16:  1 FAILED TEST
16/23 Test #16: surface_marching_cubes ...........***Failed    0.36 sec

taketwo · 2017-07-04T08:09:11Z

surface/include/pcl/surface/impl/marching_cubes.hpp

@@ -191,7 +171,7 @@ template <typename PointNT> void
 pcl::MarchingCubes<PointNT>::getNeighborList1D (std::vector<float> &leaf,
                                                Eigen::Vector3i &index3d)
 {
-  leaf = std::vector<float> (8, 0.0f);
+  leaf.resize(8);


Space between resize and (8)

taketwo · 2017-07-04T08:09:20Z

surface/include/pcl/surface/impl/marching_cubes.hpp

@@ -201,6 +181,15 @@ pcl::MarchingCubes<PointNT>::getNeighborList1D (std::vector<float> &leaf,
  leaf[5] = getGridValue (index3d + Eigen::Vector3i (1, 1, 0));
  leaf[6] = getGridValue (index3d + Eigen::Vector3i (1, 1, 1));
  leaf[7] = getGridValue (index3d + Eigen::Vector3i (0, 1, 1));
+
+  for(int i = 0; i < 8; ++i)


Space after for

taketwo · 2017-07-04T08:09:33Z

surface/include/pcl/surface/impl/marching_cubes.hpp

+
+  for(int i = 0; i < 8; ++i)
+  {
+    if(std::isnan(leaf[i]))


Space after if and isnan

taketwo · 2017-07-04T08:09:42Z

surface/include/pcl/surface/impl/marching_cubes.hpp

+  {
+    if(std::isnan(leaf[i]))
+    {
+      leaf.clear();


Space after clear

taketwo · 2017-07-04T08:10:10Z

surface/include/pcl/surface/impl/marching_cubes.hpp

  for (int x = 1; x < res_x_-1; ++x)
    for (int y = 1; y < res_y_-1; ++y)
      for (int z = 1; z < res_z_-1; ++z)
      {
        Eigen::Vector3i index_3d (x, y, z);
        std::vector<float> leaf_node;
        getNeighborList1D (leaf_node, index_3d);
-        createSurface (leaf_node, index_3d, points);
+        if(!leaf_node.empty())


Space after if and empty

taketwo · 2017-07-04T08:10:46Z

surface/include/pcl/surface/impl/marching_cubes_hoppe.hpp

        p.getVector3fMap () = point;

        tree_->nearestKSearch (p, 1, nn_indices, nn_sqr_dists);

-        grid_[x * res_y_*res_z_ + y * res_z_ + z] = input_->points[nn_indices[0]].getNormalVector3fMap ().dot (
-            point - input_->points[nn_indices[0]].getVector3fMap ());
+        if( !is_far_ignored || nn_sqr_dists[0] < dist_ignore_ )


Space after if, no space inside braces

taketwo · 2017-07-04T08:11:01Z

surface/include/pcl/surface/impl/marching_cubes_hoppe.hpp

+        {
+          const Eigen::Vector3f normal = input_->points[nn_indices[0]].getNormalVector3fMap ();
+
+          if(!std::isnan (normal(0)) && normal.norm () > 0.5f)


Space after if and normal

SergioRAgostinho · 2017-07-04T08:11:32Z

actually I'm not familiar with MC RBF. The author didn't use parameters for constructor, and instead used constant values in initial list. Should I do that too?

Not at all. It's good that you expose those parameters in the constructor. Given the lack of insight then let's always put parameters of the child class first followed by parameters of the base class. For instance, for Hoppe

MarchingCubesHoppe (const float dist_ignore = -1.0f,
                    const float percentage_extend_grid = 0.0f,
                    const float iso_level = 0.0f)
    : [...]

taketwo · 2017-07-04T08:14:37Z

let's always put parameters of the child class first followed by parameters of the base class

What is the motivation, why not the other way round?

SergioRAgostinho · 2017-07-04T08:19:20Z

So keeping in mind that we're not really aware which parameter is actually changed more often, when invoking a child class I normally tend tweak parameters of the child class more often, then the base class. The child class normally tends to set good default values for its Base class constructor.

But then again. Only because we lack experience with the class to have an educated guess.

t-lou · 2017-07-04T08:43:12Z

@taketwo
very likely this is caused by checking whether the normal is correct, which leads to different size of output polygon. Will the unit test be executed each time I push commit?

taketwo · 2017-07-04T08:46:11Z

Yes, tests run after every push on the last commit, but it takes up to 3 hours on Travis :(
It probably makes sense to experiment with them locally. Simply enable BUILD_global_tests option in CMake and then compile and run the test in question.

t-lou · 2017-07-04T09:30:01Z

@taketwo
I have found the problem. The old version expended(the one used for unit test) the bounding box like

lower_boundary_ -= 0.5f * percentage_extend_grid_ * (upper_boundary_ - lower_boundary_);
upper_boundary_ += 0.5f * percentage_extend_grid_ * (upper_boundary_ - lower_boundary_);

while I changed it to
const Eigen::Array3f size3_extend = 0.5f * percentage_extend_grid_ * (upper_boundary_ - lower_boundary_);
lower_boundary_ -= size3_extend;
upper_boundary_ += size3_extend;

When I switched to older way, this module is successful. I think my way is correct. What is your suggestion? Change the code or unit test?

SergioRAgostinho · 2017-07-04T09:37:10Z

Good catch! Change the unit tests!

In the old one the resultant expansion would not correspond to the requested by the user.

… on last commit works, this commit is valid

t-lou · 2017-07-04T11:56:49Z

Should be no problem now :)

taketwo · 2017-07-04T12:45:14Z

surface/include/pcl/surface/marching_cubes_hoppe.h

        */
-      inline void


What happened to inline? Also, getDistanceIgnore can be inline.

t-lou · 2017-07-04T14:57:28Z

I remember too many inline keywords could make other inline functions less optimized. Is it right? I think getter and setter don't need to be inline. Is it a tradition to make smaller functions inline?

t-lou · 2017-07-05T06:33:12Z

Hi, where should I ask before adding new features? I rewrote the implementation of ball pivoting. Does pcl need it? maybe I can fit it as a pcl class. One part of one function was from another repo https://github.com/t-lou/bpa_remake

taketwo · 2017-07-05T06:47:23Z

Is it a tradition to make smaller functions inline?

Yes, this is common in PCL to make all one-liner getter/setter functions inline and put their implementation in the header file. Over time, I've seen multiple discussions of the inline keyword with people claiming that modern compilers ignore it, that it is an implementation detail and should go into cpp file, etc. I'm not a compiler writer and not an expert in compiler optimization. So unless some one comes up with compelling evidence that inlined getter/setter functions in PCL have any negative effect, I'd prefer to just stick with this "convention".

Hi, where should I ask before adding new features? I rewrote the implementation of ball pivoting. Does pcl need it? maybe I can fit it as a pcl class. One part of one function was from another repo

In principle, you can create an issue describing what you want to do. But in this case I think it is an easy "yes". Such an algorithm will nicely fit in the surface module of PCL. One thing we need to be careful is that if you will be re-using some third-party code, it's license should be permissive enough. Even better, get explicit agreement from the original author.

SergioRAgostinho · 2017-07-05T06:48:13Z

I remember too many inline keywords could make other inline functions less optimized. Is it right? I think getter and setter don't need to be inline. Is it a tradition to make smaller functions inline?

That's a very cheap accessor. It won't be an issue.

Hi, where should I ask before adding new features? I rewrote the implementation of ball pivoting. Does pcl need it? maybe I can fit it as a pcl class. One part of one function was from another repo https://github.com/t-lou/bpa_remake

I would say the place for that is the developer mailing list. I have no idea of what ball pivoting is, so I'll need to get acquainted with the topic. What general benefits did your implementation bring?

t-lou · 2017-07-05T07:31:41Z

It is quite like greedy_projection, but slower and in my test case more robust(leaving fewer holes). Is it only necessary to mention the paper where the algorithm was proposed and avoid code from others?

taketwo

Note to self: will need to squash on merge.

taketwo · 2017-07-05T07:58:53Z

Is it only necessary to mention the paper where the algorithm was proposed and avoid code from others?

Reference to the paper is very helpful to have. Code from others is fine as long as it is BSD or MIT I'd say.

SergioRAgostinho

Looks good. Thank you all for your efforts

Final curiosity: from what I can tell the method just makes use of xyz and normal data. Why are actually using PointNT here and there?

SergioRAgostinho · 2017-07-05T12:27:42Z

surface/include/pcl/surface/impl/marching_cubes.hpp

-    p3.x = vertex_list[triTable[cubeindex][i+2]][0];
-    p3.y = vertex_list[triTable[cubeindex][i+2]][1];
-    p3.z = vertex_list[triTable[cubeindex][i+2]][2];
+    p3.getVector3fMap () = vertex_list[triTable[cubeindex][i+2]];
    cloud.push_back (p3);


I just noticed that this cloud is only being populated with XYZ info. Why exactly are we using PointNT?

SergioRAgostinho

Well ignore that... I think I have an idea now why.

Second reminder squash merge

t-lou · 2017-07-05T13:03:07Z

Thank you for your help! I use this library a lot and I'm glad to be able to do something for it :D

PointCloudLibrary#1874) Add spatial restriction for marching cube hoppe to avoid phantom surfaces in complex point clouds

spatial restriction for marching cube hoppe to avoid phantom surfaces…

22a4977

… in complex point clouds. basically only build surface in cubes near points.

SergioRAgostinho added this to the pcl-1.9.0 milestone Jun 29, 2017

SergioRAgostinho requested changes Jun 29, 2017

View reviewed changes

taketwo added the needs: author reply Specify why not closed/merged yet label Jun 29, 2017

requested changes for pr

778adec

taketwo requested changes Jul 3, 2017

View reviewed changes

Tongxi Lou added 2 commits July 3, 2017 10:24

space, reserve of intermediate cloud

97969d6

should have made min_p_ and max_p_ local

10fe1eb

taketwo reviewed Jul 3, 2017

View reviewed changes

remove min_p_ and max_p_ from mc rbf. remove debug info for mc get bo…

0334173

…unding box. default parameter for mc

SergioRAgostinho requested changes Jul 3, 2017

View reviewed changes

Tongxi Lou added 3 commits July 3, 2017 16:50

move constructor to header. set boundary and voxel size to class memb…

e918642

…er and Eigen::Array3f instead of Eigen::Vector3f

remove unnecessary local bounding box

6cc5a6d

clean undeleted comment and cut long statement

13b2a8f

SergioRAgostinho requested changes Jul 4, 2017

View reviewed changes

some minor changes

a5cea53

taketwo reviewed Jul 4, 2017

View reviewed changes

Tongxi Lou added 2 commits July 4, 2017 12:00

fake commit, to make sure bb is the problem

f08107a

undo last commit and fix gtest data for marching cube. if travis test…

96d1eb4

… on last commit works, this commit is valid

taketwo reviewed Jul 4, 2017

View reviewed changes

make getter and setter for dist_ignore inline

3a7390f

taketwo approved these changes Jul 5, 2017

View reviewed changes

SergioRAgostinho reviewed Jul 5, 2017

View reviewed changes

SergioRAgostinho approved these changes Jul 5, 2017

View reviewed changes

SergioRAgostinho added status: ready to merge and removed needs: author reply Specify why not closed/merged yet labels Jul 5, 2017

taketwo merged commit 04fa347 into PointCloudLibrary:master Aug 27, 2017

taketwo removed the status: ready to merge label Aug 27, 2017

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

SergioRAgostinho mentioned this pull request Sep 2, 2018

Change Log generation tool. Automates change log generation. #2396

Merged

taketwo changed the title ~~spatial restriction for marching cube hoppe to avoid phantom surfaces…~~ Avoid phantom surfces in marching cubes hoppe Sep 2, 2018

taketwo added the module: surface label Sep 2, 2018

Avoid phantom surfces in marching cubes hoppe #1874

Avoid phantom surfces in marching cubes hoppe #1874

Conversation

t-lou commented May 30, 2017

SergioRAgostinho commented May 30, 2017

t-lou commented Jun 2, 2017

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

SergioRAgostinho Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-lou commented Jun 29, 2017

taketwo commented Jun 29, 2017 • edited Loading

t-lou commented Jul 3, 2017

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

SergioRAgostinho 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

t-lou Jul 3, 2017 • edited Loading

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

SergioRAgostinho Jul 3, 2017 • edited Loading

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

SergioRAgostinho 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

SergioRAgostinho Jul 4, 2017 • edited Loading

Choose a reason for hiding this comment

taketwo commented Jul 4, 2017 • edited Loading

t-lou commented Jul 4, 2017

taketwo commented Jul 4, 2017

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

SergioRAgostinho commented Jul 4, 2017

taketwo commented Jul 4, 2017

SergioRAgostinho commented Jul 4, 2017 • edited Loading

t-lou commented Jul 4, 2017

taketwo commented Jul 4, 2017

t-lou commented Jul 4, 2017 • edited Loading

SergioRAgostinho commented Jul 4, 2017

t-lou commented Jul 4, 2017

taketwo Jul 4, 2017 • edited Loading

SergioRAgostinho Jun 29, 2017 •

edited

Loading

taketwo commented Jun 29, 2017 •

edited

Loading

t-lou Jul 3, 2017 •

edited

Loading

SergioRAgostinho Jul 3, 2017 •

edited

Loading

SergioRAgostinho Jul 4, 2017 •

edited

Loading

taketwo commented Jul 4, 2017 •

edited

Loading

SergioRAgostinho commented Jul 4, 2017 •

edited

Loading

t-lou commented Jul 4, 2017 •

edited

Loading

taketwo Jul 4, 2017 •

edited

Loading

t-lou commented Jul 5, 2017 •

edited

Loading