Template intersections functions #646

VictorLamoine · 2014-04-17T09:04:34Z

It is my first attempt to template C++ functions so please review my code.
planeWithPlaneIntersection and threePlanesIntersection are now templated to allow usage with floats/doubles.

I also tested when using an Eclipse project; Eclipse still finds the functions definitions:

Test unit is working (testing both floats/doubles behaviour); the output is :

./test/common/test_plane_intersection 
[==========] Running 3 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 3 tests from PCL
[ RUN      ] PCL.lineWithLineIntersection
[       OK ] PCL.lineWithLineIntersection (0 ms)
[ RUN      ] PCL.planeWithPlaneIntersection
Plane A and Plane B are parallel
Plane A and Plane B are parallel

Plane A and Plane B are parallel
Plane A and Plane B are parallel

Plane A and Plane B are parallel
Plane A and Plane B are parallel

Plane A and Plane B are parallel
Plane A and Plane B are parallel

Plane A and Plane B are parallel
Plane A and Plane B are parallel

Plane A and Plane B are parallel
Plane A and Plane B are parallel

Plane A and Plane B are parallel
Plane A and Plane B are parallel

Plane A and Plane B are parallel
Plane A and Plane B are parallel

Plane A and Plane B are parallel
Plane A and Plane B are parallel

Plane A and Plane B are parallel
Plane A and Plane B are parallel

Plane A and Plane B are parallel
Plane A and Plane B are parallel

Plane A and Plane B are parallel
Plane A and Plane B are parallel


[       OK ] PCL.planeWithPlaneIntersection (1 ms)
[ RUN      ] PCL.threePlanesIntersection

At least two planes are parralel.
At least two planes are parralel.

At least two planes are parralel.
At least two planes are parralel.


[       OK ] PCL.threePlanesIntersection (0 ms)
[----------] 3 tests from PCL (1 ms total)

[----------] Global test environment tear-down
[==========] 3 tests from 1 test case ran. (1 ms total)
[  PASSED  ] 3 tests.

taketwo · 2014-04-17T09:14:55Z

common/include/pcl/common/impl/intersections.hpp

-  double test_cosine = plane_a.head<3>().dot(plane_b.head<3>());
-  double upper_limit = 1 + angular_tolerance;
-  double lower_limit = 1 - angular_tolerance;
+  Scalar test_cosine = plane_a.head(3).dot(plane_b.head(3));


Why did you change <3> to (3)?

Its does not compile with <3>()
http://pastebin.com/8zRHGj65

I found this in Eigen documentation (Vector Blocks section):
http://eigen.tuxfamily.org/dox/Eigen2ToEigen3.html

(3) seems to be an strict equivalent to <3>(); plus it's compiling

(3) seems to be an strict equivalent to <3>()

No, when the number is a template parameter it is a compile-time constant. I won't claim that in this particular situation it yields faster/more optimized code, but I do believe that generally it makes sense to give the compiler as much information as possible.

As for the compilation error, you need to add template keyword, i.e. plane_a.template head<3> ().dot (plane_b.template head<3> ());

I get the idea; not the syntax. I'll dig this later !
I'm updating the code right now

Do this:

Scalar test_cosine = plane_a.head<3> ().dot(plane_b.head<3> ());

It is faster since you already know how much of the vector you need

nizar-sallem · 2014-04-17T10:20:55Z

@VictorLamoine as is we can't accept I am afraid, you will need to make some changes so that it becomes better (please read Sergey's comments and mine).
I would suggest that you use typedefs for Eigen::Matrix<Scalar, 4, 1> such us

typedef Eigen::Matrix<Scalar, 4, 1> Vector4;
typedef Eigen::Matrix<Scalar, 3, 3> Matrix3;

It would make the code much more readable and more elegant,

taketwo · 2014-04-17T10:23:46Z

I would suggest that you use typedefs for Eigen::Matrix

I would avoid creating such typedefs in .h or .hpp file because everyone who includes these files will get the typedefs whether he whats it or not.

VictorLamoine · 2014-04-17T10:32:32Z

It still compiles and test is fine

nizar-sallem · 2014-04-17T10:36:23Z

@taketwo if you put them inside the functions they will be just local anyway it is just a suggestion :)
@VictorLamoine problem is not if it compiles and tests but if it is well done ;)

taketwo · 2014-04-17T10:40:35Z

@nizar-sallem That's true, putting them into functions is fine. But this means that in the function argument list we still need to spell type names entirely.

anyway it is just a suggestion :)

That was just a counter-suggestion from my side :)

nizar-sallem · 2014-04-17T10:42:34Z

@taketwo that is exactly what I meant inside function implementation not declaration I should have mentioned it.

taketwo · 2014-04-17T10:46:32Z

common/include/pcl/common/intersections.h

-                              double angular_tolerance = 0.1);
+                              float angular_tolerance = 0.1)
+  {
+    return ( planeWithPlaneIntersection<float> (plane_a, plane_b, line, angular_tolerance) );


We don't put spaces inside parenthesis (also line 107).

Should I always put a space before a parenthesis ?
.normalized () or .normalized() ?
.foo (5) or .foo(5) ?

It's not clear in PCL Style guide: http://pointclouds.org/documentation/advanced/pcl_style_guide.php

I refered to these spaces:

| | v v return ( planeWithPlaneIntersection<float> (plane_a, plane_b, line, angular_tolerance) );

As for your question, yes, always put a space. Excerpt from the guide:

We also include a space before the bracketed list of arguments to a function/method

nizar-sallem · 2014-04-17T10:48:31Z

@taketwo original version was with a double angular_tolerance that is why I said it can break the API and that is why the static cast is done.

taketwo · 2014-04-17T10:55:23Z

Hm... that is true, double is not implicitly convertible to float. What would you suggest?

nizar-sallem · 2014-04-17T11:02:46Z

I commented on this sergey but basically he will need to do this

bool 
pcl::threePlanesIntersection (const Eigen::Vector4f &plane_a, 
                              const Eigen::Vector4f &plane_b,
                              const Eigen::Vector4f &plane_c,
                              Eigen::Vector3f &intersection_point,
                              double determinant_tolerance)
{
  float tolerance (determinant_tolerance);
  return (pcl::threePlanesIntersection<float> (plane_a, plane_b, plane_c, intersection_point, tolerance));
}

Or any equivalent form of his choice

taketwo · 2014-04-17T11:06:44Z

Oh, I see you comment, sorry.

Okay, for threePlanesIntersection we don't really need to create this proxy because it has not been released in PCL yet, so we won't break anyone's code. But for planeWithPlaneIntersection we need this. Though I would mark the proxy as deprecated so we can remove it later on.

nizar-sallem · 2014-04-17T11:52:56Z

Not sure if it has to be deprecated (I don't know what kind of values are taken by determinant_tolerance but they can be less than std::numeric_limits<float>::epsilon can't they ?).

taketwo · 2014-04-17T11:55:58Z

Not sure. But if they can, then the best way to be make tolerance a double independently from template scalar.

nizar-sallem · 2014-04-17T11:59:55Z

I believe that was the main idea behind the double tolerance whereas the planes coefficients were floats so I won't change that and I would keep the tolerance as a double but again I don't believe I used this method before so I would stick with a common agreement ( @jspricke what do you think ?)

nizar-sallem · 2014-04-17T12:08:38Z

@VictorLamoine a general comment on the unit test : don't bother using .f unless you don't use .0
Also please use the square brackets operator to access a vector element it is just common sense and a bit faster AFAIK in Eigen.

VictorLamoine · 2014-04-18T09:03:32Z

To summarize:
Doesn't break planeWithPlaneIntersection
Changes threePlanesIntersection usage (but this is not in any stable API)

Compiles & test unit is ok.
Is it ok now ?

taketwo · 2014-04-18T09:09:21Z

I would keep the tolerance as a double

I agree with Nizar, let's have double precision tolerance everywhere.

VictorLamoine · 2014-04-18T09:20:14Z

Ok now all intersections functions uses doubles for precision/tolerance
I also changed PCL_ERROR to PCL_DEBUG

taketwo · 2014-04-18T10:25:34Z

Could you also remove newline printing in the unit test so we do not get these gaps in test output:

16: Test command: /home/travis/build/PointCloudLibrary/pcl/build/test/common/test_plane_intersection
16: Test timeout computed to be: 9.99988e+06
16: [==========] Running 3 tests from 1 test case.
16: [----------] Global test environment set-up.
16: [----------] 3 tests from PCL
16: [ RUN      ] PCL.lineWithLineIntersection
16: [       OK ] PCL.lineWithLineIntersection (0 ms)
16: [ RUN      ] PCL.planeWithPlaneIntersection
16: 
16: 
16: 
16: 
16: 
16: 
16: 
16: 
16: 
16: 
16: 
16: 
16: 
16: [       OK ] PCL.planeWithPlaneIntersection (0 ms)
16: [ RUN      ] PCL.threePlanesIntersection
16: 
16: 
16: 
16: 
16: [       OK ] PCL.threePlanesIntersection (1 ms)
16: [----------] 3 tests from PCL (1 ms total)
16: 
16: [----------] Global test environment tear-down
16: [==========] 3 tests from 1 test case ran. (1 ms total)
16: [  PASSED  ] 3 tests.
16/95 Test #16: common_int .............................   Passed    0.04 sec

taketwo · 2014-04-18T10:28:24Z

Everything else looks fine for me. If others won't have any further comments I think we can merge.

nizar-sallem · 2014-04-18T13:17:24Z

Same as for Sergey, looks fine for me except for the new lines.

VictorLamoine · 2014-04-18T20:13:18Z

Done :

[==========] Running 3 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 3 tests from PCL
[ RUN      ] PCL.lineWithLineIntersection
[       OK ] PCL.lineWithLineIntersection (0 ms)
[ RUN      ] PCL.planeWithPlaneIntersection
[       OK ] PCL.planeWithPlaneIntersection (0 ms)
[ RUN      ] PCL.threePlanesIntersection
[       OK ] PCL.threePlanesIntersection (0 ms)
[----------] 3 tests from PCL (1 ms total)

[----------] Global test environment tear-down
[==========] 3 tests from 1 test case ran. (1 ms total)
[  PASSED  ] 3 tests.

I also removed a useless typename in the .hpp.

taketwo · 2014-04-18T21:28:48Z

Neat! Thanks for the effort.

Template intersections functions

Regression fix for commit #646

taketwo reviewed Apr 17, 2014
View reviewed changes

Template intersections functions

8eb53d8

taketwo added a commit that referenced this pull request Apr 18, 2014

Merge pull request #646 from VictorLamoine/template_intersections

04b2a6a

Template intersections functions

taketwo merged commit 04b2a6a into PointCloudLibrary:master Apr 18, 2014

taketwo added a commit that referenced this pull request Apr 24, 2014

Merge pull request #650 from VictorLamoine/template_intersections

75c32d4

Regression fix for commit #646

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Template intersections functions #646

Template intersections functions #646

VictorLamoine commented Apr 17, 2014

taketwo Apr 17, 2014

VictorLamoine Apr 17, 2014

taketwo Apr 17, 2014

VictorLamoine Apr 17, 2014

nizar-sallem Apr 17, 2014

nizar-sallem commented Apr 17, 2014

taketwo commented Apr 17, 2014

VictorLamoine commented Apr 17, 2014

nizar-sallem commented Apr 17, 2014

taketwo commented Apr 17, 2014

nizar-sallem commented Apr 17, 2014

taketwo Apr 17, 2014

VictorLamoine Apr 17, 2014

taketwo Apr 17, 2014

nizar-sallem commented Apr 17, 2014

taketwo commented Apr 17, 2014

nizar-sallem commented Apr 17, 2014

taketwo commented Apr 17, 2014

nizar-sallem commented Apr 17, 2014

taketwo commented Apr 17, 2014

nizar-sallem commented Apr 17, 2014

nizar-sallem commented Apr 17, 2014

VictorLamoine commented Apr 18, 2014

taketwo commented Apr 18, 2014

VictorLamoine commented Apr 18, 2014

taketwo commented Apr 18, 2014

taketwo commented Apr 18, 2014

nizar-sallem commented Apr 18, 2014

VictorLamoine commented Apr 18, 2014

taketwo commented Apr 18, 2014

Template intersections functions #646

Template intersections functions #646

Conversation

VictorLamoine commented Apr 17, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nizar-sallem commented Apr 17, 2014

taketwo commented Apr 17, 2014

VictorLamoine commented Apr 17, 2014

nizar-sallem commented Apr 17, 2014

taketwo commented Apr 17, 2014

nizar-sallem commented Apr 17, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nizar-sallem commented Apr 17, 2014

taketwo commented Apr 17, 2014

nizar-sallem commented Apr 17, 2014

taketwo commented Apr 17, 2014

nizar-sallem commented Apr 17, 2014

taketwo commented Apr 17, 2014

nizar-sallem commented Apr 17, 2014

nizar-sallem commented Apr 17, 2014

VictorLamoine commented Apr 18, 2014

taketwo commented Apr 18, 2014

VictorLamoine commented Apr 18, 2014

taketwo commented Apr 18, 2014

taketwo commented Apr 18, 2014

nizar-sallem commented Apr 18, 2014

VictorLamoine commented Apr 18, 2014

taketwo commented Apr 18, 2014