Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Eigen for math operations #150

Merged
merged 38 commits into from
Aug 5, 2016
Merged

Use Eigen for math operations #150

merged 38 commits into from
Aug 5, 2016

Conversation

jslee02
Copy link
Member

@jslee02 jslee02 commented Jul 28, 2016

This PR switches to Eigen for the math operations as suggested (#96).

The goal of this PR is to pass all the unit tests with equal to or better than the previous computational performance, and it seems it's achieved as of now. Other possible improvements (e.g., the templatizing suggested by @sherm1 (comment)) might be done in a separate PR.

Here are several todos that will be done in this PR:

Edit: Above todos will be done in future PRs to keep this PR as simple as possible for code review.

@traversaro
Copy link
Contributor

traversaro commented Jul 29, 2016

Some minor comments on the CMake parts of this PR, that can be probably fixed in other PRs .

  • There is any specific reason to use a custom pkg-config based FindEIGEN3.cmake rather then to copy FindEigen3.cmake officially distributed by Eigen [1]? I always had bad experience in using pkg-config in Windows, especially regarding path with spaces. I also suggest to integration the modification proposed in this PR [2], so if there is a Eigen3Config.cmake available in the system we use the information provided by the config file.
  • The Eigen3 headers directories are include at directory level with a #include_directories(${EIGEN3_INCLUDE_DIRS}), but given that Eigen3 headers are include in the public headers, we need to populate the INTERFACE_INCLUDE_DIRECTORIES of the exported fcl target. We can do that by calling target_include_directories(fcl PUBLIC ${EIGEN3_INCLUDE_DIRS}). Note that this will result in a non-relocatable fclConfig.cmake, but this can be solved in future PRs I think.

[1] : https://bitbucket.org/eigen/eigen/src/8267656b597c03c7fe501be33056cda9a63e88ab/cmake/FindEigen3.cmake?at=default&fileviewer=file-view-default
[2] : https://bitbucket.org/eigen/eigen/pull-requests/213/modify-findeigen3cmake-to-find/diff

@jslee02
Copy link
Member Author

jslee02 commented Jul 30, 2016

Thanks for the comments! I just didn't know that there is an officially distributed FindEigen3.cmake. Could we apply the proposed modification once it gets merged, though? Just in case it's updated with some feedbacks.

@jslee02 jslee02 changed the title [WIP] Use Eigen for math operations Use Eigen for math operations Jul 30, 2016
@traversaro
Copy link
Contributor

traversaro commented Jul 30, 2016

Yes, it makes sense to adopt the changes in FindEigen3.cmake only when they are adopted upstream.

@jslee02 jslee02 merged commit 0168edf into master Aug 5, 2016
@sherm1
Copy link
Member

sherm1 commented Aug 5, 2016

Eigen conversion looks great, @jslee02. Very nice to see files get deleted!

@peterkty
Copy link

Just a note that
setQuatRotation(), setTranslation() for transformation are no longer available after the conversion.
Transform3f was a quaternion plus translation. Now it is just a 3x3 matrix.
Vec3f is now Vector3f

I'm compiling moveit_core and it throw out these compile error.

@jslee02
Copy link
Member Author

jslee02 commented Aug 11, 2016

For completion: Vec3f is actually Vector3d. The scalar type of Vec3f was FCL_REAL, which is double, as Vector3d is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants