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

Levi-Armstrong · 2017-08-07T23:38:25Z

This PR adds a new projection method which projects the point orthogonal to the mls surface. This produces a better reconstruction in areas of high curvature when using a polynomial fit. There are several free function added that could be used by other mls techniques and meshing algorithms.

Note: This is builds on PR #1952 which has not been merged but I wanted to get this submitted to start discussions on the new features added.

Levi-Armstrong · 2017-08-11T16:29:01Z

Any preferences on the structure (free functions)?

taketwo · 2017-08-11T21:39:33Z

Hi, I'm away all next week, so no inputs from my side till next next week.

UnaNancyOwen · 2017-08-12T08:20:04Z

I'm sorry, it is a click mistake. :'(

SergioRAgostinho · 2017-08-15T11:23:08Z

Sorry for the delay guys. I'll be making a push on things until the end of the week.

SergioRAgostinho

Hey @Levi-Armstrong. I'm sorry to ask for this, but I think I'll need some help to understand all changes you did. I can see you added a projection method, but you also moved a lot of code around, and it's not being easy for me to understand all changes you did.

Can you provide me with a bullet list with more details? I'm especially concerned with the refactoring process you did.

I'm also unsure about the free functions and constants but I first I need to get a better feel for the PR first.

Apologies once again.

SergioRAgostinho · 2017-08-22T23:13:34Z

surface/include/pcl/surface/mls.h


 namespace pcl
 {
+
+  const double MLS_PROJECTION_CONVERGENCE_TOLERANCE = 1e-8;
+  const double MLS_MINIMUM_PRINCIPLE_CURVATURE = 1e-5;


I'm not sure about having this here...

Where would you like them? I could also do away with them and put the values in there respected places.

I'd say so. They both are implementation details and each is used only in a single place. Just move these const definitions into their respective functions, nobody will miss them in the main header.

SergioRAgostinho · 2017-08-22T23:15:12Z

surface/include/pcl/surface/mls.h

    Eigen::Vector3d mean;         /**< \brief The mean point of all the neighbors. */
    Eigen::Vector3d plane_normal; /**< \brief The normal of the local plane of the query point. */
    Eigen::Vector3d u_axis;       /**< \brief The axis corresponding to the u-coordinates of the local plane of the query point. */
    Eigen::Vector3d v_axis;       /**< \brief The axis corresponding to the v-coordinates of the local plane of the query point. */
    Eigen::VectorXd c_vec;        /**< \brief The polynomial coefficients Example: z = c_vec[0] + c_vec[1]*v + c_vec[2]*v^2 + c_vec[3]*u + c_vec[4]*u*v + c_vec[5]*u^2 */
    int num_neighbors;            /**< \brief The number of neighbors used to create the mls surface. */
    float curvature;              /**< \brief The curvature at the query point. */
+    int order;                    /**< \brief The order of the polynomial used if polynomial_fit = true */
+    bool polynomial_fit;          /**< \brief If True, a polynomial surface was fitted to the neighbors as the mls surface */


I think would use a negative order value to indicate no polynomial_fit was done.

I like this. I can make it so zero indicates no polynomial fit.

Then consider this variable to be unsigned and probably you don't need 32bits to represent the order of your polynomial.

I agree with the unsigned recommendation, it expresses the intent about the expected valid range and semantics better. However, I don't think that using a smaller int will add anything to the picture and make the code more expressive. From the storage viewpoint it will probably also make no difference due to alignment.

That being said, I wonder if this variable is needed at all. Isn't it the same as c_vec.length()?

The only issue I see with using c_vec.length() is you can get a length of zero if the fit failed.

Levi-Armstrong · 2017-08-28T20:54:58Z

Hey @Levi-Armstrong. I'm sorry to ask for this, but I think I'll need some help to understand all changes you did. I can see you added a projection method, but you also moved a lot of code around, and it's not being easy for me to understand all changes you did.

Can you provide me with a bullet list with more details? I'm especially concerned with the refactoring process you did.

Sorry, I was on travel all last week. As for the free function, I went back and forth on this on whether to make them free function or member function. The main advantage is if they are free functions I can easily write tests for these individual function, but I am not opposed to moving them to member function if this is preferred.

In short most of the refactoring was to reduce code duplication and make it easier to read given the addition of different projection methods.

Original there was some duplication of code with in the existing implementation for example add data to the point cloud here. So to prevent from duplicating code I created this helper function.
Additional items like calculating the partial derivatives was written inline, but with the addition of a new projection method it was moved to an independent function since it will be used several places to prevent duplicating code.
Also moved the currently projections method (Simple Projection) to its own function given the addition of a new projection method for readability.
Also moved the code for computing the mls surface to its own code for readability and given that there are various techniques for computing this via custom weighting functions it would make it simpler in the future if such techniques wanted to made available.

taketwo · 2017-08-29T08:34:17Z

surface/include/pcl/surface/mls.h

@@ -474,7 +626,7 @@ namespace pcl
      }

      /** \brief Smooth a given point and its neighborghood using Moving Least Squares.
-        * \param[in] index the inex of the query point in the input cloud
+        * \param[in] index the index of the query point in the input cloud
        * \param[in] nn_indices the set of nearest neighbors indices for pt
        * \param[in] nn_sqr_dists the set of nearest neighbors squared distances for pt


This param is gone

I will remove it.

taketwo · 2017-08-29T08:41:38Z

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

+{
+}
+
+void pcl::getMLSCoordinates (const Eigen::Vector3d &pt, const pcl::MLSResult &mls_result, double &u, double &v, double &w)


Return type should be on a separate line

I will make the change.

The comments applies to a number of methods below.

taketwo · 2017-08-29T08:45:42Z

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

-    P_weight_Pt.llt ().solveInPlace (c_vec);
-  }
-
-  if (cache_mls_results_)


Do I understand right that caching became unconditional?

It is conditional. Since the the vector gets resized I just fill out the object when it is passed to the function instead of constructing a new object within the computeMLSPointNormal method.

Location 1

Location 2

Ah, right. Thanks for clarification.

SergioRAgostinho · 2017-09-03T03:46:27Z

Thanks for the explanation. I will try to make the best out of it 😅

Sorry, I was on travel all last week. As for the free function, I went back and forth on this on whether to make them free function or member function. The main advantage is if they are free functions I can easily write tests for these individual function, but I am not opposed to moving them to member function if this is preferred.

In general, you should not bend method visibility in order to be able write unit tests. Writing the tests comes after you've decided what makes sense to be public in your API.

In short most of the refactoring was to reduce code duplication and make it easier to read given the addition of different projection methods.

Original there was some duplication of code with in the existing implementation for example add data to the point cloud here. So to prevent from duplicating code I created this helper function.

Additional items like calculating the partial derivatives was written inline, but with the addition of a new projection method it was moved to an independent function since it will be used several places to prevent duplicating code.

Also moved the currently projections method (Simple Projection) to its own function given the addition of a new projection method for readability.

Also moved the code for computing the mls surface to its own code for readability and given that there are various techniques for computing this via custom weighting functions it would make it simpler in the future if such techniques wanted to made available.

For future PRs, please split logic changes like the ones you described into multiple commits. It makes it easier to review.

SergioRAgostinho

I'm still lacking insight into this, but overall I feel this code is messy. I also don't like that the default precision is double for almost everything.

I aknowledge that my comments are more directed towards the original implementation and not necessarily this PR :/

SergioRAgostinho · 2017-09-03T03:51:11Z

surface/include/pcl/surface/mls.h

    Eigen::Vector3d mean;         /**< \brief The mean point of all the neighbors. */
    Eigen::Vector3d plane_normal; /**< \brief The normal of the local plane of the query point. */
    Eigen::Vector3d u_axis;       /**< \brief The axis corresponding to the u-coordinates of the local plane of the query point. */
    Eigen::Vector3d v_axis;       /**< \brief The axis corresponding to the v-coordinates of the local plane of the query point. */
    Eigen::VectorXd c_vec;        /**< \brief The polynomial coefficients Example: z = c_vec[0] + c_vec[1]*v + c_vec[2]*v^2 + c_vec[3]*u + c_vec[4]*u*v + c_vec[5]*u^2 */
    int num_neighbors;            /**< \brief The number of neighbors used to create the mls surface. */
    float curvature;              /**< \brief The curvature at the query point. */
+    int order;                    /**< \brief The order of the polynomial used if polynomial_fit = true */
+    bool polynomial_fit;          /**< \brief If True, a polynomial surface was fitted to the neighbors as the mls surface */


Then consider this variable to be unsigned and probably you don't need 32bits to represent the order of your polynomial.

SergioRAgostinho · 2017-09-03T03:56:54Z

surface/include/pcl/surface/mls.h

+    double z_v;   /**< @brief The partial derivative dz/dv. */
+    double z_uu;  /**< @brief The partial derivative d^2z/du^2. */
+    double z_vv;  /**< @brief The partial derivative d^2z/dv^2. */
+    double z_uv;  /**< @brief The partial derivative d^2z/dudv. */


This comment is applicable for the rest. You use double precision by default. Is this really required?

SergioRAgostinho · 2017-09-03T04:01:06Z

surface/include/pcl/surface/mls.h

+    * \param[out] v The v-coordinate of the point in local MLS frame.
+    * \param[out] w The w-coordinate of the point in local MLS frame.
+    */
+  void getMLSCoordinates (const Eigen::Vector3d &pt, const pcl::MLSResult &mls_result, double &u, double &v, double &w);


This and a number of methods which take a reference to a const MLSResult feel like they should be methods of MLSResult.

SergioRAgostinho · 2017-09-03T04:01:13Z

surface/include/pcl/surface/mls.h

+    * \param[out] u The u-coordinate of the point in local MLS frame.
+    * \param[out] v The v-coordinate of the point in local MLS frame.
+    */
+  void getMLSCoordinates (const Eigen::Vector3d &pt, const pcl::MLSResult &mls_result, double &u, double &v);


Same as above

SergioRAgostinho · 2017-09-03T04:01:51Z

surface/include/pcl/surface/mls.h

+    * \param[in] mls_result The MLS results data
+    * \return The polynomial value at the provide uv coordinates.
+    */
+  double getPolynomialValue (const double u, const double v, const pcl::MLSResult &mls_result);


Suggest move to method.

SergioRAgostinho · 2017-09-03T09:49:42Z

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

+      double F2u = d.z_uv * gw + d.z_v * d.z_u - d.z_uv * w;
+      double F2v = 1 + d.z_vv * gw + d.z_v * d.z_v - d.z_vv * w;
+
+      Eigen::MatrixXd J (2, 2);


This matrix has known size at compile time.

I think the idea behind Sergio's message was that J can be changed to a fixed-sized Eigen type, which will (maybe) make the following matrix inverse computation faster by using a specialized 2x2 implementation (compared to generic size implementation).

Actually, I would expect Eigen to be smart enough to perform runtime dispatching to specialized implementations, but we'd need to dig into the sources to find this out. So the easiest way to ensure we are as efficient as possible is to just switch to Eigen::Matrix2d.

I think the idea behind Sergio's message was that J can be changed to a fixed-sized Eigen type, which will (maybe) make the following matrix inverse computation faster by using a specialized 2x2 implementation (compared to generic size implementation).

It was this exactly. Sorry for being unclear.

SergioRAgostinho · 2017-09-03T09:50:35Z

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

+      J(1, 1) = F2v;
+
+      Eigen::Vector2d err (e1, e2);
+      Eigen::MatrixXd update = J.inverse () * err;


And this one as well it's 2x1

SergioRAgostinho · 2017-09-03T09:52:26Z

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

+  Eigen::Vector4d model_coefficients;
+  pcl::eigen33 (covariance_matrix, eigen_value, eigen_vector);
+  model_coefficients.head<3> ().matrix () = eigen_vector;
+  model_coefficients[3] = 0;


Use the constructor instead

SergioRAgostinho · 2017-09-03T09:53:36Z

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

+  mls_result.curvature = static_cast<float> (covariance_matrix.trace ());
+  // Compute the curvature surface change
+  if (mls_result.curvature != 0)
+    mls_result.curvature = fabsf (float (eigen_value / double (mls_result.curvature)));


Why is the curvature casted to double?

Also, please always use std::abs et al., we had problems with MSVC if I remember correctly.

I believe that is how it was in the original implementation.

Nevertheless. PCL is written C++ and we should always prefer C++ library functions over their C-style counterparts. Besides, as I wrote, we occasionally get bug reports about MSVC choking at some of these functions (e.g. #848, #1548).

Sorry, my comment was in reference to the cast to double.

SergioRAgostinho · 2017-09-03T09:56:16Z

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

+      for (size_t ni = 0; ni < mls_result.num_neighbors; ++ni)
+      {
+        de_meaned[ni][0] = cloud.points[nn_indices[ni]].x - mls_result.mean[0];
+        de_meaned[ni][1] = cloud.points[nn_indices[ni]].y - mls_result.mean[1];


I'm not sure vectorization is properly exploited here. You should use the vector maps for this operation.

Sorry, I don't follow. Can you provide more detail?

Levi-Armstrong · 2017-09-12T11:20:20Z

I will work on the requested changes today.

Levi-Armstrong · 2017-09-12T11:45:02Z

I'm still lacking insight into this, but overall I feel this code is messy. I also don't like that the default precision is double for almost everything.

I aknowledge that my comments are more directed towards the original implementation and not necessarily this PR :/

I am happy to change things from the original implementation, just let me know what it is if it has not already been mentioned above.

Levi-Armstrong · 2017-09-12T19:54:46Z

For the mls class should the setPolynomialFit be removed and just use the order for determining if a polynomial fit should be used like the request in the MLSResults structure? The reason I ask is with the change to MLSResults I had to add logic to the setPolynomialFit and SetPolynomialOrder to insure order get set properly since the order is the only parameter used within the MLSResults to determine if a polynomial fit was used.

Levi-Armstrong · 2017-09-12T20:03:33Z

@SergioRAgostinho and @taketwo I have made most of the requested changes except a few which I had questions about.

…esult

Levi-Armstrong · 2017-09-19T11:38:58Z

@taketwo and @SergioRAgostinho I removed the mls variable polynomial_fit_ and fix style guide related issues. Should I fix item internal to pcl that use the setPolynomialFit (MLS Test and Command Line Tool)?

taketwo · 2017-09-19T11:50:18Z

Should I fix item internal to pcl that use the setPolynomialFit (MLS Test and Command Line Tool)?

You mean to remove usage of depreciated functions within the library? Definitely!

Levi-Armstrong · 2017-09-19T12:11:29Z

OK, The use of setPolynomialFit has been removed.

SergioRAgostinho · 2017-09-19T12:23:34Z

Yeah. I'm somewhat constrained right now, so the moment you find it ready, just mark it accepted. I'll do flash review and merge.

…

Sent from my Phone

On 18 Sep 2017, at 10:23, Sergey Alexandrov ***@***.***> wrote: For the mls class should the setPolynomialFit be removed and just use the order for determining if a polynomial fit should be used I agree that having two functions is confusing. But I'd prefer to keep the old one with a depreciation warning. remove polynomial_fit_ field add depreciation warning update docstring for setPolynomialOrder() Regarding the rest of the PR, from my side it looks good. Please just make sure to format spaces between function name and brackets according to the PCL style guide. @SergioRAgostinho Do you think we can merge this soon so that Levi can move on with other PRs? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

taketwo

LGTM

SergioRAgostinho

I'm just waiting for the tools build job to complete (since it pertains this PR) and then I'll proceed with the merge.

SergioRAgostinho · 2017-09-21T00:56:26Z

FYI, our tools job is exceeding the build time. It needs some intervention.

taketwo · 2018-08-26T19:22:04Z

This qualifies for new feature, doesn't it?

SergioRAgostinho · 2018-08-26T19:53:40Z

Probably. I'll add the label.

PR PointCloudLibrary#1960 deprecated `setPolynomialFit()` but did not update the openni_mls_smoothing app.

keineahnung2345 · 2020-11-06T08:07:35Z

@Levi-Armstrong Hi, I am reading the code of projectPointOrthogonalToPolynomialSurface and there is a reference link in the comment. From the code, it looks like an iterative algorithm, and also Jacobian matrix is used, but I cannot find those things in the reference link. Could you please explain how the function projectPointOrthogonalToPolynomialSurface and the reference link are related? Thank you.

Levi-Armstrong force-pushed the addNewMLSProjection branch from a42b935 to 7d78a8b Compare August 8, 2017 19:56

Add alternate projection method

bb53295

Levi-Armstrong force-pushed the addNewMLSProjection branch from 7d78a8b to bb53295 Compare August 10, 2017 02:57

UnaNancyOwen requested a review from VictorLamoine August 12, 2017 08:17

UnaNancyOwen assigned SergioRAgostinho and VictorLamoine Aug 12, 2017

UnaNancyOwen removed the request for review from VictorLamoine August 12, 2017 08:18

SergioRAgostinho unassigned VictorLamoine Aug 22, 2017

SergioRAgostinho reviewed Aug 22, 2017

View reviewed changes

taketwo reviewed Aug 29, 2017

View reviewed changes

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

SergioRAgostinho requested changes Sep 3, 2017

View reviewed changes

Move MLS methods inside MLSResult struct

396bbd7

Levi-Armstrong force-pushed the addNewMLSProjection branch from ccc98d9 to 44f59da Compare September 12, 2017 19:58

Levi-Armstrong force-pushed the addNewMLSProjection branch 2 times, most recently from 0fa5ea3 to 07a5533 Compare September 14, 2017 12:22

create MLSResult::projectPoint and MLSResult::projectQueryPoint

491cc44

Levi-Armstrong force-pushed the addNewMLSProjection branch from 07a5533 to 491cc44 Compare September 14, 2017 12:36

Add inline keyword to MLSResult and move computeMLSSurface in to MLSR…

9f54618

…esult

Levi-Armstrong added 2 commits September 18, 2017 14:16

Remove mls polynomial_fit_ variable

9a3b87e

Fix style guide issues

b34691d

Remove use of depreciated function setPolynomialFit in mls

c60e97c

Levi-Armstrong force-pushed the addNewMLSProjection branch from 55fd8be to c60e97c Compare September 19, 2017 12:10

taketwo approved these changes Sep 20, 2017

View reviewed changes

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

taketwo added this to the pcl-1.9.0 milestone Sep 20, 2017

SergioRAgostinho approved these changes Sep 21, 2017

View reviewed changes

SergioRAgostinho merged commit 079cf24 into PointCloudLibrary:master Sep 21, 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 module: tools labels Aug 26, 2018

SergioRAgostinho added the changelog: new feature Meta-information for changelog generation label Aug 26, 2018

taketwo added a commit to taketwo/pcl that referenced this pull request Sep 9, 2018

Do not use deprecated method in openni_mls_smoothing app

5d984ef

PR PointCloudLibrary#1960 deprecated `setPolynomialFit()` but did not update the openni_mls_smoothing app.

taketwo mentioned this pull request Sep 9, 2018

Do not use deprecated method in openni_mls_smoothing app #2421

Merged

taketwo added a commit to taketwo/pcl that referenced this pull request Sep 9, 2018

Do not use deprecated method in openni_mls_smoothing app

08c0ab7

PR PointCloudLibrary#1960 deprecated `setPolynomialFit()` but did not update the openni_mls_smoothing app.

SergioRAgostinho added the changelog: deprecation Meta-information for changelog generation label Sep 25, 2018

SergioRAgostinho changed the title ~~Add new mls projection method~~ Add new mls projection method. Deprecated MovingLeastSquares::setPolynomialFit(). Sep 25, 2018

mvieth mentioned this pull request Oct 15, 2022

[Surface]Using MovingLeastSquares results in a segfault within performUpsampling function #5471

Closed

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

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

Conversation

Levi-Armstrong commented Aug 7, 2017

Levi-Armstrong commented Aug 11, 2017

taketwo commented Aug 11, 2017

UnaNancyOwen commented Aug 12, 2017 • edited Loading

SergioRAgostinho commented Aug 15, 2017

SergioRAgostinho left a comment • 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

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

Choose a reason for hiding this comment

SergioRAgostinho commented Sep 3, 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

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 Sep 12, 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

Levi-Armstrong commented Sep 12, 2017

Levi-Armstrong commented Sep 12, 2017

Levi-Armstrong commented Sep 12, 2017

Levi-Armstrong commented Sep 12, 2017

Levi-Armstrong commented Sep 19, 2017

taketwo commented Sep 19, 2017

Levi-Armstrong commented Sep 19, 2017

SergioRAgostinho commented Sep 19, 2017 via email

taketwo left a comment

Choose a reason for hiding this comment

SergioRAgostinho left a comment

Choose a reason for hiding this comment

SergioRAgostinho commented Sep 21, 2017

taketwo commented Aug 26, 2018

SergioRAgostinho commented Aug 26, 2018

keineahnung2345 commented Nov 6, 2020

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

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

UnaNancyOwen commented Aug 12, 2017 •

edited

Loading

SergioRAgostinho left a comment •

edited

Loading

taketwo Sep 12, 2017 •

edited

Loading