Rework of eigen features in common

[VictorLamoine]

Most of the inline definitions were moved into the hpp file.

Some existing functions are now templated :

• getTransformation
• getTranslationAndEulerAngles
• getEulerAngles

Added : (all templated)

• transformPoint
• transformVector
• transformLine
• transformPlane
• checkCoordinateSystem
• transformBetween2CoordinateSystems

No change in the API
Test unit updated

[jspricke]

Hi Victor, thanks for the work. Having all changes in one commit makes it hard to review. Could you split it up? Maybe have one commit per function move and an extra one for the unit tests (because the functions have been there before), thanks for them btw! Also, you are changing quite a number of whitespaces, can you exclude these, as log as you are not touching the lines anyhow, because it brakes git blame ;).

[VictorLamoine]

I broke the commits pretty much as you said, one commit = one function
I updated the test unit every time I could. This way the code should be compilable at every commit.

There are bunch of errors in this, I'm correcting this right now

[taketwo]

Victor, could you please also split this into at least two pull requests? One would rearrange the stuff, and another would add new functionality. Because right now (even though everything is is its own commit) in the "Files changed" tab I see 1.5K lines added and 600 lines deleted, which is a bit hard to get grasp of.

[jspricke]

@taketwo just click the single commits ;). For me it's fine like that.

[taketwo]

@jspricke Hmm... you're right!

[jspricke]

@taketwo any comments? Otherwise I'm fine with this.

[taketwo]

Out of curiosity: why do you use this representation of coordinate systems? Wouldn't it be simpler to use homogeneous transformation matrices? Then it is trivial to verify that a system is valid and transformation boils down to a single matrix multiplication.

[VictorLamoine]

In my work I use coordinate system built with plane normals; that's why I use this representation. It's not the best representation but it works fine and is quite more intuitive than matrices.

Maybe someone in the future will implement that !

I don't get why there are functions to invert matrices, compute determinants etc..
For example invert3x3Matrix is useless in my opinion because already available in Eigen.

The Travis build failed again; I built PCL on my side and the test unit went fine.

[jspricke]

• Victor Lamoine notifications@github.com [2014-05-15 08:35]:

  I don't get why there are functions to invert matrices, compute determinants etc..
  For example invert3x3Matrix is useless in my opinion because already available in Eigen.

Either it was not available in Eigen back then or our method is faster.
As we don't seem to use this one, I would propose to simple delete it.

[rbrusu]

That method is being used in external code btw, so please do not delete it. Also, it is significantly faster than its Eigen counterpart.

[taketwo]

@VictorLamoine Ok, I see. Well, there are a few missing spaces before braces here and there, as usual. Apart from that I do not have any objections.

[VictorLamoine]

I ran the Eclipse formater with the style sheet found .
Tell me if you want me to include these (heavy) modifcations in this pull request or not.

https://github.com/VictorLamoine/pcl/compare/templating...templating_formated

[jspricke]

Hi Victor, the usual rule is to only fix formating errors in lines with actual changes (no formating only commit because it changes git blame). Also, it would be nice to have them in the actual commit, not in an extra one. Would be great if you could do that!

[VictorLamoine]

(no formating only commit because it changes git blame)

You are talkin about git-blame right ?
I can do this but you have to tell me if the format is right or needs to be tweaked;

I used the eclipse formatter (XML file) I found in PCL:
https://github.com/PointCloudLibrary/pcl/blob/master/doc/advanced/content/files/PCL_eclipse_profile.xml.
I don't want to spend an hour reformatting each commit if it's not perfect.

https://github.com/VictorLamoine/pcl/commit/4d15caf6c2bf884dc5b6a8c95df3971ebb272b66

Thanks!

[jspricke]

The general style looks good. I think I found some missing spaces in front of a (), but these should be easy to find.

[VictorLamoine]

The formatting is now included in every single commit.
I only re-formatted the functions that I have modified/moved.

Building and tests went fine for me.

[taketwo]

Spaces look okay now, thanks for the effort!

What I dislike though is how the formatter aligned the parameters in functions with long signatures. For example,

getTransformation (float x, float y, float z, float roll, float pitch,
    float yaw)

Or even more weird

inline bool
checkCoordinateSystem (const Eigen::Matrix<double, 3, 1> &origin,
    const Eigen::Matrix<double, 3, 1> &x_direction,
    const Eigen::Matrix<double, 3, 1> &y_direction, const double norm_limit =
        1e-3, const double dot_limit = 1e-3)

Is this the default behavior? I do not think that we have any guidelines regarding line width.

[VictorLamoine]

The current PCL Eclipse profile has a limit of 80 as a maximum line width.
http://stackoverflow.com/questions/903754/do-you-still-limit-line-length-in-code

I can set this value to 250 for example (to avoid huge lines) and improve readability by aligning the parameters. The same could be done when assigning values in matrices etc.

In fact it's not that long to edit the single commits format :)

[taketwo]

I can set this value to 250 for example

Let's do this. I think everyone will agree that in the examples I posted formatting is a nonsense.

[taketwo]

Looks perfect now, I'm merging. Thanks Victor!