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.

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

Correct setting of is_dense flag in SegmentDifferences. Deprecate unused parameter in method. #2380

Merged
merged 2 commits into from
Jul 20, 2018

Conversation

ThorstenHarter
Copy link
Contributor

  • is_dense flag must be set after copyPointCloud, otherwise it will be overwritten.
  • Correct argumentation why is_dense should be set to true (because it IS true, not because we don't want to check).
  • Remove redundant copying of attributes, they will be copied in copyPointCloud.
  • Remove unused parameter tgt in getPointCloudDifference, it only needs the kd-tree.

Flag must be set after copyPointCloud, otherwise it will be overwritten. Remove unused parameter tgt and redundant copying of attributes.
@taketwo
Copy link
Member

taketwo commented Jul 19, 2018

All makes sense to me, thanks. However we'll need to keep around the old function signature and add a depreciation warning. Something like this:

template <typename PointT> 
PCL_DEPRECATED ("getPointCloudDifferences() does not use the tgt parameter, thus it is
                 deprecated and will be removed in future releases.")
inline void getPointCloudDifference (
   const pcl::PointCloud<PointT> &src, const pcl::PointCloud<PointT> &tgt, 
   double threshold, const boost::shared_ptr<pcl::search::Search<PointT> > &tree,
   pcl::PointCloud<PointT> &output)
{
    getPointCloudDifferences<PointT> (src, pcl::PointCloud<PointT> (), threshold, tree, output);
}

@ThorstenHarter
Copy link
Contributor Author

Hi Sergey, I have made the changes you requested, it's now possible again to call the function with the old signature.

I cannot see the deprecated warning with my GCC 4.9 and VS 2015 compilers however. We use the same macro and it stopped working when changing from VS 2010 to 2015.
I'm looking forward to C++14 where you can use the [[deprecated]] attribute.

@taketwo taketwo merged commit 310b93f into PointCloudLibrary:master Jul 20, 2018
@taketwo
Copy link
Member

taketwo commented Jul 20, 2018

Thanks. I experimented with deprecation warnings last month on GCC 5.5; it worked. So at least some users will see them.

@SergioRAgostinho SergioRAgostinho changed the title Correct setting of is_dense flag in SegmentDifferences Correct setting of is_dense flag in SegmentDifferences. Deprecate unused parameter in method. Aug 25, 2018
{
getPointCloudDifference<PointT> (src, pcl::PointCloud<PointT>(), threshold, tree, output);
}

Copy link
Member

@SergioRAgostinho SergioRAgostinho Aug 25, 2018

Choose a reason for hiding this comment

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

Sorry to rock the boat this late. Isn't the idea behind this deprecation accept the extra tgt parameter but invoke the version without it like shown below?

  template <typename PointT>
  PCL_DEPRECATED("getPointCloudDifference() does not use the tgt parameter, thus it is deprecated and will be removed in future releases.")
  inline void getPointCloudDifference (
      const pcl::PointCloud<PointT> &src,
      const pcl::PointCloud<PointT> &, // removed parameter to suppress warning
      double threshold,
      const boost::shared_ptr<pcl::search::Search<PointT> > &tree,
      pcl::PointCloud<PointT> &output)
  {
    getPointCloudDifference<PointT> (src, threshold, tree, output); // without the extra parameter
  }

Isn't the method recursively calling itself with the current change?

Copy link
Member

Choose a reason for hiding this comment

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

Very good catch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants