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

Switch to using Eigen for Quaternion and Matrix. #21

Merged
merged 2 commits into from Jan 5, 2018

Conversation

Projects
None yet
2 participants
@clalancette
Copy link
Collaborator

commented Nov 21, 2017

This should fix #6, though to be conservative we may want to merge this onto a melodic-devel branch, rather than kinetic-devel.

Signed-off-by: Chris Lalancette clalancette@osrfoundation.org

Switch to using Eigen for Quaternion and Matrix.
Signed-off-by: Chris Lalancette <clalancette@osrfoundation.org>
@sloretz
Copy link
Collaborator

left a comment

Sorry it took me so long to review this. Looks good after a couple CMake changes.

# We don't build anything here, but in order to export the right
# build flags in catkin_package(), we still have to find Eigen3 here. Note
# that this is somewhat complicated because of our need to support Ubuntu
# all the way back to saucy. First we look for the Eigen3 cmake module

This comment has been minimized.

Copy link
@sloretz

sloretz Dec 18, 2017

Collaborator

REP 3 says kinetic targets xenial and above. libeigen3-dev is in trusty and above. I think this block can be replaced with find_package(Eigen3 REQUIRED)

This comment has been minimized.

Copy link
@clalancette

clalancette Jan 5, 2018

Author Collaborator

Ah, good point. It looks like Kinetic used to support Wily, but we've long stopped worrying about that. Fixed in 4edadbf

set(EIGEN3_INCLUDE_DIRS ${EIGEN_INCLUDE_DIRS})
endif()

# Note that eigen 3.2 (on Ubuntu Wily) only provides EIGEN3_INCLUDE_DIR,

This comment has been minimized.

Copy link
@sloretz

sloretz Dec 18, 2017

Collaborator

Same here, xenial through bionic is 3.3.*

@sloretz

This comment has been minimized.

Copy link
Collaborator

commented Dec 18, 2017

I notice #6 has a melodic label. I can't think of anything that would block targeting this at kinetic. Can you think of anything?

@clalancette

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 18, 2017

I notice #6 has a melodic label. I can't think of anything that would block targeting this at kinetic. Can you think of anything?

I agree with you; this is all internal code, so we should be able to merge this for kinetic as well. There is a slight chance for downstream breakage if downstream packages were depending on this one to pull in TF, but I think that is acceptable (and should be relatively easy to fix).

Simplify Eigen3 dependency.
We only support Xenial+ now, so we don't need to worry about
backwards compatibility anymore.

Signed-off-by: Chris Lalancette <clalancette@osrfoundation.org>
@sloretz

sloretz approved these changes Jan 5, 2018

@clalancette

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 5, 2018

Great, thanks! Merging.

@clalancette clalancette merged commit d7c60a7 into kinetic-devel Jan 5, 2018

2 checks passed

Kpr__collada_urdf__ubuntu_xenial_amd64 Build finished.
Details
Lpr__collada_urdf__ubuntu_xenial_amd64 Build finished.
Details

@clalancette clalancette deleted the remove-tf-dep branch Jan 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.