Add IncrementalICP #1202
Add IncrementalICP #1202
Conversation
|
Hi v4hn, could you please adopt this to the style guide (spaces in front of parenthesis).. |
v4hn
force-pushed
the
incremental_icp
branch
3 times, most recently
from
d9299e5 to
9da16cd
Compare
|
the style should be fixed now.
|
| registerCloud (const PointCloudConstPtr& cloud, const Matrix4& delta_estimate = Matrix4::Identity ()); | ||
|
|
||
| /** \brief Get estimated transform between the last two registered clouds */ | ||
| inline Matrix4 |
There was a problem hiding this comment.
Do not return fixed-size vectorizable Eigen objects by value. Rather add an output (reference) argument. Same for the other function.
There was a problem hiding this comment.
I know this is a common pattern in PCL,
but it also makes usage more cumbersome imho.
Where the explicit return of the type allows to write
'''f( iicp.getDeltaTransform() )'''
your version requires
'''Matrix4 delta; iicp.getDeltaTransform(delta); f( delta )'''
We're talking about 64 bytes of data here, which might
have to be copied once iff the compiler ignores the
inline keyword and needs the explicit return value.
If you require it, I can change it,
but I consider this to be premature optimization.
There was a problem hiding this comment.
Sorry, I put the wrong link, this one was intended.
passing fixed-size vectorizable Eigen objects by value is not only inefficient, it can be illegal or make your program crash
Yes, usage is more cumbersome, but I would not neglect the possibility of producing a program that may crash ;)
There was a problem hiding this comment.
There was a problem hiding this comment.
AFAIK compilers may or may not respect the inline keyword, it is only a hint to the compiler.
I didn't look at how the interface is defined in other registration classes. Since there is such a big body of code that is implemented this way, and since there are no problems reported so far, I'd say let's keep this way.
There was a problem hiding this comment.
On Mon, Apr 13, 2015 at 02:34:00PM -0700, Sergey Alexandrov wrote:
I'd say let's keep this way.
Ok, that should be it then. Thanks again for your feedback.
I'll go to bed then and hope to see this merged soon as @JochenSp
already gave his +1.
I'm pretty sure many people implemented that over and over again up to now...
This simplifies the code and gives a good example for using the class
|
@jspricke No more comments from my side, please merge if you think it's ready. |
This is a standard use of ICP and in this lab we've implemented it
at least three times (probably more often...). To avoid having to think
about which way to multiply the delta with the old transform
and to clean up a bit of code, I propose to merge this into PCL to
use it directly.