Re: [eigen] Avoid SVD in rotation() for Isometry

On Fri, Jan 11, 2019 at 6:42 PM Greg Coombe <greg.coombe@xxxxxxxxx> wrote:

Hi,
We have been transitioning off our patchwork collection of custom transform libraries and onto Eigen for performance, code quality, etc. The part we're working on now is rigid-body transformations, and so we're using the isometric mode of Transform (no scaling).

One of the things that we've run into is that our old transform class used "rotation()" to access the linear part of the transform, and "translation()" to access the translation. Naively moving the old code over the new system hits a little gotcha: the "rotation()" call in Transform actually performs a full SVD to extract the scale component from the linear part: https://bitbucket.org/eigen/eigen/src/9a28171b87b280b9ef77502a33b77fc7f4532dcf/Eigen/src/Geometry/Transform.h?at=default&fileviewer=file-view-default#Transform.h-1061

So we've tried to be careful about replacing "rotation()" with "linear()", but it struck me that there is another approach: if we know that the Transform is an isometry, we can just return the linear part directly. I've attached a simple patch that does this. This patch would help us avoid the SVD gotcha, but it does add a little complexity.

Testing: I tried to adapt some of the tests in geo_transformations.cpp, but I didn't have a lot of luck. The main transformation() test function has a bunch of tests inside it, but they sort of mix-and-match scaling transformations with non-scaling transformations, so I couldn't just change the template parameter to Isometry with breaking apart a lot of the function. I'm happy to go down that route (it might slightly improve parallelism), but I didn't want to take that on as my first patch. So I just added some checks to make sure existing rotation() calls are not affected, but there is probably more that needs to be done.

Thanks,
Greg