Add advancing front surface reconstruction algorithm #1996

Levi-Armstrong · 2017-09-17T19:16:05Z

A implementation of a advancing front surface reconstruction algorithm based on the following papers.

Advancing Front Papers

High-Quality Extraction of Isosurfaces from Regular and Irregular Grids
J. Schreiner, C. Scheidegger, C. Silva
IEEE Transactions on Visualization and Computer Graphics (Proceedings of IEEE Visualization); 2006
Direct (Re)Meshing for Efficient Surface Processing
J. Schreiner, C. Scheidegger, S. Fleishman, C. Silva
Computer Graphics Forum (Proceedings of Eurographics); 2006
Triangulating Point-Set Surfaces With Bounded Error
C. Scheidegger, S. Fleishman, C. Silva
Proceedings of the third Eurographics/ACM Symposium on Geometry Processing; 2005

Testing
pcl_advancing_front_reconstruction bun0.pcd bun0.ply -radius 0.05 -rho 0.6

Debug interface
In cmake gui enable BUILD_surface_afront_debug and rebuild. When running the command above a pcl viewer window will open and press numbers 0-9 to generated 10^x triangles.

Maintainers
This includes commits from the PR #1960. I planned on waiting for the PR to be merged before submitting this, but my budget goes away at the end of this month so I wanted to get this submitted before then.

Images

taketwo · 2017-09-20T20:29:56Z

Hi, thanks again for contributing. This PR is particularly hard to deal with: a complex algorithm implemented in some 3.5K lines of code. I'm no expert in meshing in general and this algorithm in particular, so I won't be able to assess this code in terms of how well it implements the papers. Further, even thoroughly checking the logic and exploring refactoring possibilities is well beyond what I can realistically do. Therefore, in my review I will concentrate on making sure that the overall code style is in the PCL spirit.

So far I have only scanned the code. Here are initial suggestions:

Class name. I'd prefer to spell out the "advanced" completely.
File names. Should match class name, but in snake_case, of course.
License. It's alright to have your employer in the copyright, but please also add a line for Open Perception.
Debugging visualization. I can imagine it was quite helpful during development and I acknowledge that it is incredibly instructive for understanding how the algorithm works. However, I think we should not include it in the class, even if carefully isolated with macro guards.

My next step will be reviewing the advancing_front_utils.h file. Some of the functions there are already present in PCL, some may be handy for other users and thus may be moved to the common module. So, ideally, I would like to see this file dissolving.

taketwo · 2017-09-20T21:03:04Z

surface/include/pcl/surface/advancing_front_utils.h

+  {
+
+    /** \brief Align pn's normal with av so they point in the same direction */
+    bool


Do we need return type? I don't see you ever using it.

taketwo · 2017-09-20T21:04:16Z

surface/include/pcl/surface/advancing_front_utils.h

+
+    /** \brief Align pn's normal with av so they point in the same direction */
+    bool
+    alignNormal (Eigen::Vector3f &pn, const Eigen::Vector3f &av)


Maybe alignNormals?

taketwo · 2017-09-20T21:05:53Z

surface/include/pcl/surface/advancing_front_utils.h

+    bool
+    alignNormal (Eigen::Vector3f &pn, const Eigen::Vector3f &av)
+    {
+      double dot = pn.dot (av);


No need to upcast to double. And in general, this function can be reduced to:

if (pn.dot (av) < 0.0f) pn *= -1;

taketwo · 2017-09-20T21:12:26Z

surface/include/pcl/surface/advancing_front_utils.h

+    bool
+    alignNormal (AfrontVertexPointType &pn, const AfrontVertexPointType &av)
+    {
+      Eigen::Vector3f normal = pn.getNormalVector3fMap ();


I think it should work if you pass pn.getNormalVector3fMap () directly to the alignNormal function, the underlying data should be modified.

I'd move these two functions and the next one (checkNormal) to the common module. I don't really see any existing header file there that would naturally fit them, so propose to create a new one "normals.h".

While doing so, I'd make alignNormals templated on the point types, something like:

template <typename PointNT1, typename PointNT2> alignNormals (PointNT1& pn, const PointNT2& av)

to ensure reusability.

taketwo · 2017-09-20T21:15:04Z

surface/include/pcl/surface/advancing_front_utils.h

+
+    /** \brief Check if two normals are within a tolerance */
+    bool
+    checkNormal (const Eigen::Vector3f n1, const Eigen::Vector3f n2, const double &tol)


Perhaps checkNormalsEqual?
tol -> angular_threshold
It also makes sense to mention in the docstring that the threshold is in radians.

taketwo · 2017-09-20T21:22:06Z

surface/include/pcl/surface/advancing_front_utils.h

+    }
+
+    AfrontGuidanceFieldPointType
+    convertAfrontPointTypeToAfrontGuidanceFieldPointType (const AfrontVertexPointType &p, const double rho)


Only used in one place, so may be moved directly there. Alternatively, make this a member function of the vertex point type.

taketwo · 2017-09-20T21:24:08Z

surface/include/pcl/surface/advancing_front_utils.h

+      * \return The mid point of the half edge
+      */
+    Eigen::Vector3f
+    getMidPoint (const Eigen::Vector3f &p1, const Eigen::Vector3f &p2)


This is really trivial and is only used in two places, I'd simply expand this at the points of usage.

taketwo · 2017-09-20T21:29:48Z

surface/include/pcl/surface/advancing_front_utils.h

+      * \return DistPoint2LineResults
+      */
+    DistPoint2LineResults
+    distPoint2Line (const Eigen::Vector3f &p1, const Eigen::Vector3f &p2, const Eigen::Vector3f &p)


There is a point to line distance function in common in "distances.h" can we reuse it?

It looks like they are for checking against a line with infinite length, where I need to check against a line segment. Should I add these to this file? I would rename the methods to:

distPointToLineSeg
distLineSegToLineSeg

Yes, please. I'd spell "segment" completely, and also move "distance" part to the end to be similar with already existing function names, i.e. pointToLineSegmentDistance and lineSegmentToLineSegmentDistance. What do you think?

Sounds good.

taketwo · 2017-09-20T21:30:48Z

surface/include/pcl/surface/advancing_front_utils.h

+      * \return DistLine2LineResults
+      */
+    DistLine2LineResults
+    distLine2Line (const Eigen::Vector3f &p1, const Eigen::Vector3f &p2, const Eigen::Vector3f &p3, const Eigen::Vector3f &p4)


Can we integrate this into aforementioned header file?

taketwo · 2017-09-20T21:31:43Z

surface/include/pcl/surface/advancing_front_utils.h

+      * \return IntersectionLine2PlaneResults
+      */
+    IntersectionLine2PlaneResults
+    intersectionLine2Plane (const Eigen::Vector3f &p1, const Eigen::Vector3f &p2, const Eigen::Vector3f &origin, const Eigen::Vector3f &u, const Eigen::Vector3f &v)


There is a function to compute line with plane intersection in common module "intersections.h" file. Can we reuse it?

I only see lineWithLineIntersection and planeWithPlaneIntersection but no lineWithPlaneIntersection. Is it located somewhere else?

Sorry, my bad. Can we add your function there, then?

OK good. I will move the function to the intersection.h file.

SergioRAgostinho · 2017-09-22T00:22:47Z

#1960 was merged so let's rebase this one to remove the redundant commits.

moriarty · 2017-09-22T00:56:27Z

Hey @taketwo, I just saw this mentioned in a talk at ROSCon, I agree with your first comment- this is hard to review.

I just thought I'd mention the ROSCon talk video should be available either now or soon.

taketwo · 2017-09-23T09:02:23Z

Hi Alex, after having read your comment several times, I'm still not sure what was mentioned in the ROSCon talk and what relation it has with this PR. But I'm intrigued, please post a link when it's online ;)

moriarty · 2017-09-23T16:19:16Z

Yes, I was in the talk when I commented above.
I'll post a link when it's online, it was available via live stream, but now we wait for Ubuntu to upload the videos and they expect it to take a month.

The talk gave a bit of a explaination, showed some results and then mentioned that it would be available soon as the PR was being reviewed. The results looked good and useful so I checked it out during the talk.

@taketwo, I saw your comment that it's hard to check the algorithm, and that you'd just review if the code was in the style of PCL, so I thought it would be helpful to see the results. Maybe @Levi-Armstrong can link to results somewhere.

Levi-Armstrong · 2017-09-23T16:39:15Z

Sure, the results I presented are shown here. I am currently traveling back from ROSCon 2017 and on Monday I will begin addressing the comments above.

SergioRAgostinho · 2017-10-10T23:16:09Z

FYI: I'm starting to look at this again.

SergioRAgostinho

Here's a first review which focuses only on CMake related code.

SergioRAgostinho · 2017-09-22T00:27:13Z

surface/include/pcl/surface/advancing_front.h

+#ifndef PCL_SURFACE_ADVANCING_FRONT_H_
+#define PCL_SURFACE_ADVANCING_FRONT_H_
+
+#define PCL_NO_PRECOMPILE


This is normally not a good policy. What forces you to define this?

I believe it was left over from when I was developing outside pcl. I was getting errors related to my custom point type when using them with other libraries like mls and kdtree, but when I added the define they all went away.

Any reason for not registering your point type in point_types.h[pp] like all the others? Looking at the points included there I would say yours is a perfectly eligible candidate.

This is a support point type for this specific algorithm. What use will it have outside?
Added deficit: will increase build time and binary sizes due to additional template instantiations for all PCL algorithms.

The plan was to only be used by this algorithm. I need to cache additional information for each point in the mesh to support the mesher, but there should not be any use for it outside of this.

👍 makes sense. Let's keep it then.

SergioRAgostinho · 2017-10-10T23:26:36Z

cmake/pcl_options.cmake


 option(BUILD_tools "Useful PCL-based command line tools" ON)

+option(BUILD_surface_afront_debug "Enable afront mesher debug interface" OFF)


This should not be added here. At this level there's no guarantee that the surface module will be built, therefore makes no sense to show this here.

Note to self: I have a comment stating that the debug interface should be limited to a tool

SergioRAgostinho · 2017-10-10T23:59:57Z

surface/CMakeLists.txt

    ENDIF(BUILD_surface_on_nurbs)

+    IF(BUILD_surface_afront_debug)
+        add_definitions(-DAFRONTDEBUG)


I still need to see how this is implemented but in general I would prefer to not add definitions. Is there an easy way to avoid this?

SergioRAgostinho · 2017-10-11T00:01:57Z

surface/CMakeLists.txt

+    else(BUILD_surface_afront_debug)
+      target_link_libraries("${LIB_NAME}" pcl_common pcl_search pcl_kdtree pcl_octree ${VTK_LIBRARIES} ${ON_NURBS_LIBRARIES})
+    endif(BUILD_surface_afront_debug)
+


Do you see how things are done for QHull in line 180, that's how you should link with VTK. Move your condition below line 194.

SergioRAgostinho · 2017-10-11T00:02:06Z

surface/CMakeLists.txt

+  set(SUBSYS_DEPS common search kdtree octree geometry visualization)
+else(BUILD_surface_afront_debug)
+  set(SUBSYS_DEPS common search kdtree octree geometry)
+endif(BUILD_surface_afront_debug)


This is the perfect file to define your BUILD_surface_afront_debug option. But this block needs to move below line 15, since if the surface module is not build, this option should not pop-up.

You're adding a dependency to geometry which in turn only depends of common. geometry has a number of meshing primitives so I would say that functionally this dependency makes sense. To be checked is exactly what you use from there.

Your conditional branch should not set SUBSYS_DEPS directly, it should just append the visualization dependency to it. I want the module mandatory dependencies to appear on top first, isolated from whatever options the user decides later down the road, in case the surfacemodule is built

SergioRAgostinho · 2017-10-11T00:05:25Z

test/surface/CMakeLists.txt

  if (BUILD_io AND BUILD_features)
+    IF(BUILD_surface_afront_debug)
+        add_definitions(-DAFRONTDEBUG)
+    ENDIF(BUILD_surface_afront_debug)


Keeping in mind my comment of adding definitions, you should not need to add this here as well. It should come exported the moment you link with the surface target.

SergioRAgostinho · 2017-10-11T00:06:09Z

tools/CMakeLists.txt

 if (BUILD_tools)
+  IF(BUILD_surface_afront_debug)
+      add_definitions(-DAFRONTDEBUG)
+  ENDIF(BUILD_surface_afront_debug)


Same comment as before

Levi-Armstrong · 2017-10-17T13:40:21Z

FYI: I should be able to start addressing the open items this week.

SergioRAgostinho · 2017-10-17T21:47:57Z

Thanks. Let's address things by stages, starting with just CMake related issues at first. If possible leave each review stage on it's own commit to make it easier to traverse the history of changes.

taketwo · 2017-10-18T06:08:51Z

Some of the CMake issues are related to debug visualization. As I wrote early on, I think we should just get rid of it. What's your stance on this?

SergioRAgostinho · 2017-10-18T23:14:33Z

I was wondering if we could refactor that code and use it as a tool.

I had a "similar" situation in the past, when I wrote a tracker class, and (of course) found out at some point that it was necessary to inject some more code to aid in understanding what was happening with the tracker. My solution for that was to derive a child class of the tracker to work as a debugger of what was going. The debugger was working as the middle man, wrapping all calls to the parent while having access to the protected members.

In this case, the debugger class would only reside in the tools module, alongside an application which invokes it. I was wondering if such strategy could be applied here as well.

taketwo · 2017-10-19T07:57:55Z

This sounds like a reasonable thing to do, although will require extra effort from Levi.

Levi-Armstrong · 2017-10-23T13:09:21Z

I will see what I can do. I believe this would be good to have since the paper mentions advanced techniques for reconstructing sharp edges where the tool would be useful for implementing this in the future.

SergioRAgostinho · 2017-10-23T22:33:13Z

Thanks! Then I would suggest to pull the visualization code/tool for another PR, and for now strip this one of everything related to that. Does it sound reasonable? I'll try to go through another review stage in the upcoming days.

SergioRAgostinho

2nd review focused on the new point type you're defining.

SergioRAgostinho · 2017-10-25T19:22:25Z

surface/include/pcl/surface/advancing_front_point_type.h

+ *
+ * Point Cloud Library (PCL) - www.pointclouds.org
+ * Copyright (c) 2017-, Southwest Research Institute
+ *


I think Sergey already mentioned this somewhere else but you need to add Open Perception here as well.

SergioRAgostinho · 2017-10-25T19:29:31Z

surface/include/pcl/surface/advancing_front_point_type.h

+#define PCL_SURFACE_ADVANCING_FRONT_POINT_TYPE_H_
+
+#include <pcl/point_types.h>
+#include <pcl/register_point_struct.h>


This one should not be required, since it's already included by point_types.h

SergioRAgostinho · 2017-10-25T19:31:27Z

surface/include/pcl/surface/advancing_front_point_type.h

+
+#include <pcl/point_types.h>
+#include <pcl/register_point_struct.h>
+#include <pcl/impl/instantiate.hpp>


Why is this required here? There's nothing on this file which makes instantiations.

SergioRAgostinho · 2017-10-25T19:32:52Z

surface/include/pcl/surface/advancing_front_point_type.h

+
+namespace pcl
+{
+  namespace afront


I don't think this extra namespace is required. You're already naming everything Afront....

SergioRAgostinho · 2017-10-25T19:34:57Z

surface/include/pcl/surface/advancing_front_point_type.h

+        curvature = p.curvature;
+        max_step = p.max_step;
+        max_step_search_radius = p.max_step_search_radius;
+      }


Why are you required to define this? This looks like the default copy constructor and the compiler should generate one for you automatically.

I just noticed that most of the existing if not all did this so I assumed that there was a reason behind it. Let me know if you would like it removed?

Yes please. 👍 There's a couple of things that happen if you define the copy constructor, because intuitively you're telling the compiler "Wait. My copy process is more complex than usual" and the compiler won't generate a couple of useful methods for you in that case. See https://stackoverflow.com/questions/3734247/what-are-all-the-member-functions-created-by-compiler-for-a-class-does-that-hap#3734268 for more info.

SergioRAgostinho · 2017-10-25T19:36:53Z

surface/include/pcl/surface/advancing_front_point_type.h

+      friend std::ostream &
+      operator<< (std::ostream &os, const AfrontVertexPointType &p)
+      {
+        os << p.x << "\t" << p.y << "\t" << p.z;


I think you should actually print all your point type's internal members.

SergioRAgostinho · 2017-10-25T19:37:53Z

surface/include/pcl/surface/advancing_front_point_type.h

+        data_n[3] = 0.0f;
+        curvature = p.curvature;
+        ideal_edge_length = p.ideal_edge_length;
+      }


Same comment as before.

SergioRAgostinho · 2017-10-25T19:38:56Z

surface/include/pcl/surface/advancing_front_point_type.h

+      friend std::ostream &
+      operator<< (std::ostream &os, const AfrontGuidanceFieldPointType &p)
+      {
+        os << p.x << "\t" << p.y << "\t" << p.z;


Same comment as before.

SergioRAgostinho · 2017-10-25T19:39:48Z

surface/include/pcl/surface/advancing_front_point_type.h

+  } // namespace afront
+} // namespace pcl
+POINT_CLOUD_REGISTER_POINT_STRUCT (pcl::afront::AfrontVertexPointType,
+                                   (float, x, x) (float, y, y) (float, z, z) (float, normal_x, normal_x) (float, normal_y, normal_y) (float, normal_z, normal_z) (float, curvature, curvature) (float, max_step, max_step) (float, max_step_search_radius, max_step_search_radius))


Can you span this over multiple lines to increase readability? Each field on each line.

SergioRAgostinho · 2017-10-25T19:40:05Z

surface/include/pcl/surface/advancing_front_point_type.h

+                                   (float, x, x) (float, y, y) (float, z, z) (float, normal_x, normal_x) (float, normal_y, normal_y) (float, normal_z, normal_z) (float, curvature, curvature) (float, max_step, max_step) (float, max_step_search_radius, max_step_search_radius))
+
+POINT_CLOUD_REGISTER_POINT_STRUCT (pcl::afront::AfrontGuidanceFieldPointType,
+                                   (float, x, x) (float, y, y) (float, z, z) (float, normal_x, normal_x) (float, normal_y, normal_y) (float, normal_z, normal_z) (float, curvature, curvature) (float, ideal_edge_length, ideal_edge_length))


Same comment as above.

SergioRAgostinho

Only had time to review the functions you added to common/distance.

Also, there's still a number of warnings being printed in Travis that need to be suppressed.

SergioRAgostinho · 2017-11-21T22:30:22Z

surface/CMakeLists.txt

        "include/pcl/${SUBSYS_NAME}/texture_mapping.h"
        "include/pcl/${SUBSYS_NAME}/poisson.h"
+        "include/pcl/${SUBSYS_NAME}/advancing_front.h"
+        "include/pcl/${SUBSYS_NAME}/advancing_front_point_type.h"


I'm thinking that it might not be worth to have a separate header for this. To decide later in a future review.

SergioRAgostinho · 2017-11-21T22:42:58Z

common/include/pcl/common/distances.h

+    Eigen::Vector3f p;         /**< \brief The closest point on line segment. */
+    float mu;                  /**< \brief The corresponding line paramentric value resulting in the minimum distance. */
+    double d;                  /**< \brief The distance between point and line segments */
+  };


I don't think it makes sense to define this PointToLineSegmentDistanceResults type. I'll suggest a different function signature for pointToLineSegmentDistance and delete this definition altogether.

SergioRAgostinho · 2017-11-21T23:04:02Z

common/include/pcl/common/distances.h

+    else if (c2 <= c1)
+      results.mu = 1.0;
+    else
+      results.mu = c1 / c2;


~~it makes no sense to clamp mu between 0 and 1.~~

It is measuring the distance between a point and a Line segment. Example for the case of triangle edge, I need to know the distance from a point to a line segment so mu must be clamped between 0 and 1 to get the correct distance.

Yes, but this function will be used for more than your triangular case, so that check needs to be done a posteriori.

That's fine as long as the function return mu I can check it in my code.

Then again, the name of the function is distance to line segment and not really distance to line.

@taketwo opinion: Should we allow the option of clamping as a parameter?

Another option could be to modify the existing sqrPointToLineDistance to calculate mu. Then I can get the information I need if that simplifies things.

Oh true, that methods exists! Then my comment makes no sense. I'll delete/strike through everything which is not longer valid.

For clamping please use a combination of std::max and std::min

SergioRAgostinho · 2017-11-21T23:04:48Z

common/include/pcl/common/distances.h

+      results.mu = c1 / c2;
+
+    results.p = results.points[0] + results.mu * v;
+    results.d = (p - results.p).norm ();


~~If mu is clamped this distance is incorrect.~~

Explanation above.

SergioRAgostinho · 2017-11-21T23:05:28Z

common/include/pcl/common/distances.h

+    * \param[in] p2 Terminating point of the line
+    * \param[in] p The point for calulating the distance to the line
+    * \return DistPoint2LineResults
+    */


This should not be defined inline and should not be in the header. Create distances.cpp, add the corresponding entry in the CMake file and move it there.

SergioRAgostinho · 2017-11-22T00:17:00Z

common/include/pcl/common/distances.h

+      result.parallel = true;
+
+      dist = pointToLineSegmentDistance (p1, p2, p3);
+      result.mu[0] = dist.mu;


~~You only need to compute the point to line segment once because they're parallel. In this situation, you should set both mus and the corresponding interception points to NaN~~

SergioRAgostinho · 2017-11-22T00:24:43Z

common/include/pcl/common/distances.h

+    else
+    {
+      result.mu[0] = (d1343 * d4321 - d1321 * d4343) / denom;
+      result.mu[1] = (d1343 + result.mu[0] * d4321) / d4343;


According to my calculations this should be

mu0 = (d4343 * d1321 - d4321* d1343) /det; mu1 = (d4321 * d1321 - d2121* d1343) /det;

Edit: Forgot the determinant. Now the expressions look the same.

SergioRAgostinho · 2017-11-22T00:25:14Z

common/include/pcl/common/distances.h

+      if (result.mu[1] > 1.0)
+        result.mu[1] = 1.0;
+      else if (result.mu[1] < 0.0)
+        result.mu[1] = 0.0;


~~Just like before, these clampings make no sense.~~

Use the combination of std::min and std::max

SergioRAgostinho · 2017-11-22T00:25:50Z

common/include/pcl/common/distances.h

+        result.mu[1] = 0.0;
+
+      result.p[0] = p1 + result.mu[0] * (p2 - p1);
+      result.p[1] = p3 + result.mu[1] * (p4 - p3);


~~This makes no sense if the mus are clamped.~~

SergioRAgostinho · 2017-11-22T00:27:48Z

common/include/pcl/common/distances.h

+
+    result.d = (result.p[1] - result.p[0]).norm ();
+    return result;
+  }


Similar comment here as before in pointToLineSegmentDistance. Move the function to a cpp file, change the signature in the same way and make the corresponding overload.

SergioRAgostinho · 2017-11-22T15:04:03Z

Update the comments. After the discussion, the target now is only changes in the functions signatures and the moving to a normal .cpp file.

taketwo

So far we've been dealing with the helper functions, not the algorithm proper, and the conversation has already 92 comments. To make this more digestable, would it make sense to branch out the meshing class and tool into a separate PR? And here keep only helpers and their tests?

taketwo · 2018-02-15T21:55:54Z

common/include/pcl/common/normals.h

+
+}
+
+#include <pcl/common/impl/normals.hpp>


None of these functions are templated, so should go into src/normals.cpp.

taketwo · 2018-02-15T21:56:31Z

common/include/pcl/common/normals.h

+
+namespace pcl
+{
+  /** \brief Align pn's normal with av so they point in the same direction */


Please add explanation of what is returned, not obvious.

taketwo · 2018-02-15T21:58:52Z

common/include/pcl/common/normals.h

+{
+  /** \brief Align pn's normal with av so they point in the same direction */
+  bool
+  alignNormals (Eigen::Ref<Eigen::Vector3f> pn, const Eigen::Ref<const Eigen::Vector3f> &av);


Apparently Eigen::Ref<> is the right way to take Eigen objects by reference, but it is not how it is done in PCL. I would be okay if you consistently used this style through the contribution, but it's not the case. Thus I'd propose to switch to "standard" reference to avoid confusing users.

stale · 2018-04-16T22:19:59Z

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.

Micalson · 2019-05-27T05:58:41Z

@Levi-Armstrong Hi, I find the pull request about "A implementation of a advancing front surface reconstruction algorithm based on the following papers." But I cannot find the function in pcl repo.
I want to use the advancing front surface reconstruction algorithm, where could I find it? Thank you for your reply.

Regards,

Levi-Armstrong · 2019-05-28T03:40:17Z

@Micalson The only way to use it currently, is to build my branch in this PR to be able to leverage this meshing algorithm at them moment. I need to wrap up a few things before this can be merged, but I wanted to fix one know issue at the moment. It currently sometimes will wrap on its self. I believe I have a solution but I have not had time to implement and test. If you run into any issues let me know.

Micalson · 2019-05-30T21:59:11Z

@Levi-Armstrong I get it. Thanks for your reply.

stale · 2019-07-29T22:22:13Z

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.

stale · 2019-10-30T18:45:06Z

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.

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

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.

taketwo reviewed Sep 20, 2017

View reviewed changes

SergioRAgostinho self-assigned this Sep 22, 2017

Levi-Armstrong force-pushed the addAfrontMesher branch from 67410b6 to cb75d1d Compare September 25, 2017 13:53

taketwo added needs: code review Specify why not closed/merged yet needs: author reply Specify why not closed/merged yet labels Sep 26, 2017

SergioRAgostinho requested changes Oct 11, 2017

View reviewed changes

SergioRAgostinho requested changes Oct 25, 2017

View reviewed changes

SergioRAgostinho added the changelog: new feature Meta-information for changelog generation label Nov 6, 2017

Levi-Armstrong force-pushed the addAfrontMesher branch 2 times, most recently from 9096172 to b50bcea Compare November 15, 2017 13:17

SergioRAgostinho requested changes Nov 22, 2017

View reviewed changes

SergioRAgostinho added needs: more work Specify why not closed/merged yet and removed needs: code review Specify why not closed/merged yet needs: author reply Specify why not closed/merged yet labels Nov 22, 2017

SergioRAgostinho mentioned this pull request Nov 22, 2017

[RFC] Added simple kinectv2 grabber and viewer #1410

Closed

3 tasks

taketwo reviewed Feb 15, 2018

View reviewed changes

stale bot added the status: stale label Apr 16, 2018

stale bot removed the status: stale label May 27, 2019

stale bot added the status: stale label Jul 29, 2019

Levi-Armstrong added 13 commits August 31, 2019 11:16

Add Advancing Front Mesher

5c71f93

Add common/normals.h and common/impl/normals.hpp

3dce1de

Move advancing front debug tool

688a93c

Add Open Perception Copyright

ce93e4f

Make requested changes to advancing_front_point_types

48ad8e5

Rename Afront to AdvancingFront

d6d909c

Move utility function to pcl common

672b909

Add legend to degub tool viewports

da26c9e

Fix clang compiler warnings and errors

06317fe

Add ability to set a max triangle edge length

beb43bd

Fix additional clang compiler warnings

a46edf1

Remove advancing front point type copy constructors

e849eda

Rebase on master and update due changes in master

52ada29

Levi-Armstrong force-pushed the addAfrontMesher branch from 65af268 to 52ada29 Compare August 31, 2019 18:09

stale bot removed the status: stale label Aug 31, 2019

stale bot added the status: stale label Oct 30, 2019

kunaltyagi removed the stale label Feb 27, 2020

stale bot removed the status: stale label Feb 27, 2020

taketwo added the status: stale label Mar 29, 2020


		option(BUILD_tools "Useful PCL-based command line tools" ON)

		option(BUILD_surface_afront_debug "Enable afront mesher debug interface" OFF)

Uh oh!

Add advancing front surface reconstruction algorithm #1996

Are you sure you want to change the base?

Add advancing front surface reconstruction algorithm #1996

Uh oh!

Conversation

Levi-Armstrong commented Sep 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taketwo commented Sep 20, 2017

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!

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!

SergioRAgostinho commented Sep 22, 2017

Uh oh!

moriarty commented Sep 22, 2017

Uh oh!

taketwo commented Sep 23, 2017

Uh oh!

moriarty commented Sep 23, 2017

Uh oh!

Levi-Armstrong commented Sep 23, 2017

Uh oh!

SergioRAgostinho commented Oct 10, 2017

Uh oh!

SergioRAgostinho left a comment

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!

Levi-Armstrong commented Sep 17, 2017 •

edited

Loading

SergioRAgostinho commented Oct 17, 2017 •

edited

Loading

SergioRAgostinho commented Oct 18, 2017 •

edited

Loading