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

Minor modifications for better usuability #184

Merged
merged 11 commits into from
May 5, 2024
Merged

Minor modifications for better usuability #184

merged 11 commits into from
May 5, 2024

Conversation

LimHyungTae
Copy link
Member

Hello @jingnanshi , I'd like to do PR.

I modified two things:

  • Add 'const' to the inputs for the Matcher class
  • Add a Quatro option to make Quatro available on the Python wrapper

@LimHyungTae
Copy link
Member Author

Also, I want to change pcl::NormalEstimation<pcl::PointXYZ, pcl::Normal> normalEstimation; to pcl::NormalEstimationOMP<pcl::PointXYZ, pcl::Normal> normalEstimation; to make FPFH faster! (Since in the 64-channel LiDAR point cloud, using OMP boosts the speed)

@LimHyungTae
Copy link
Member Author

I also enable getting normal vectors, which is the side-output of the procedure of FPFH. By doing so, the TEASER++ library can be more easily integrated with ROBIN

@LimHyungTae
Copy link
Member Author

Also, I fix the equal error in #177

@@ -11,6 +11,7 @@
#include <boost/smart_ptr/shared_ptr.hpp>
#include <pcl/features/fpfh.h>
#include <pcl/features/fpfh_omp.h>
#include <pcl/features/normal_3d_omp.h>
Copy link
Member

Choose a reason for hiding this comment

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

Is this header always there? Will there be any PCL installation that does not have this header?

Copy link
Member Author

@LimHyungTae LimHyungTae May 3, 2024

Choose a reason for hiding this comment

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

I double-checked it, and I found that it is stabilized since PCL 1.8 version as follow:
https://github.com/PointCloudLibrary/pcl/releases/tag/pcl-1.8.1
Thus, I think it's fine because in our CMakelLists.txt, we mention "1.8" version when finding the package.

In addition, until the latest version (1.14), the PCL library still supports normal_3d_omp as follows:
https://pcl-docs.readthedocs.io/en/latest/api/program_listing_file_pcl_features_include_pcl_features_normal_3d_omp.h.html

Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -59,3 +59,5 @@ void teaser::FPFHEstimation::compute(pcl::PointCloud<pcl::FPFHSignature33>& outp
fpfh_estimation_->compute(output_cloud);
}
void teaser::FPFHEstimation::setRadiusSearch(double r) { fpfh_estimation_->setRadiusSearch(r); }

pcl::PointCloud<pcl::Normal> teaser::FPFHEstimation::getNormals() { return *normals_; }
Copy link
Member

Choose a reason for hiding this comment

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

need to add a new line at the end of the file (I think clang format can do this automatically?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, there's

InsertFinalNewline: 'yes'

option. May I add that option to .clang_format and add an additional line for all files?

Copy link
Member Author

Choose a reason for hiding this comment

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

But I've noticed that other files also have no blank line; could you tell me why the new line at the end is needed?

Copy link
Member

Choose a reason for hiding this comment

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

Yes about clang format. If the other files do not have newline character at the end, then we should also add them. See https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-newline

@jingnanshi
Copy link
Member

All good except the missing new line character

@LimHyungTae
Copy link
Member Author

I did not know the importance of the new line at the end of the files. Here are my modifications:

@jingnanshi
Copy link
Member

@LimHyungTae thanks! merging

@jingnanshi jingnanshi merged commit 6ceb9c8 into MIT-SPARK:master May 5, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants