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

rhaschke
Copy link
Contributor

@rhaschke 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
Copy link
Contributor Author

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

@rhaschke
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!

Copy link
Member

@davetcoleman davetcoleman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Rebased to resolve merge conflict.

@davetcoleman
Copy link
Member

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

PickNikRobotics/rviz_visual_tools#105
moveit/moveit_visual_tools#39

Copy link

@mcevoyandy mcevoyandy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
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 merged commit 68eb44d into moveit:melodic-devel Nov 28, 2018
@rhaschke rhaschke deleted the fix-affine-isometry branch November 28, 2018 06:16
@rhaschke
Copy link
Contributor Author

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

@v4hn
Copy link
Contributor

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
Copy link
Contributor Author

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
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
Copy link
Contributor

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
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
Copy link
Contributor

Yeah - I was missing geometric_shapes from source. Thanks!

@simonschmeisser
Copy link
Contributor

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

jschleicher pushed a commit to PilzDE/pilz_industrial_motion that referenced this pull request Dec 17, 2018
This is to keep up with the recent changes in moveit
(moveit/moveit#1096) and
keep this package building.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
martiniil pushed a commit to PilzDE/pilz_common that referenced this pull request Nov 20, 2020
…_motion/#38)

This is to keep up with the recent changes in moveit
(moveit/moveit#1096) and
keep this package building.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
JafarAbdi pushed a commit to JafarAbdi/moveit that referenced this pull request Mar 24, 2022
Signed-off-by: Gaël Écorchard <gael.ecorchard@cvut.cz>
v4hn pushed a commit to v4hn/moveit that referenced this pull request May 6, 2022
Signed-off-by: Gaël Écorchard <gael.ecorchard@cvut.cz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants