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

fix Eigen::Affine3d for Melodic (using Eigen::Isometry3d) #1096

Merged
merged 9 commits into from Nov 28, 2018

Conversation

Projects
None yet
7 participants
@rhaschke
Copy link
Contributor

rhaschke commented Oct 17, 2018

Alternative implementation of #1091 using the type Eigen::Isometry instead of Eigen::Affine for homogeneous transforms. While Eigen::Affine allows for arbitrary affine transformation, we actually need to deal with rigid-body transforms only, i.e. rotations + translations. To this end, Eigen provides the type Isometry, which provides more efficient versions for invert() and rotation().

@rhaschke rhaschke changed the title Fix affine isometry fix Eigen::Affine3d -> Eigen::Isometry3d Oct 17, 2018

@rhaschke

This comment has been minimized.

Copy link
Contributor Author

rhaschke commented Oct 17, 2018

Just noticed that we need to switch to Eigen::Isometry in geometry_shapes as well!

@rhaschke rhaschke force-pushed the ubi-agni:fix-affine-isometry branch from 894965d to 8e96201 Oct 17, 2018

@rhaschke

This comment has been minimized.

Copy link
Contributor Author

rhaschke commented Oct 17, 2018

Now it compiles on my side. So, looks like the approach is feasible w/o too much effort.
Note: All changes to ros-planning related packages need to be made synchronously!

@davetcoleman
Copy link
Member

davetcoleman left a comment

i did an initial review and i like this approach, thanks Robert!

@@ -104,7 +104,7 @@ class World
/** \brief The poses of the corresponding entries in shapes_.
*
* @copydetails shapes_ */
EigenSTL::vector_Affine3d shape_poses_;
EigenSTL::vector_Isometry3d shape_poses_;

This comment has been minimized.

@davetcoleman

davetcoleman Oct 18, 2018

Member

@Levi-Armstrong what do you think of these changes to collision world?

Show resolved Hide resolved moveit_core/constraint_samplers/src/default_constraint_samplers.cpp

@davetcoleman davetcoleman requested a review from mcevoyandy Oct 18, 2018

@rhaschke rhaschke force-pushed the ubi-agni:fix-affine-isometry branch from c205473 to 8e96201 Oct 20, 2018

@rhaschke rhaschke changed the title fix Eigen::Affine3d -> Eigen::Isometry3d fix Eigen::Affine3d for Melodic (using Eigen::Isometry3d) Oct 20, 2018

@rhaschke rhaschke referenced this pull request Oct 21, 2018

Closed

2018 Fall Release Kinetic and Melodic #1083

11 of 11 tasks complete

@rhaschke rhaschke force-pushed the ubi-agni:fix-affine-isometry branch from bf6f53f to c9183b7 Oct 22, 2018

@rhaschke

This comment has been minimized.

Copy link
Contributor Author

rhaschke commented Oct 22, 2018

Rebased to resolve merge conflict.

@davetcoleman

This comment has been minimized.

Copy link
Member

davetcoleman commented Oct 24, 2018

Tested locally and it builds for me. Note you need to catkin clean first. Also ported moveit_visual_tools:

PickNikRobotics/rviz_visual_tools#105
ros-planning/moveit_visual_tools#39

@rhaschke rhaschke force-pushed the ubi-agni:fix-affine-isometry branch from c9183b7 to fb15c97 Oct 24, 2018

@rhaschke rhaschke force-pushed the ubi-agni:fix-affine-isometry branch 2 times, most recently from e5e029d to bd4373d Oct 27, 2018

@mcevoyandy
Copy link

mcevoyandy left a comment

I think this looks good. Would it be worth it to put more of an explanation of why Isometry3d is used instead of Affine3d?

@rhaschke rhaschke force-pushed the ubi-agni:fix-affine-isometry branch 2 times, most recently from e09b2ff to 9e0113d Nov 11, 2018

@rhaschke rhaschke force-pushed the ubi-agni:fix-affine-isometry branch from 9e0113d to 43128ca Nov 11, 2018

@rhaschke

This comment has been minimized.

Copy link
Contributor Author

rhaschke commented Nov 11, 2018

@davetcoleman I rebased this to fix conflicts introduced by your renaming of kstate / kmodel variables in #1179. Can we proceed with this set of PRs?
If so, I suggest to merge and prepare a release immediately after.

@rhaschke rhaschke force-pushed the ubi-agni:fix-affine-isometry branch from 43128ca to 635de0d Nov 11, 2018

@rhaschke rhaschke force-pushed the ubi-agni:fix-affine-isometry branch from 6de5d7f to e7cf8eb Nov 28, 2018

@rhaschke

This comment has been minimized.

Copy link
Contributor Author

rhaschke commented Nov 28, 2018

I will start the merging...

@rhaschke rhaschke force-pushed the ubi-agni:fix-affine-isometry branch 2 times, most recently from e0c3238 to f907cf2 Nov 28, 2018

@rhaschke rhaschke force-pushed the ubi-agni:fix-affine-isometry branch from f907cf2 to 0b1d470 Nov 28, 2018

@rhaschke rhaschke merged commit 68eb44d into ros-planning:melodic-devel Nov 28, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rhaschke rhaschke deleted the ubi-agni:fix-affine-isometry branch Nov 28, 2018

@rhaschke

This comment has been minimized.

Copy link
Contributor Author

rhaschke commented Nov 28, 2018

Merging done. Waiting for feedback on #1225 (comment) before proceeding with releases.

@v4hn

This comment has been minimized.

Copy link
Member

v4hn commented Nov 29, 2018

It looks like this broke the build on Ubuntu 16.04 with an Eigen static assertion failure...

YOU_PERFORMED_AN_INVALID_TRANSFORMATION_CONVERSION

@2scholz just spotted the problem.

@rhaschke

This comment has been minimized.

Copy link
Contributor Author

rhaschke commented Nov 29, 2018

Could you please be more specific (as you can easily verify, Travis succeeds).
This whole set of changes (see main comment) needs to be applied to let this work. You might also need to adapt your own code to use Eigen::Isometry instead of Eigen::Affine.

@2scholz

This comment has been minimized.

Copy link
Contributor

2scholz commented Nov 29, 2018

After numerous clean builds, updating every package in the workspace and a reboot the error is gone now.
I am not sure what the cause was. Sorry for the false alarm.

@jonbinney

This comment has been minimized.

Copy link
Contributor

jonbinney commented Dec 6, 2018

Huh... I ran into that same invalid transformation assertion 18.04 just now using latest melodic-devel. Running apt-get dist-upgrade to see if that helps.

@rhaschke

This comment has been minimized.

Copy link
Contributor Author

rhaschke commented Dec 6, 2018

@jonbinney As pointed out above, this whole change-set is not yet released. Hence, you need to build all mentioned packages from source to succeed.

@jonbinney

This comment has been minimized.

Copy link
Contributor

jonbinney commented Dec 6, 2018

Yeah - I was missing geometric_shapes from source. Thanks!

@simonschmeisser

This comment has been minimized.

Copy link
Contributor

simonschmeisser commented Dec 6, 2018

Same happened for me some days ago, can we have version checks on catkin packages in CMake?

clalancette added a commit to clalancette/pilz_industrial_motion that referenced this pull request Dec 11, 2018

Convert Eigen::Affine3d -> Eigen::Isometry3d.
This is to keep up with the recent changes in moveit
(ros-planning/moveit#1096) and
keep this package building.

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

jschleicher added a commit to PilzDE/pilz_industrial_motion that referenced this pull request Dec 17, 2018

Convert Eigen::Affine3d -> Eigen::Isometry3d. (#38)
This is to keep up with the recent changes in moveit
(ros-planning/moveit#1096) and
keep this package building.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.