Add ability to cache mls results #1952

Levi-Armstrong · 2017-07-28T19:59:43Z

No description provided.

taketwo

Looks almost fine, except to the return type, see the inline comments.

taketwo · 2017-07-29T13:36:52Z

surface/include/pcl/surface/mls.h

@@ -106,6 +128,7 @@ namespace pcl
                              upsampling_radius_ (0.0),
                              upsampling_step_ (0.0),
                              desired_num_points_in_radius_ (0),
+                              cache_mls_results_(false),


Missing space

taketwo · 2017-07-29T13:39:24Z

surface/include/pcl/surface/mls.h

+       *  /param[in] True if the mls results should be stored, otherwise false.
+       */
+      inline void
+      setCacheMLSResults(bool cache_mls_results) {cache_mls_results_ = cache_mls_results;}


Missing space after function name and inside curly braces.
Also, I don't know how useful this will be, but in PCL we always have accessors for the algorithm configuration parameters. This would be getCacheMLSResults() in this case.

Edit: actually, this parameter can be modified during the algorithm execution, so I think this indeed will be a useful getter.

taketwo · 2017-07-29T13:42:02Z

surface/include/pcl/surface/mls.h

+       *  /note The results are only stored if setCacheMLSResults(true) was called
+       *        or when using the upsampling method DISTINCT_CLOUD or VOXEL_GRID_DILATION.
+       */
+      inline std::vector<MLSResult> getMLSResults() const {return mls_results_;}


Why not returning by const ref? Otherwise will be quite a costly function.
Also missing spaces as above.

Levi-Armstrong · 2017-07-30T01:14:16Z

@taketwo I made the requested changes.

Levi-Armstrong · 2017-08-01T05:25:52Z

With these changes I did not need a wrapper class around the MovingLeastSquares class so I started relocating the functions and ran into an issue. If I want to create a function that take the MLSResults structure I have to define it with the template arguments pcl::MoveingLeastSquares<pcl::PointXYZ, pcl::PointNormal>::MLSResults. Should this MLSResults structure be moved outside the class or some other location?

taketwo · 2017-08-01T06:32:52Z

MLSResults per se is point type agnostic, so I agree it makes sense to shift it out of the class. I'd keep the declaration it in the same header since it's tightly coupled.

Levi-Armstrong · 2017-08-01T13:22:22Z

@taketwo, Thank you I pushed a new commit moving it outside the class.

SergioRAgostinho

I need to better understand what is the intent of this functionality. Currently this is what it does:

Allows the user to specify caching, regardless of the upsampling method.
Forces caching, regardless of user option if the method is VOXEL_GRID_DILATION or DISTINCT_CLOUD

What was the original objective of this PR? Extend caching to all upsampling methods? Because from what I can tell VOXEL_GRID_DILATION or DISTINCT_CLOUD were already caching mls_results_, although it was not publicly accessible.

SergioRAgostinho · 2017-08-02T20:43:56Z

surface/include/pcl/surface/mls.h

@@ -549,6 +568,7 @@ namespace pcl
      using MovingLeastSquares<PointInT, PointOutT>::order_;
      using MovingLeastSquares<PointInT, PointOutT>::compute_normals_;
      using MovingLeastSquares<PointInT, PointOutT>::upsample_method_;
+      using MovingLeastSquares<PointInT, PointOutT>::cache_mls_results_;


Can we move all these which are not public to a non-public scope?

SergioRAgostinho · 2017-08-02T21:01:29Z

surface/include/pcl/surface/mls.h

+    */
+  struct MLSResult
+  {
+    MLSResult () : mean (), plane_normal (), u_axis (), v_axis (), c_vec (), num_neighbors (), curvature (), valid (false) {}


Can we remove the eigen default initializers which do nothing and explicitly set the value initialized the native types to 0/false?

Sure I will make the change. I just kept the same as it was.

SergioRAgostinho · 2017-08-02T21:02:26Z

surface/include/pcl/surface/mls.h

+    Eigen::VectorXd c_vec;
+    int num_neighbors;
+    float curvature;
+    bool valid;


It would be cool to have some doxygen comments on these.

Sure, I will add the documentation.

Thanks a lot

SergioRAgostinho · 2017-08-02T21:17:24Z

surface/include/pcl/surface/impl/mls.hpp


      break;
    }
    case (VOXEL_GRID_DILATION):
    case (DISTINCT_CLOUD):
      {
-        mls_results_.resize (input_->size ());
+        cache_mls_results_ = true;


So if the method is VOXEL_GRID_DILATION or DISTINCT_CLOUD, the caching is automatically set to true, despite if the user previously set caching to be false. It this happens we should be throwing a warning.

For everything else, it uses whatever option the user has decided before.

I agree, I will wait until a determination is made if this should always cache because then this function would be removed.

SergioRAgostinho · 2017-08-02T21:27:55Z

surface/include/pcl/surface/impl/mls.hpp

@@ -396,9 +407,6 @@ pcl::MovingLeastSquares<PointInT, PointOutT>::computeMLSPointNormal (int index,
    case (VOXEL_GRID_DILATION):
    case (DISTINCT_CLOUD):
    {
-      // Take all point pairs and sample space between them in a grid-fashion
-      // \note consider only point pairs with increasing indices
-      mls_result = MLSResult (point, plane_normal, u_axis, v_axis, c_vec, static_cast<int> (nn_indices.size ()), curvature);


Any reason to leave the empty case blocks here?

I think it was to suppress warning messages. I can remove the two and default instead?

Makes sense. I would go with default.

SergioRAgostinho · 2017-08-02T21:35:12Z

surface/include/pcl/surface/mls.h

@@ -52,6 +52,28 @@

 namespace pcl
 {
+  /** \brief Data structure used to store the results of the MLS fitting
+    * \note Used only in the case of VOXEL_GRID_DILATION or DISTINCT_CLOUD upsampling


I don't think this comment is true. VOXEL_GRID_DILATION or DISTINCT_CLOUD set the the caching procedure to true. But the other methods, also use caching as long as the user manually sets it.

I agree. I will remove the note.

SergioRAgostinho · 2017-08-02T21:35:36Z

surface/include/pcl/surface/mls.h

-        bool valid;
-      };
+      /** \brief True if the mls results for the input cloud should be stored */
+      bool cache_mls_results_;


I think we need a warning here in the comments stating that this will be forced to true if the methods used are VOXEL_GRID_DILATION or DISTINCT_CLOUD.

I agree, I will wait until a determination is made if this should always cache because then this function would be removed.

SergioRAgostinho · 2017-08-02T21:35:59Z

surface/include/pcl/surface/mls.h

+        * \param[in] True if the mls results should be stored, otherwise false.
+        */
+      inline void
+      setCacheMLSResults (bool cache_mls_results) { cache_mls_results_ = cache_mls_results; }


I think we need a warning here in the comments stating that cache_mls_results_ will be forced to true if the methods used are VOXEL_GRID_DILATION or DISTINCT_CLOUD.

I agree, I will wait until a determination is made if this should always cache because then this function would be removed.

SergioRAgostinho · 2017-08-02T21:48:29Z

surface/include/pcl/surface/mls.h

@@ -106,6 +128,7 @@ namespace pcl
                              upsampling_radius_ (0.0),
                              upsampling_step_ (0.0),
                              desired_num_points_in_radius_ (0),
+                              cache_mls_results_ (false),


Should we not cache by default?

Levi-Armstrong · 2017-08-04T02:54:24Z

What was the original objective of this PR?

The objective is what you observed.

Allows the user to specify caching, regardless of the upsampling method.

This was needed for the implementation of the Advancing Front Meshing.

Forces caching, regardless of user option if the method is VOXEL_GRID_DILATION or DISTINCT_CLOUD

This is required for these two methods only because they are preformed after the MLS surface was generated but the MLS data is required to perform the upsampling.

Extend caching to all upsampling methods? Because from what I can tell VOXEL_GRID_DILATION or DISTINCT_CLOUD were already caching mls_results_, although it was not publicly accessible.

That is correct, I just kept it the same as it was before. The only reason I assume they did not cache it for each was for the purpose of memory consumption. I can change it to always cache which would simplify the code.

SergioRAgostinho · 2017-08-04T03:26:54Z

I would leave the option to cache/not cache if desired, but enable cache by default on the constructor. That way, if the user has special memory constraints, it still can save some memory.

What are your feelings on this @Levi-Armstrong and @taketwo ?

Levi-Armstrong · 2017-08-04T03:42:56Z

I am OK with leaving the option to cache. Though if we do set it to true by default this is not the same behavior as the original, but not sure if that would be an issue.

Levi-Armstrong · 2017-08-04T03:51:03Z

Also I noticed that several of the getter methods are not const. Are you OK if I make them const?

SergioRAgostinho · 2017-08-04T03:54:02Z

Also I noticed that several of the getter methods are not const. Are you OK if I make them const?

Yes. Keep it on a separate commit.

Levi-Armstrong · 2017-08-04T04:18:34Z

I have made the requested changes and set the default to cache the mls results.

taketwo · 2017-08-04T06:43:08Z

I don't have a strong opinion about what should be the default. I think that making caching "opt-out" is fine as long as there is a note in the documentation which sheds some light on why the user might want to disable it.

Side note: Sergio, should we maybe have a separate section in our next changelog which lists breaking changes and changes of behavior?

Levi-Armstrong · 2017-08-04T16:26:53Z

I don't have a strong opinion about what should be the default. I think that making caching "opt-out" is fine as long as there is a note in the documentation which sheds some light on why the user might want to disable it.

I will add a note explaining when setting it to false may be necessary.

SergioRAgostinho · 2017-08-04T17:28:30Z

Side note: Sergio, should we maybe have a separate section in our next changelog which lists breaking changes and changes of behavior?

I can see the importance yes... How about we format it differently using italic (change of behavior) and bold (breaking changes).

SergioRAgostinho

Thanks for the effort guys.

SergioRAgostinho · 2017-08-04T18:15:29Z

surface/include/pcl/surface/mls.h

+  /** \brief Data structure used to store the results of the MLS fitting */
+  struct MLSResult
+  {
+    MLSResult () : num_neighbors (), curvature (), valid (false) {}


My previous comment on this was also to explicitly initialize an curvature as well to make more obvious what's happening

MLSResult () : num_neighbors (0), curvature (0.f), valid (false) {}

OK, I make the change.

Levi-Armstrong · 2017-08-05T02:53:08Z

@SergioRAgostinho I added the note about when not caching mls result would be useful and added update the initialized members.

Levi-Armstrong mentioned this pull request Jul 28, 2017

Add Advancing Front Meshing Algorithm to PCL #1946

Closed

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

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

taketwo reviewed Jul 29, 2017

View reviewed changes

taketwo added needs: author reply Specify why not closed/merged yet and removed needs: code review Specify why not closed/merged yet labels Jul 29, 2017

Add ability to cache mls results

e8c036e

Levi-Armstrong force-pushed the cacheMLS branch from b82ee77 to e8c036e Compare July 30, 2017 01:13

taketwo approved these changes Jul 30, 2017

View reviewed changes

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

Move struct MLSResult outside the MoveingLeastSquares class.

b8e0c56

SergioRAgostinho requested changes Aug 2, 2017

View reviewed changes

SergioRAgostinho approved these changes Aug 4, 2017

View reviewed changes

Levi-Armstrong force-pushed the cacheMLS branch from 2ba1b3a to 1208060 Compare August 5, 2017 02:49

Levi-Armstrong added 2 commits August 4, 2017 21:51

Add documentation for MLSResults and cache MLS results by default

21721cc

Make MovingLeastSqures getter methods const

90b1b7c

Levi-Armstrong force-pushed the cacheMLS branch from 1208060 to 90b1b7c Compare August 5, 2017 02:51

Levi-Armstrong mentioned this pull request Aug 7, 2017

Add new mls projection method. Deprecated MovingLeastSquares::setPolynomialFit(). #1960

Merged

taketwo merged commit 3651eb9 into PointCloudLibrary:master Aug 9, 2017

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

SergioRAgostinho mentioned this pull request Aug 6, 2018

Change Log automation #2385

Closed

SergioRAgostinho mentioned this pull request Aug 25, 2018

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

Merged

SergioRAgostinho added changelog: API break Meta-information for changelog generation module: surface labels Aug 26, 2018

kunaltyagi mentioned this pull request May 30, 2020

Temporary bugfix for issue #4082 #4151

Closed

Add ability to cache mls results #1952

Add ability to cache mls results #1952

Conversation

Levi-Armstrong commented Jul 28, 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

Levi-Armstrong commented Jul 30, 2017

Levi-Armstrong commented Aug 1, 2017 • edited Loading

taketwo commented Aug 1, 2017

Levi-Armstrong commented Aug 1, 2017

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

SergioRAgostinho Aug 2, 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

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

Levi-Armstrong commented Aug 4, 2017 • edited Loading

SergioRAgostinho commented Aug 4, 2017

Levi-Armstrong commented Aug 4, 2017

Levi-Armstrong commented Aug 4, 2017

SergioRAgostinho commented Aug 4, 2017

Levi-Armstrong commented Aug 4, 2017

taketwo commented Aug 4, 2017

Levi-Armstrong commented Aug 4, 2017

SergioRAgostinho commented Aug 4, 2017

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

Levi-Armstrong commented Aug 5, 2017

Levi-Armstrong commented Aug 1, 2017 •

edited

Loading

SergioRAgostinho Aug 2, 2017 •

edited

Loading

Levi-Armstrong commented Aug 4, 2017 •

edited

Loading