Replace Geometry2d::Point with Eigen::Vector2f?

[justbuchanan]

Right now we have our own class for representing 2d points, but there is one provided in Eigen, the matrix library we use for many things. The benefit of switching is that if we natively use Eigen's Vector2f class, it makes it much easier to do other matrix operations on them. This will come in handy as we start doing more advanced path-planning and motion control.

Here's the API reference for the Matrix class, which Vector2f is an instance of: http://eigen.tuxfamily.org/dox-devel/classEigen_1_1Matrix.html.

@barulicm, do you see any reasons not to switch? I know there are a few convenience methods we added to the Point class that are nice, but we could just make those functions that take Vector2fs as parameters.

[barulicm]

I'm not sure I completely agree with this idea. While I love Eigen with all my heart, I don't know that replacing Geometry2d::Point is worth the eigen compatibility. There are a few issues that should be seriously considered in this decision.

Library Dependencies Turning Point into Vector2f breaks the independence of the Geometry2d library. This will permanently tie the two libraries together and turn Geometry2d into a mere extension of the Eigen framework. Now, as Geometry2d is a library developed specifically for this project, this may or may not be a concern. It definitely feels weird to replace such a fundamental part of a library with a type from another library entirely.

Stylistic Effects "A few convenience methods" translates to "every public method on Point" based on my understanding of the Matrix and Point interface differences. Eigen is built as a generic linear algebra library with types such as Vector2f that can do anything but have built-in support for almost nothing. Our Point interface has been tuned for the actions we take on a Point in our current code base. Geometric concepts such as rotation, distance, and magnitude are all easily implemented on Vector2f, but not explicitly built in. We would end up with a header file containing nearly every Point method as an external function taking a self-style parameter. Thousands of calls on Point objects would have to change to match this new interface, all for the sake of a relatively few places that want to multiply them by matrices.

Relative Difficulty of Alternatives Of course, opinions are most worthwhile when accompanied by alternatives. In this case, I would recommend we look closely at the kinds of operations we really want to do. From personal reflection, I imagine most of our future math will be reduced to matrix-vector multiplications. I believe it will be very easy to define a small set of binary operators in a single-header extension of our Geometry2d library that would facilitate the interface of Eigen matrices and Geometry2d points. I think this should be seriously considered, as it allows for natural notation in all uses of Point, rather than only those few uses that interact with Eigen. Existing method calls would be left unchanged, and Eigen-Geometry2d interactions would look just like Eigen-Eigen interactions via binary operators.

Changes such as this, which I would consider a breach of library etiquette, should be approached cautiously. Even for private libraries, APIs define style conventions that permeate the code base. Prioritizing use cases is important. Where are these classes most often used? How can we ensure that all interactions with our library feel as natural as possible? A geometric point and an algebraic vector, while incredibly similar, are often, contextually, very different.

[justbuchanan]

Alright, I'll leave it... I'm not as worried about it from the dependency point of view, but I do agree that the method names on Vector2f are wierd and the code is a lot better with the nice names that we have on the Point class. I'll go ahead and just add some convenience methods for converting between the two classes.

[ashaw596]

What if we made the geometry point extend the vector2f? That would provide the performance gains and matrix operations without having to change all the point calls. It would mess up the dependency independence though. On Jan 10, 2015 2:11 PM, "Justin Buchanan" notifications@github.com wrote:

Alright, I'll leave it... I'm not as worried about it from the dependency point of view, but I do agree that the method names on Vector2f are wierd and the code is a lot better with the nice names that we have on the Point class. I'll go ahead and just add some convenience methods for converting between the two classes.

— Reply to this email directly or view it on GitHub https://github.com/RoboJackets/robocup-software/issues/175#issuecomment-69467841 .

[justbuchanan]

That actually sounds pretty good, I forgot about subclassing...

[ashaw596]

I finished doing the subclassing on the albert/refactorPoint branch, but its way way way too slow. It makes the speed go down to 10 fps from 60 fps. I think its because their constructor is more expensive and we make alot of instances of Point.

[justbuchanan]

Hmm, what do you think is expensive about the constructor? Btw, you can leave the constructor out of there and just use the inherited one. Same for the x() and y() methods and several of the math functions. I wonder if the fact that x() and y() are functions now is making it slow for the python code. C++ can inline those calls to be the same speed as a direct ivar access, but the same doesn't work across the language barrier as far as I know.

[justbuchanan]

@ashaw596, I just realized that we haven't had compiler optimizations flags on, so by default it just tries to make the compile as fast as possible (not the code that runs). Could you try this out again with optimizations on (add -O2 or -O3) and see if performance is still degraded with the switch to Eigen?

[jgkamat]

@ashaw596 albert/refactorPoint seems to be gone now. Do you happen to still have a copy of it (or did you trash it intentionally?)

[ashaw596]

I moved it to https://github.com/ashaw596/robocup-software/tree/albert/refactorPoint since it wasn't working well.