Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add ability to cache mls results #1952

Merged
merged 4 commits into from
Aug 9, 2017
Merged

Add ability to cache mls results #1952

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e8c036e
Add ability to cache mls results
Levi-Armstrong Jul 28, 2017
b8e0c56
Move struct MLSResult outside the MoveingLeastSquares class.
Levi-Armstrong Aug 1, 2017
21721cc
Add documentation for MLSResults and cache MLS results by default
Levi-Armstrong Aug 4, 2017
90b1b7c
Make MovingLeastSqures getter methods const
Levi-Armstrong Aug 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 24 additions & 17 deletions surface/include/pcl/surface/impl/mls.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,24 +116,30 @@ pcl::MovingLeastSquares<PointInT, PointOutT>::process (PointCloudOut &output)
float tmp = static_cast<float> (search_radius_ / 2.0f);
boost::uniform_real<float> uniform_distrib (-tmp, tmp);
rng_uniform_distribution_.reset (new boost::variate_generator<boost::mt19937&, boost::uniform_real<float> > (rng_alg_, uniform_distrib));

mls_results_.resize (1); // Need to have a reference to a single dummy result.

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

@SergioRAgostinho SergioRAgostinho Aug 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

break;
}
default:
{
mls_results_.resize (1); // Need to have a reference to a single dummy result.
break;
}
}

if (cache_mls_results_)
{
mls_results_.resize (input_->size ());
}
else
{
mls_results_.resize (1); // Need to have a reference to a single dummy result.
}

// Perform the actual surface reconstruction
performProcessing (output);

Expand Down Expand Up @@ -271,6 +277,11 @@ pcl::MovingLeastSquares<PointInT, PointOutT>::computeMLSPointNormal (int index,
P_weight_Pt.llt ().solveInPlace (c_vec);
}

if (cache_mls_results_)
{
mls_result = MLSResult (point, plane_normal, u_axis, v_axis, c_vec, static_cast<int> (nn_indices.size ()), curvature);
}

switch (upsample_method_)
{
case (NONE):
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to leave the empty case blocks here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I would go with default.

break;
}
}
Expand Down Expand Up @@ -494,7 +502,7 @@ pcl::MovingLeastSquares<PointInT, PointOutT>::performProcessing (PointCloudOut &
// Get a plane approximating the local surface's tangent and project point onto it
int index = (*indices_)[cp];

if (upsample_method_ == VOXEL_GRID_DILATION || upsample_method_ == DISTINCT_CLOUD)
if (cache_mls_results_)
mls_result_index = index; // otherwise we give it a dummy location.

computeMLSPointNormal (index, nn_indices, nn_sqr_dists, projected_points, projected_points_normals, *corresponding_input_indices_, mls_results_[mls_result_index]);
Expand Down Expand Up @@ -557,7 +565,7 @@ pcl::MovingLeastSquaresOMP<PointInT, PointOutT>::performProcessing (PointCloudOu
int index = (*indices_)[cp];
size_t mls_result_index = 0;

if (upsample_method_ == VOXEL_GRID_DILATION || upsample_method_ == DISTINCT_CLOUD)
if (this->cache_mls_results_)
mls_result_index = index; // otherwise we give it a dummy location.

this->computeMLSPointNormal (index, nn_indices, nn_sqr_dists, projected_points[tn], projected_points_normals[tn], corresponding_input_indices[tn], this->mls_results_[mls_result_index]);
Expand Down Expand Up @@ -696,14 +704,13 @@ pcl::MovingLeastSquares<PointInT, PointOutT>::performUpsampling (PointCloudOut &
}

//////////////////////////////////////////////////////////////////////////////////////////////
template <typename PointInT, typename PointOutT>
pcl::MovingLeastSquares<PointInT, PointOutT>::MLSResult::MLSResult (const Eigen::Vector3d &a_mean,
const Eigen::Vector3d &a_plane_normal,
const Eigen::Vector3d &a_u,
const Eigen::Vector3d &a_v,
const Eigen::VectorXd a_c_vec,
const int a_num_neighbors,
const float &a_curvature) :
pcl::MLSResult::MLSResult (const Eigen::Vector3d &a_mean,
const Eigen::Vector3d &a_plane_normal,
const Eigen::Vector3d &a_u,
const Eigen::Vector3d &a_v,
const Eigen::VectorXd a_c_vec,
const int a_num_neighbors,
const float &a_curvature) :
mean (a_mean), plane_normal (a_plane_normal), u_axis (a_u), v_axis (a_v), c_vec (a_c_vec), num_neighbors (a_num_neighbors),
curvature (a_curvature), valid (true)
{
Expand Down
64 changes: 42 additions & 22 deletions surface/include/pcl/surface/mls.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I will remove the note.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


MLSResult (const Eigen::Vector3d &a_mean,
const Eigen::Vector3d &a_plane_normal,
const Eigen::Vector3d &a_u,
const Eigen::Vector3d &a_v,
const Eigen::VectorXd a_c_vec,
const int a_num_neighbors,
const float &a_curvature);

Eigen::Vector3d mean, plane_normal, u_axis, v_axis;
Eigen::VectorXd c_vec;
int num_neighbors;
float curvature;
bool valid;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I will add the documentation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot

};

/** \brief MovingLeastSquares represent an implementation of the MLS (Moving Least Squares) algorithm
* for data smoothing and improved normal estimation. It also contains methods for upsampling the
* resulting cloud based on the parametric fit.
Expand Down Expand Up @@ -106,6 +128,7 @@ namespace pcl
upsampling_radius_ (0.0),
upsampling_step_ (0.0),
desired_num_points_in_radius_ (0),
cache_mls_results_ (false),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we not cache by default?

mls_results_ (),
voxel_size_ (1.0),
dilation_iteration_num_ (0),
Expand Down Expand Up @@ -283,6 +306,22 @@ namespace pcl
inline int
getDilationIterations () { return dilation_iteration_num_; }

/** \brief Set wether the mls results should be stored for each point in the input cloud
* \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; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/** \brief Get the cache_mls_results_ value (True if the mls results should be stored, otherwise false). */
inline bool
getCacheMLSResults () const { return cache_mls_results_; }

/** \brief Get the MLSResults for input cloud
* \note The results are only stored if setCacheMLSResults(true) was called or when using the upsampling method DISTINCT_CLOUD or VOXEL_GRID_DILATION.
* \note This vector is align with the input cloud indicies, so use getCorrespondingIndices to get the correct results when using output cloud indicies.
*/
inline const std::vector<MLSResult>& getMLSResults() const { return mls_results_; }

/** \brief Base method for surface reconstruction for all points given in <setInputCloud (), setIndices ()>
* \param[out] output the resultant reconstructed surface model
*/
Expand Down Expand Up @@ -341,28 +380,8 @@ namespace pcl
*/
int desired_num_points_in_radius_;


/** \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
*/
struct MLSResult
{
MLSResult () : mean (), plane_normal (), u_axis (), v_axis (), c_vec (), num_neighbors (), curvature (), valid (false) {}

MLSResult (const Eigen::Vector3d &a_mean,
const Eigen::Vector3d &a_plane_normal,
const Eigen::Vector3d &a_u,
const Eigen::Vector3d &a_v,
const Eigen::VectorXd a_c_vec,
const int a_num_neighbors,
const float &a_curvature);

Eigen::Vector3d mean, plane_normal, u_axis, v_axis;
Eigen::VectorXd c_vec;
int num_neighbors;
float curvature;
bool valid;
};
/** \brief True if the mls results for the input cloud should be stored */
bool cache_mls_results_;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/** \brief Stores the MLS result for each point in the input cloud
* \note Used only in the case of VOXEL_GRID_DILATION or DISTINCT_CLOUD upsampling
Expand Down Expand Up @@ -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_;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

using MovingLeastSquares<PointInT, PointOutT>::VOXEL_GRID_DILATION;
using MovingLeastSquares<PointInT, PointOutT>::DISTINCT_CLOUD;

Expand Down