kzampog / cilantro

A lean C++ library for working with point cloud data
MIT License
1.01k stars 206 forks source link

Add symmetric transform estimation to ICP #34

Closed wannesvanloock closed 5 years ago

wannesvanloock commented 5 years ago

This is a proposal to add symmetric transform estimation to the existing CombinedMetricSingleTransformICP interface. Adding a new class probably causes significant code duplication but doesn't have the branching. Not sure if this is the way to go?

Also adds the offsetting by the mean to the transform estimation. We still deduct the mean every call since the ICP implementation uses ConstVectorSetMatrixMap. An alternative would be to store a centered copy of the data in the class.

Some quick preliminary tests reveal that the new method is more accurate (~order of magnitude on my example) and may convergence in less or equally many iterations as normal based ICP, depending on weighting of objectives.

Nit: @kzampog what formatting guideline are you using? I feel that I sometimes deviate from it, but would prefer consistency with the rest of the code base.

kzampog commented 5 years ago

Perfect, thanks! I will take a closer look in the following days!

Regarding the formatting style, I use 4 space indentation and (not too fanatically) try to not exceed a 120 char line limit. I put opening braces in the existing line, unless it's a function definition whose arguments span multiple lines, in which case braces have their own line. I indent 2 levels for initializer lists (if they don't fit in the same line). I put function return types in the same line as the header, unless they are very long (e.g. template expressions), in which case they can have their own line. Of course, I am open to suggestions! :)