Port tf2_kdl #90

vmayoral · 2019-02-03T17:39:42Z

No description provided.

tfoote

This would be great to have. Before merging this I'd like to see the unit tests ported too. They use a launch file in ROS1 but in ROS2 there's no master so that's not necessary for unit tests so everything can just be a unit test. The launch file only launches a single executable at a time anyway.

tf2_kdl/package.xml

mlautman · 2019-03-18T19:10:41Z

This port is still incomplete. Here are some issues that remain:

Finish package.xml port make tf2_kdl an ament pkg AcutronicRobotics/geometry2#3
Port underlying python_orcos_kdl for PyKDL
Remove roslib and rospy from tf2_kdl
Remove deprecated tf2_kdl/include/tf2_kdl/tf2_kdl subdirectory
Port the CPP test and python test. Also, test.py should be moved to the test/ subdirectory instead of scripts and should not be built as an executable but as a python test.

tf2_kdl/CMakeLists.txt

mlautman · 2019-03-18T19:14:30Z

tf2_kdl/CMakeLists.txt


-catkin_python_setup()


python libs need to be exported with

ament_python_install_package(${PROJECT_NAME} PACKAGE_DIR src/${PROJECT_NAME})

mlautman · 2019-03-18T19:55:34Z

tf2_kdl/CMakeLists.txt

-endif()
-
-endif()
+# if(BUILD_TESTING)


Don't just comment out tests. This port still needs work which gets hidden because the tests are disabled.

jacobperron · 2019-04-25T16:49:13Z

@vmayoral I'm moved this back to "in progress", let me know if/when it's ready for another round of review.

vmayoral · 2019-04-25T22:04:46Z

Sure @jacobperron, will do.

vmayoral · 2019-04-29T10:14:33Z

@ahcorde cherry-picked your commits here. Please review.

vmayoral · 2019-04-29T10:14:52Z

@jacobperron, after @ahcorde's review, it should be ready to go from our side.

clalancette

There are a few things to fixup here, but nothing major.

clalancette · 2019-04-29T22:55:16Z

tf2_kdl/test/test_tf2_kdl.cpp

+  // auto node = rclcpp::Node::make_shared("test");
+  rclcpp::init(argc, argv);
+  rclcpp::Clock::SharedPtr clock = std::make_shared<rclcpp::Clock>(RCL_SYSTEM_TIME);
+  tf_buffer = new tf2_ros::Buffer(clock);


I think this would be better as a std::unique_ptr. Then we would make sure not to leak memory below.

clalancette · 2019-04-29T22:55:36Z

tf2_kdl/test/test_tf2_kdl.cpp

 #include "tf2_ros/buffer.h"
-
+#include <tf2/convert.h>

 tf2_ros::Buffer* tf_buffer;


See comment below about making this a std::unique_ptr.

clalancette · 2019-04-29T23:04:05Z

tf2_kdl/include/tf2_kdl/tf2_kdl.h

+ * \return The TimePoint conversion.
+ */
+inline
+tf2::TimePoint timePointFromTime(const builtin_interfaces::msg::Time& t)


We already have versions of this and timeFromTimePoint in https://github.com/ros2/geometry2/blob/ros2/tf2_ros/include/tf2_ros/buffer_interface.h#L46 ; is there any reason we can't use those?

Done, But I was wondering if this methods should go into tf2/time.h. inside tf2_ros/buffer_interface.h it's not very intuitive

Done, But I was wondering if this methods should go into tf2/time.h. inside tf2_ros/buffer_interface.h it's not very intuitive

The reason for that is that tf2 itself is meant to be ROS-agnostic; you'll notice that there are no includes of ROS header files anywhere directly in tf2. The interface between tf2 and ROS comes via the tf2_ros package.

That being said, I agree that it is not very intuitive that the time functions are in tf2_ros/buffer_interface.h. I'd be in favor of making a new file tf2_ros/time.h where these methods would go, but I'll suggest we do that in a separate PR.

Sure! I like tf2_ros/time.h

clalancette

One more minor change, then this is good to go from me. I'll run CI on it in the meantime.

clalancette · 2019-04-30T14:53:11Z

tf2_kdl/test/test_tf2_kdl.cpp

  t.header.frame_id = "A";
  t.child_frame_id = "B";
  tf_buffer->setTransform(t, "test");

  int retval = RUN_ALL_TESTS();
-  delete tf_buffer;
+  tf_buffer.reset();


With the change to a unique_ptr, this shouldn't be necessary; it will be destroyed as it goes out of scope. So you can just remove this.

This is still unresolved in the latest patches.

clalancette · 2019-04-30T14:59:47Z

CI:

Linux
Linux-aarch64
macOS
Windows

vmayoral · 2019-04-30T15:05:49Z

@clalancette could you please consider #93 as well (required for moveit2 as well)

tf2_eigen/include/tf2_eigen/tf2_eigen.h

clalancette · 2019-04-30T15:49:50Z

It looks like Windows is upset for two different reasons:

15:19:18 C:\J\workspace\ci_windows\ws\src\ros2\geometry2\tf2_kdl\test\test_tf2_kdl.cpp(45): error C2065: 'M_PI': undeclared identifier [C:\J\workspace\ci_windows\ws\build\tf2_kdl\test_kdl.vcxproj]
15:19:18 C:\J\workspace\ci_windows\ws\src\ros2\geometry2\tf2_kdl\test\test_tf2_kdl.cpp(120): warning C4244: '=': conversion from 'double' to 'builtin_interfaces::msg::Time_<std::allocator<void>>::_sec_type', possible loss of data [C:\J\workspace\ci_windows\ws\build\tf2_kdl\test_kdl.vcxproj]
15:19:18 C:\J\workspace\ci_windows\ws\src\ros2\geometry2\tf2_kdl\test\test_tf2_kdl.cpp(121): warning C4244: '=': conversion from 'double' to 'builtin_interfaces::msg::Time_<std::allocator<void>>::_nanosec_type', possible loss of data [C:\J\workspace\ci_windows\ws\build\tf2_kdl\test_kdl.vcxproj]

The M_PI thing we've dealt with before, but I can't exactly remember how; @tfoote , do you remember? The loss of precision thing is actually true, so we either need to explicitly cast it or do something smarter there.

tf2_eigen/include/tf2_eigen/tf2_eigen.h

tf2_kdl/test/test_tf2_kdl.cpp

tf2_eigen/include/tf2_eigen/tf2_eigen.h

clalancette · 2019-04-30T22:39:49Z

Actually, before I run CI again, there hasn't been any change for the Windows loss of precision, so @ahcorde will need to do something about that before moving forward.

ahcorde · 2019-05-01T09:38:47Z

can you try the CI with this last commit?

header.stamp contains:

int32 sec
uint32 nanosec

I was assigning a double. Probably the sizeof(double) and the sizeof(int32) (or sizeof(uint32)) is different and that's why it's failing.

clalancette · 2019-05-01T16:26:22Z

Rebuild of just Windows:

If this succeeds, I'll do one more CI run of everything.

clalancette · 2019-05-01T17:05:11Z

All right, Windows is happy, here is the whole lot:

Linux
Linux-aarch64
macOS
Windows

clalancette

One last nitpick, then I'm happy to approve.

tf2_kdl/include/tf2_kdl/tf2_kdl.h

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

clalancette · 2019-05-09T20:07:43Z

Is there any chance to revisit this PR #99 and merge it? otherwise this Python test is not going to work

Sorry for being quiet here. After a bit of discussion with @tfoote , I think we've determined that for now it will be fine to just not build and install the python stuff, and leave that as a TODO for later. So we can forget about #99 in terms of this PR; I'll open an issue on this repository to finish that porting.

Towards getting things finished, I've taken the liberty of rebasing this PR and doing some cleanup in the CMakeLists.txt and package.xml to get things moving again. Take a look at what I've done in ec63e8b, and let me know what you think.

I'm also going to run CI on this shortly.

clalancette · 2019-05-09T20:09:31Z

@vmayoral Apologies, we had a collision with 3bbfee5 and my rebase. I'll restore your changes in a few minutes.

clalancette · 2019-05-09T20:13:56Z

Linux
Linux-aarch64
macOS
Windows

clalancette · 2019-05-09T20:18:56Z

And issue for python port opened in #110

clalancette · 2019-05-09T20:59:17Z

All right, CI is all green and things seem happy. I'm going to give this 24 hours for any further objections, then I'll merge it.

ahcorde · 2019-05-10T07:17:56Z

@clalancette Thanks for reviewing it 👍

ros-discourse · 2023-12-14T20:45:50Z

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/the-moveit-2-journey-part-1-porting-and-understanding-moveit-core/8718/1

tfoote added the in review Waiting for review (Kanban column) label Feb 3, 2019

tfoote self-requested a review February 6, 2019 08:48

tfoote reviewed Feb 7, 2019

View reviewed changes

tf2_kdl/package.xml Show resolved Hide resolved

vmayoral mentioned this pull request Feb 10, 2019

ROS 2 port moveit/random_numbers#12

Closed

cottsay assigned tfoote Feb 21, 2019

mlautman suggested changes Mar 18, 2019

View reviewed changes

jacobperron added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Apr 25, 2019

This was referenced Apr 26, 2019

Update with ros2/geometry AcutronicRobotics/geometry2#4

Closed

Adding tf2_kdl tests AcutronicRobotics/geometry2#6

Closed

jacobperron added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Apr 29, 2019

clalancette requested changes Apr 29, 2019

View reviewed changes

clalancette requested changes Apr 30, 2019

View reviewed changes

clalancette reviewed Apr 30, 2019

View reviewed changes

tf2_eigen/include/tf2_eigen/tf2_eigen.h Outdated Show resolved Hide resolved

tfoote requested changes Apr 30, 2019

View reviewed changes

tf2_eigen/include/tf2_eigen/tf2_eigen.h Outdated Show resolved Hide resolved

tf2_kdl/test/test_tf2_kdl.cpp Show resolved Hide resolved

tf2_eigen/include/tf2_eigen/tf2_eigen.h Outdated Show resolved Hide resolved

vmayoral mentioned this pull request Apr 30, 2019

conversions for Eigen::Isometry3d in ROS 2 #93

Closed

clalancette reviewed May 1, 2019

View reviewed changes

tf2_kdl/include/tf2_kdl/tf2_kdl.h Outdated Show resolved Hide resolved

vmayoral and others added 16 commits May 9, 2019 19:35

Make unit test compile

5a62555

Fixing test tf2_kdl

5759155

including it own includes in tf2_kdl

c048af0

Using timeFromTimePoint function from tf2_ros2

5e9b38c

Fixing tf2_kdl test memory leaks

80e2787

indent return

8f29b64

Define _USE_MATH_DEFINES so Windows gets M_PI symbol.

284e109

Quaternion one to one mapping

e85b82a

Removing delete of a unique_ptr object

f5253a4

Fixing possible loss of data in windows

50259c5

Remove unused NSEC_PER_SECOND.

ee08296

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

Adding ament_python_install_package to tf2_kdl

ebdf144

Remove deprecated tf2_kdl/include/tf2_kdl/tf2_kdl subdirectory

9d6d882

Porting tf2_kdl python test

dffc911

Porting tf2_kdl pythonn library

ae5652d

Cleanup the package.xml and CMakeLists.txt

ec63e8b

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

clalancette force-pushed the ros2 branch from f892128 to ec63e8b Compare May 9, 2019 20:06

clalancette mentioned this pull request May 9, 2019

Finish porting Python portions of tf2_kdl #110

Open

clalancette merged commit c4e331b into ros2:ros2 May 10, 2019

clalancette removed the in review Waiting for review (Kanban column) label May 10, 2019

anasarrak mentioned this pull request Jun 3, 2019

Keeping Isometry3d in ROS2 moveit/moveit2#32

Closed

vmayoral mentioned this pull request Jun 12, 2019

Not finding dashing tf2_kdl on debians #116

Closed

aprotyas mentioned this pull request Sep 4, 2021

Covariance is not transformed in do_transform_pose_with_covariance_stamped #431

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port tf2_kdl #90

Port tf2_kdl #90

vmayoral commented Feb 3, 2019

tfoote left a comment

mlautman commented Mar 18, 2019 •

edited by tfoote

Loading

mlautman Mar 18, 2019

mlautman Mar 18, 2019

jacobperron commented Apr 25, 2019

vmayoral commented Apr 25, 2019

vmayoral commented Apr 29, 2019

vmayoral commented Apr 29, 2019

clalancette left a comment

clalancette Apr 29, 2019

clalancette Apr 29, 2019

clalancette Apr 29, 2019

ahcorde Apr 30, 2019

clalancette Apr 30, 2019

ahcorde Apr 30, 2019

clalancette left a comment

clalancette Apr 30, 2019

clalancette Apr 30, 2019

ahcorde May 1, 2019

clalancette commented Apr 30, 2019

vmayoral commented Apr 30, 2019

clalancette commented Apr 30, 2019

clalancette commented Apr 30, 2019

ahcorde commented May 1, 2019

clalancette commented May 1, 2019

clalancette commented May 1, 2019

clalancette left a comment

clalancette commented May 9, 2019

clalancette commented May 9, 2019

clalancette commented May 9, 2019

clalancette commented May 9, 2019

clalancette commented May 9, 2019

ahcorde commented May 10, 2019

ros-discourse commented Dec 14, 2023


		catkin_python_setup()

Port tf2_kdl #90

Port tf2_kdl #90

Conversation

vmayoral commented Feb 3, 2019

tfoote left a comment

Choose a reason for hiding this comment

mlautman commented Mar 18, 2019 • edited by tfoote Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobperron commented Apr 25, 2019

vmayoral commented Apr 25, 2019

vmayoral commented Apr 29, 2019

vmayoral commented Apr 29, 2019

clalancette left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clalancette left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clalancette commented Apr 30, 2019

vmayoral commented Apr 30, 2019

clalancette commented Apr 30, 2019

clalancette commented Apr 30, 2019

ahcorde commented May 1, 2019

clalancette commented May 1, 2019

clalancette commented May 1, 2019

clalancette left a comment

Choose a reason for hiding this comment

clalancette commented May 9, 2019

clalancette commented May 9, 2019

clalancette commented May 9, 2019

clalancette commented May 9, 2019

clalancette commented May 9, 2019

ahcorde commented May 10, 2019

ros-discourse commented Dec 14, 2023

mlautman commented Mar 18, 2019 •

edited by tfoote

Loading