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

conversions for Eigen::Isometry3d in ROS 2 #93

Closed
wants to merge 44 commits into from

Conversation

vmayoral
Copy link
Member

@vmayoral vmayoral commented Mar 3, 2019

This commits fetch ros/geometry2#335 into ROS 2 and addresses the requests from moveit/moveit2#32 originated at moveit/moveit2#12

@tfoote tfoote added the in review Waiting for review (Kanban column) label Mar 3, 2019
vmayoral added a commit to AcutronicRobotics/moveit2 that referenced this pull request Mar 3, 2019
Revert the changes in transform and use Isometry3d after
changes in geometry2 ros2/geometry2#93
vmayoral added a commit to AcutronicRobotics/moveit2 that referenced this pull request Mar 3, 2019
Revert the changes in transform and use Isometry3d after
changes in geometry2 ros2/geometry2#93
@davetcoleman
Copy link

@rhaschke please review since i believe this was originally yours

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

I'm not in ROS2, so I cannot judge the corresponding changes. The Eigen stuff looks reasonable.

tf2_eigen/include/tf2_eigen/tf2_eigen.h Show resolved Hide resolved
tf2_eigen/include/tf2_eigen/tf2_eigen.h Outdated Show resolved Hide resolved
msg.orientation.y = Eigen::Quaterniond(in.rotation()).y();
msg.orientation.z = Eigen::Quaterniond(in.rotation()).z();
msg.orientation.w = Eigen::Quaterniond(in.rotation()).w();
Eigen::Quaterniond q(in.linear());
Copy link
Contributor

Choose a reason for hiding this comment

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

If that's possible, flip the quaternion in a single line operation, instead of manually doing it below.

Suggested change
Eigen::Quaterniond q(in.linear());
Eigen::Quaterniond q(in.linear());
if (q.w() < 0)
q *= 1.0;

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that we want to be doing another operation if we can avoid it. Every extra operation is potentially a loss of precision as well as costing resources.

The negative w values are completely valid quaternions. I'd rather not just normalize it here. This breaks the expectation that fromMsg(toMsg(...)) will return the same value.

Michael Jeronimo added 2 commits March 13, 2019 14:42
In the MessageFilter class, use node interfaces instead of using the node directly
so that the code works with either rclcpp::Node or rclcpp_lifecycle::LifecycleNode.
Retain the existing node-based interface for backwards compatibility.
davetcoleman pushed a commit to moveit/moveit2 that referenced this pull request Mar 15, 2019
* Port transforms submodule of moveit_core of ROS 2

* transforms submodule, add tests

Added tests to moveit_core submodule

* Cleanup transforms submodule meta-files

* moveit_core, transforms submodule, back to Isometry3d

Revert the changes in transform and use Isometry3d after
changes in geometry2 ros2/geometry2#93

* transforms, fix logging

Follows from #21

Changes in the code, particularly in the for loops
are required due to the use of rclcpp macros
@davetcoleman
Copy link

closes moveit/moveit2#32

vmayoral pushed a commit to AcutronicRobotics/moveit2 that referenced this pull request Mar 16, 2019
* Port transforms submodule of moveit_core of ROS 2

* transforms submodule, add tests

Added tests to moveit_core submodule

* Cleanup transforms submodule meta-files

* moveit_core, transforms submodule, back to Isometry3d

Revert the changes in transform and use Isometry3d after
changes in geometry2 ros2/geometry2#93

* transforms, fix logging

Follows from moveit#21

Changes in the code, particularly in the for loops
are required due to the use of rclcpp macros
* Fixed ros2#658

Signed-off-by: ivanpauno <[email protected]>

* Modified Eigen include directory again

Signed-off-by: ivanpauno <[email protected]>

* Deleted custom variable 'Eigen_INCLUDE_DIR'

Signed-off-by: ivanpauno <[email protected]>

* revert unrelated whitespace change

Signed-off-by: ivanpauno <[email protected]>

* Added Eigen message again

Signed-off-by: ivanpauno <[email protected]>
mm318 and others added 7 commits May 8, 2019 11:55
* add LICENSE file

Signed-off-by: Miaofei <[email protected]>
* Use node interface pointers in the TransformListener class

In the TransformListener class, use node interfaces instead of using the node directly
so that the code works with either rclcpp::Node or rclcpp_lifecycle::LifecycleNode.
Retain the existing node-based interface for backwards compatibility.

* Format code based on ament_cpplint and ament_uncrustify

* use node interfaces consistently

Signed-off-by: Karsten Knese <[email protected]>

* adher to new rclcpp API

Signed-off-by: Karsten Knese <[email protected]>

* remove std::forward

Signed-off-by: Karsten Knese <[email protected]>

* export symbols for windows

Signed-off-by: Karsten Knese <[email protected]>

* callback also has to be public

Signed-off-by: Karsten Knese <[email protected]>

* address review

Signed-off-by: Karsten Knese <[email protected]>

* refactor node wrapper class

Signed-off-by: Karsten Knese <[email protected]>

* restore latched todo

Signed-off-by: Karsten Knese <[email protected]>
* Adding tf2_msgs actions
* improve computation efficiency

* improve comments

* normalize quaternions to be in half-space w >= 0

... as in tf1

* add Eigen::Isometry3d conversions

Fix conflicts and adapt code to ROS 2.0
Edited by @vmayoral.

* Port tf2_kdl

* Adapt for eigen3

* Return to eigen in package.xml

Follow advice of @tfoote

* Remove cmake_modules

* Update tf2_kdl messages

* Include the right headers

* tf2_eigen, leftover from the cherry-pick

While cherry-picking changes to get isometry3d in

* Fix problem with stamp

Picked also a leftover from the ROS2 conversion

* Fix more issues while porting to ROS2

* Remove duplicated definitions

* Undo changes in unit tests

Leave code as it was left by @clalancette.
Only added some additional lines to test also
Isometry as per the commits of Robert

* Update tf2_eigen, add toMsg2

Convert a Eigen::Vector3d type to a geometry_msgs::msg::Vector3
while avoiding overloading issues with previous definitions

* make tf2_kdl an ament pkg

* Default to C++14

* First steps towards getting tests

* Fix timePointFromTime conversions

* Make unit test compile

* Fixing test tf2_kdl

* including it own includes in tf2_kdl

* Using timeFromTimePoint function from tf2_ros2

* Fixing tf2_kdl test memory leaks

* indent return

* Define _USE_MATH_DEFINES so Windows gets M_PI symbol.

* Quaternion one to one mapping

* Removing delete of a unique_ptr object

* Fixing possible loss of data in windows

* Remove unused NSEC_PER_SECOND.

Signed-off-by: Chris Lalancette <[email protected]>

* Adding ament_python_install_package to tf2_kdl

* Remove deprecated tf2_kdl/include/tf2_kdl/tf2_kdl subdirectory

* Porting tf2_kdl python test

* Porting tf2_kdl pythonn library

* Cleanup the package.xml and CMakeLists.txt

Signed-off-by: Chris Lalancette <[email protected]>
@vmayoral
Copy link
Member Author

#90 has been merged but this PR still remains. @ahcorde, how do you want to proceed about this? AFAIK, these changes are needed to compile http://github.com/acutronicrobotics/moveit2

@clalancette
Copy link
Contributor

@vmayoral The thing to do here is to rebase this on top of the current ros2 branch. Then this will be much easier to review. Thanks!

@vmayoral
Copy link
Member Author

@vmayoral The thing to do here is to rebase this on top of the current ros2 branch. Then this will be much easier to review. Thanks!

Thanks @clalancette, I was expecting @ahcorde to take care of it since he's ensuring everything's aligned for the Dashing release but I went ahead and did it myself. @ahcorde can you have a look yourself at the changes as well please?

@tfoote
Copy link
Contributor

tfoote commented May 14, 2019

This rebase looks like it has ended up with a lot of duplicate already merged content in the diff. It's over 1000 lines of diff and includes 44 commits several of which are in the last release or two including things like the changelogs making this diff indecipherable. Which are the commits in this branch that are adding new content to be reviewed?

I pulled it and computed the diff to look like this:

$ git diff ros2
diff --git a/tf2_eigen/include/tf2_eigen/tf2_eigen.h b/tf2_eigen/include/tf2_eigen/tf2_eigen.h
index 8f8fd2e..9213915 100644
--- a/tf2_eigen/include/tf2_eigen/tf2_eigen.h
+++ b/tf2_eigen/include/tf2_eigen/tf2_eigen.h
@@ -50,7 +50,7 @@ namespace tf2
  */
  inline
  Eigen::Isometry3d transformToEigen(const geometry_msgs::msg::Transform& t) {
-   return Eigen::Isometry3d(Eigen::Translation3d(t.translation.x, t.translation.y, t.translation.z)
+ return Eigen::Isometry3d(Eigen::Translation3d(t.translation.x, t.translation.y, t.translation.z)
                         * Eigen::Quaterniond(t.rotation.w, t.rotation.x, t.rotation.y, t.rotation.z));
 }
 
@@ -224,12 +224,12 @@ void doTransform(const Eigen::Isometry3d& t_in,
  */
 inline
 geometry_msgs::msg::Quaternion toMsg(const Eigen::Quaterniond& in) {
- geometry_msgs::msg::Quaternion msg;
- msg.w = in.w();
- msg.x = in.x();
- msg.y = in.y();
- msg.z = in.z();
- return msg;
+  geometry_msgs::msg::Quaternion msg;
+  msg.w = in.w();
+  msg.x = in.x();
+  msg.y = in.y();
+  msg.z = in.z();
+  return msg;
 }
 
 /** \brief Convert a Quaternion message type to a Eigen-specific Quaterniond type.
@@ -341,22 +341,6 @@ geometry_msgs::msg::Pose toMsg(const Eigen::Isometry3d& in) {
   return msg;
 }
 
-/** \brief Convert a Eigen::Vector3d type to a geometry_msgs::msg::Vector3.
- * This function is a specialization of the toMsg template defined in tf2/convert.h.
- * The function was renamed to toMsg2 to avoid overloading conflicts with definitions above.
- *
- * \param in The Eigen::Vector3d to convert.
- * \return The Eigen::Vector3d converted to a geometry_msgs::msg::Vector3 message.
- */
-inline
-geometry_msgs::msg::Vector3 toMsg2(const Eigen::Vector3d& in) {
-  geometry_msgs::msg::Vector3 msg;
-  msg.x = in(0);
-  msg.y = in(1);
-  msg.z = in(2);
-  return msg;
-}
-
 /** \brief Convert a Pose message transform type to a Eigen Affine3d.
  * This function is a specialization of the toMsg template defined in tf2/convert.h.
  * \param msg The Pose message to convert.

@dirk-thomas
Copy link
Member

@vmayoral Friendly ping.

@vmayoral
Copy link
Member Author

That's right @tfoote, sorry. Thanks for the ping @dirk-thomas.

Following at #113.

@vmayoral vmayoral closed this May 16, 2019
@tfoote tfoote removed the in review Waiting for review (Kanban column) label May 16, 2019
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.

None yet