4pcs registration method ...

[theilerp]

...as proposed by Aiger et al. (2007, 4-Points Congruent Sets for Robust Surface Registration) and Theiler et al. (2014, Keypoint-based 4-Points Congruent Sets – Automated marker-less registration of laser scans).

Commit is together with a fast transformation estimation method to speed up the calculation using a minimum number of 3 points. This method simplifies the calculation of transformation parameters when dealing with a 3 point planar case.

I do realize, that a comparable contribution (fpcs by stheg) was done in June 2014, but since the additional method k-4pcs is inheriting a lot from its parent 4pcs, this commitment is only useful in combination.

[taketwo]

Hi, thanks for contributing!

Could you please explain in greater detail how this pull request is related with #726? Is this supposed to replace it? Was the code copied or derived from that contribution?

[theilerp]

Hi Sergey

Sadly, it is absolutely independent of #726 and it probably has a quite large overlap. I had the following reasons to anyway contribute my code:

• I've worked with and investigated on 4pcs since 2 years and developed a further algorithm K-4pcs which is specialized for registering static terrestrial laser scans.
• Because I wanted to understand and be able to inverstigate on 4pcs, I rewrote the original source code from aiger, mitra and cohen-or. As I was working right from the start in the scope of PCL I wrote this code to match the requirements of PCL. But since I started with very limited programming skill, I first hesitated to publish it. After optimizing it over the last 2 years, I'm quite confident, that it is a very useful and good implementation. I also received the official permition from mitra to publish 4pcs in the scope of PCL.
• A further and for me most important reason to contribute this impelementation for 4pcs is, that my newly developed K-4pcs is based on 4pcs and thus inherits many major parts. I thus implemented 4pcs in such a way to be able to accomplish this inheritence. Sadly this would not work with Four-point congruent sets algorithm (FPCS) #726 and thus K-4pcs would have to be rewritten completely.
• The only big difference between my implementation and the original suggested method is, that is now runs in parallel using OpenMP. Since the method is generally based on independent random sampling runs, the generated speed up is close to the number of cores.

I hope this clarifies things
Cheers
Pascal

[taketwo]

Pascal, thanks for the detailed explanation. I think we should adopt your contribution since a) it is more complete and feature-rich; b) the author of the other one does not reply to our comments anyways.

I skimmed through the code already and I think it is in a good shape, though I had several questions here and there. But before we go into details, I would like to ask you to split this pull request in two parts (original and keypoint-based method) so it is easier to review and merge. Also we will need a few unit tests for 4PCS/K4PCS.

[theilerp]

Sergey, I split up the two pull requests. Although it was a little confusing to do this and it has to be mentioned, that the other request only works if this one has already been merged, because k-4pcs inherits a lot for 4pcs and also uses the transformation estimation based on 3 points...

[VictorLamoine]

Sergey probably meant you should split this pull request into multiple (logical) commits; not split the pull request into multiple pull requests 😉

For example the first commit adds a class, the second commits adds tests for the first class etc..

[taketwo]

No no, I actually meant multiple pull requests. When a request adds/changes 3k+ lines it is hard to review and discuss it, as well as to say "yes" in the end and merge.

Although it was a little confusing to do this and it has to be mentioned, that the other request only works if this one has already been merged

Sure, I understand this. We'll simply forget about the second one until the first one is merged.

[theilerp]

Not sure now, is it fine like that or not? Splitting up the commit itself will be quite hard, because there are no changes to existing code which I could split but rather a complete new feature.

About the unit tests: because this is my first contribution to such a project, I don't know what unit tests are. Please help me out here. What exactly do you need?

[taketwo]

Not sure now, is it fine like that or not?

It's fine.

I will review the code and leave inline comments. Unfortunately I am quite busy with other things now, so maybe I will only have time to dig into this next week.

I don't know what unit tests are. Please help me out here. What exactly do you need?

We need a small test program that would instantiate your class, setup it's parameters, feed it some input, and verify that the output is as expected. Just have a look inside test/ folder, we have 100+ unit tests for classes in PCL there.

[theilerp]

I already have a simple test program based on a small data set (2 x ply-files, 250kb), but I'm struggling with implementing this in the gtest environment to get a functioning unit test.

Any help would be appreciated, but it may be the easiest solution if I just commit the unit test (cpp file and header with method parameters), although I can not test it on my computer and it may not run like it is now. Or are there any other suggestions?

[taketwo]

I'm struggling with implementing this in the gtest environment to get a functioning unit test.
Any help would be appreciated

If you have concrete questions or problems please don't hesitate to ask.

[theilerp]

Sergey, before you go too much into coding detail, I just realized (during setting up the unit test, which by the way should soon work) that the complete method should be moved to a registration method thus inheriting from the registration base class rather than from correspondence estimation one. The goal of the method is comparable to the SAC-IA which is also placed there. Do you agree?

I did some checks and realized that the transfer is not too complicated and I should be able to do it by tomorrow. I would then update the commits. I have just one question since the base class of registration provides a lot of setter and getter methods for protected member which are not needed in 4pcs, do I have to overwrite them with PCL_DEPRECATED return info or can I just ignore them?

Cheers
Pascal

[jspricke]

Looks like an encoding problem?

[theilerp]

I don't get the point here, can you clarify what you mean?

[jspricke]

Github is showing only a question mark, so I guess it's not ASCII.

[theilerp]

oh yeah, it was just a degree symbol. I have replaced it with the actual word "degree"

[jspricke]

Looks like a nice contribution, thanks! Back when we planned the registration module, we discussed breaking the API into estimators, matches, and so on (see http://pointclouds.org/documentation/tutorials/registration_api.php). Wouldn't it make sense to do the same here? I know it would be a lot of work, just asking.

[taketwo]

Please drop Willow Garage, they have nothing to do with the code.

[theilerp]

I will drop that one, must have been an old copy of the license agreement

[theilerp]

@jspricke The question about splitting up the method into a correspondence estimation and a matching part is not easy to answer, especially because personally, I consider these two things unseperable. In case of 4pcs, I think it is too strongly connected. I even had placed the method first in the area of correspondence estimation (worked also quite well), but then again it already solves the matching and provides the result of the initial alignment between source<->target. Consider the following situation:

Given a base set of four points, the method searches for a congruent 4 point set in the target cloud. This step could be considered as a correspondence estimation method, but the result is not a single set of correspondences but rather a series of possible 4 point matches, which have to be further evaluated. This is done by deriving for each possible match the resulting transformation parameters and calculating the resulting overlap between the source and target (or in case of k-4pcs an extended evaluation criteria). Thus to be able to return the "correct" (or most probable) correspondences, one already has to carry out the actual matching and gets the initial alignment in return.

In conclusion, I would keep it as an unseperated, single method and place it at the same level as the initial aligment method of rusu (SampleConsenusInitialAlignment).

[jspricke]

Leaving it all in one class sounds fine then, thanks for clearing that up.

[taketwo]

Do not pass Eigen structures by value. Here const& shoud be used.

Actually, this also counts for the correspondences argument, because that is an STL vector.

[theilerp]

The argument constructor was a left-over from previous versions, thats why I didn't check that, but since it may be useful, I keep it and replaced the arguments with const & (except of course the float argument).

[taketwo]

Hi Pascal, thanks for polishing up the contribution. I think it is in a great shape now. @jspricke please merge if you have no further comments.

Speaking about doxygen warnings... they are not critical of course. And it's true that the existing code produces tons of them. But I can understand Victor in his attempts to prevent new warnings from being introduced, considering that he has invested quite some effort in cleaning them up some time ago. Personally, I have similar feeling against compiler warnings, which I also fought before.

[VictorLamoine]

I've never seen a PCL class where getter functions have documented return types...

It's not forbidden to do better than what the others have done 😉

Here are the last warnings:

ia_fpcs.h:83: warning: Member Ptr (typedef) of class pcl::registration::FPCSInitialAlignment is not documented.
ia_fpcs.h:84: warning: Member ConstPtr (typedef) of class pcl::registration::FPCSInitialAlignment is not documented.
ia_fpcs.h:86: warning: Member KdTree (typedef) of class pcl::registration::FPCSInitialAlignment is not documented.
ia_fpcs.h:87: warning: Member KdTreePtr (typedef) of class pcl::registration::FPCSInitialAlignment is not documented.
ia_fpcs.h:88: warning: Member KdTreeReciprocal (typedef) of class pcl::registration::FPCSInitialAlignment is not documented.
ia_fpcs.h:89: warning: Member KdTreeReciprocalPtr (typedef) of class pcl::registration::FPCSInitialAlignment is not documented.
ia_fpcs.h:91: warning: Member PointCloudTarget (typedef) of class pcl::registration::FPCSInitialAlignment is not documented.
ia_fpcs.h:92: warning: Member PointCloudSource (typedef) of class pcl::registration::FPCSInitialAlignment is not documented.
ia_fpcs.h:93: warning: Member PointCloudSourcePtr (typedef) of class pcl::registration::FPCSInitialAlignment is not documented.
ia_fpcs.h:94: warning: Member PointCloudSourceIterator (typedef) of class pcl::registration::FPCSInitialAlignment is not documented.
ia_fpcs.h:96: warning: Member Normals (typedef) of class pcl::registration::FPCSInitialAlignment is not documented.
ia_fpcs.h:97: warning: Member NormalsConstPtr (typedef) of class pcl::registration::FPCSInitialAlignment is not documented.
ia_fpcs.h:99: warning: Member MatchingCandidate (typedef) of class pcl::registration::FPCSInitialAlignment is not documented.
ia_fpcs.h:100: warning: Member MatchingCandidates (typedef) of class pcl::registration::FPCSInitialAlignment is not documented.
ia_fpcs.h:362: warning: parameters of member pcl::registration::FPCSInitialAlignment::bruteForceCorrespondences are not (all) documented
ia_fpcs.h:409: warning: expected whitespace after ] command
ia_fpcs.h:406: warning: argument 'in' of command @param is not found in the argument list of pcl::registration::FPCSInitialAlignment< PointSource, PointTarget, NormalT, Scalar >::handleMatches(const std::vector< int > &base_indices, std::vector< std::vector< int > > &matches, MatchingCandidates &candidates)
ia_fpcs.h:406: warning: The following parameters of pcl::registration::FPCSInitialAlignment::handleMatches(const std::vector< int > &base_indices, std::vector< std::vector< int > > &matches, MatchingCandidates &candidates) are not documented:
  parameter 'matches'
ia_fpcs.h:333: warning: argument 'ratio' of command @param is not found in the argument list of pcl::registration::FPCSInitialAlignment< PointSource, PointTarget, NormalT, Scalar >::selectBaseTriangle(std::vector< int > &base_indices)
matching_candidate.h:92: warning: Member operator()(MatchingCandidate const &left, MatchingCandidate const &right) (function) of class pcl::registration::by_score is not documented.
transformation_estimation_3point.h:64: warning: Member Ptr (typedef) of class pcl::registration::TransformationEstimation3Point is not documented.
transformation_estimation_3point.h:65: warning: Member ConstPtr (typedef) of class pcl::registration::TransformationEstimation3Point is not documented.
transformation_estimation_3point.h:67: warning: Member Matrix4 (typedef) of class pcl::registration::TransformationEstimation3Point is not documented.

If you want to test the doc, use this doxy file

.
├── documentation
│   ├── html
│   └── test.doxy
└── src
    ├── ia_fpcs.h
    ├── ia_fpcs.hpp
    ├── matching_candidate.h
    └── transformation_estimation_3point.h

find . -iname "*.doxy" -exec doxygen {} 1> /dev/null \;

[theilerp]

Of course not :-). I will correct the warnings regarding the method parameters a.s.a.p.

Considering the typedefs, do they also need a documentation? Are there any PCL classes, where I can find examples? (I don't wont the reinvent the wheel ;-))

[VictorLamoine]

Yes if you want to silent warnings, I usually do: (The typedefs don't really need to be explained IMO)
/** \brief Convenience typedef */

It leads to a lot of useless lines though.
A better solution is to deactivate Doxygen for the code block:

/** \cond */
typedef int my_int;
                         // Lots of typedefs...
/** \endcond */

I think it's the best option

[taketwo]

Perhaps doxygen has a global setting for that?

[VictorLamoine]

Looks like no: http://www.stack.nl/~dimitri/doxygen/manual/config.html

[theilerp]

Allright, I have updated the doxy comments. Can you please recheck if there are remaining warnings?

[VictorLamoine]

That's a perfect 👍

[jspricke]

picky mode this could be removed.

[theilerp]

Corrections applied. Anything else?

I will squash all the commits before merging, but keep them for the moment until fine polishing is finished.

[jspricke]

👍 Thanks for the work!

[VictorLamoine]

👍 for me

[taketwo]

👍

[VictorLamoine]

@theilerp your pull request introduces a compiling error with clang, can you fix it?
https://travis-ci.org/PointCloudLibrary/pcl/jobs/46999555#L1479

[theilerp]

Since this pull request is already closed, I opened up a new one for bugfixes (#1089)

[VictorLamoine]

@theilerp other compiling errors:
http://www.pcl-users.org/PCL-Compile-Errors-td4036944.html

I'll tell him to update his copy and try again.

[VictorLamoine]

Might seem like a dumb question but how come this code is compiled, it's not referenced in any CMake file?!
@SergioRAgostinho: http://www.pcl-users.org/QUESTION-ABOUT-FPCS-BUG-REPORT-td4040695.html

Even if it's compiled, it's not installed, so it's not very useful 😄

[taketwo]

This is not compiled in the normal build, only when the tests are enabled, because the corresponding test includes the headers. Of course, goes without saying, this contribution is useless without install instructions :D I'll send a pull request.

[theilerp]

Oh crab, sorry, that was clearly my mistake. Thanks for cleaning it up...