Port transforms submodule of moveit_core of ROS 2 #12
Conversation
| typedef std::map<std::string, Eigen::Isometry3d, std::less<std::string>, | ||
| Eigen::aligned_allocator<std::pair<const std::string, Eigen::Isometry3d> > > | ||
| typedef std::map<std::string, Eigen::Affine3d, std::less<std::string>, | ||
| Eigen::aligned_allocator<std::pair<const std::string, Eigen::Affine3d> > > |
There was a problem hiding this comment.
This was a big change we made to moveit in Dec, why is it being reverted here?
There was a problem hiding this comment.
There was a problem hiding this comment.
Going back to Affine (or cluttering the history with hundreds of lines of unnecessary revert/counter-revert) is not an option. If you found a problem with ros2's geometry2-package, this should be fixed there (and this repository should probably require the patched version until then.)
I would hope upstream is very open to fixes in their interfaces if we need them.
There was a problem hiding this comment.
agreed, we definitely should not revert back to Affine3d, that's a performance hit
i just looked into the difference between melodic-devel and ros2 branch and i don't see any changes in functions for in tf2_eigen.h. why does this require changing for moveit2?
There was a problem hiding this comment.
@davetcoleman and @v4hn, have a look at the ROS 2 geometry2 package as indicated above. For further details:
- ROS1: https://github.com/ros/geometry2/blob/melodic-devel/tf2_eigen/include/tf2_eigen/tf2_eigen.h
- ROS2: https://github.com/ros2/geometry2/blob/ros2/tf2_eigen/include/tf2_eigen/tf2_eigen.h#L46
There's been a switch to Affine3d. Maybe @clalancette could advice if you are willing to accept a change reverting to Isometry3d? We should try to reach a compromise before producing any further changes.
There was a problem hiding this comment.
@vmayoral: the switch was actually to Isometry3D (as that makes much more sense). There's nothing to revert.
See:
- fix Eigen::Affine3d -> Eigen::Isometry3d geometric_shapes#88
- fix Eigen::Affine3d for Melodic (using Eigen::Isometry3d) moveit#1096
It seems ros2/geometry2 just hasn't been kept up-to-date (example of ROS1 - ROS2 community/maintenance split?).
There was a problem hiding this comment.
Thanks @gavanderhoorn for clarifying.
It seems
ros2/geometry2just hasn't been kept up-to-date (example of ROS1 - ROS2 community/maintenance split?).
I guess so.
@clalancette, how shall we proceed about this? Shall I book some time next week and provide a PR to adjust things in geometry2 for ROS2 or do you want to take care of this yourself? We can then re-adjust things here afterwards.
There was a problem hiding this comment.
This is the PR that needs to be forward ported: ros/geometry2#335
There was a problem hiding this comment.
See 94ebbcc. I believe we should be fine with this, right @davetcoleman?
Added tests to moveit_core submodule
Revert the changes in transform and use Isometry3d after changes in geometry2 ros2/geometry2#93
Follows from moveit#21 Changes in the code, particularly in the for loops are required due to the use of rclcpp macros
| @@ -37,11 +37,12 @@ | |||
| #ifndef MOVEIT_TRANSFORMS_ | |||
| #define MOVEIT_TRANSFORMS_ | |||
|
|
|||
| #include <geometry_msgs/TransformStamped.h> | |||
| #include <geometry_msgs/Pose.h> | |||
| #include <geometry_msgs/msg/transform_stamped.hpp> | |||
There was a problem hiding this comment.
It may make sense to make these changes repo wide with a find-replace all. Otherwise we will be manually making 462 changes across 91 files for just geometry_msgs:: and 23 times across 19 files for #include <geometry_msgs/msg/
|
@davetcoleman Would you want to give this another review? |
* 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
No description provided.