-
Notifications
You must be signed in to change notification settings - Fork 92
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
Port to ROS 2 #96
Port to ROS 2 #96
Conversation
Replaced by shape_msgs::msg::SolidPrimitive
Directed at ros2 branch. |
Ported a few bits of the build testing infrastructure but it requires further work. Help is welcome @rhaschke, @davetcoleman, @gavanderhoorn and others. |
Change bodies Eigen::Issometry calls to Eigen::Affine3d
include/geometric_shapes/bodies.h
Outdated
@@ -499,21 +499,21 @@ class BodyVector | |||
BodyVector(); | |||
|
|||
/** \brief Construct a body vector from a vector of shapes, a vector of poses and a padding */ | |||
BodyVector(const std::vector<shapes::Shape*>& shapes, const EigenSTL::vector_Isometry3d& poses, double padding = 0.0); | |||
BodyVector(const std::vector<shapes::Shape*>& shapes, const EigenSTL::vector_Affine3d& poses, double padding = 0.0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that this is currently blocking you in your porting efforts, but it seems more efficient and forward thinking to me to assume that the Isometry3d
migration will happen in all ROS pkgs that use Eigen::Affine3d
.
Instead of reverting all those changes in MoveIt and related pkgs, perhaps forward-porting the Isometry3d
changes to the pkgs that should have it would be more efficient in the long run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 we do not want to revert back to affine3d
CMakeLists.txt
Outdated
# find_package(ament_cmake_gtest REQUIRED) | ||
# # Unit tests | ||
# add_subdirectory(test) | ||
# endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enable tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
<name>geometric_shapes</name> | ||
<version>0.6.1</version> | ||
<version>0.6.2</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it necessary to bump the version here? my understanding is that only bloom does this during the release process @130s ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok to modify version number along with other code change although that sounds unusual. IMO it's typically done during release flow, e.g. in Catkin's world, catkin_prepare_release
nicely takes care of it automatically. Using a toolchain like that is preferable as it allows release flow to be more systematic and organized.
And the change suggested in this PR seems non-trivial, so bumping minor or even major number makes more sense to me (if the repo follows common major-minor-patch version semantics).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should keep ROS and ROS2 versions separate. In the RQT port we changed all of the ROS2 versions to 1.0.0 although I am not sure that is ideal
Return back to Isometry3d
CMakeLists.txt
Outdated
# find_package(ament_cmake_gtest REQUIRED) | ||
# # Unit tests | ||
# add_subdirectory(test) | ||
# endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
CMakeLists.txt
Outdated
ARCHIVE DESTINATION ${CATKIN_PACKAGE_LIB_DESTINATION} | ||
LIBRARY DESTINATION ${CATKIN_PACKAGE_LIB_DESTINATION}) | ||
ARCHIVE DESTINATION lib | ||
LIBRARY DESTINATION lib) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix alignment
ARCHIVE DESTINATION ${CATKIN_PACKAGE_LIB_DESTINATION} | ||
LIBRARY DESTINATION ${CATKIN_PACKAGE_LIB_DESTINATION}) | ||
ARCHIVE DESTINATION lib | ||
LIBRARY DESTINATION lib) | ||
|
||
install(DIRECTORY include/${PROJECT_NAME}/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use include_directories(include)
no?
include/geometric_shapes/bodies.h
Outdated
@@ -188,7 +188,7 @@ class Body | |||
virtual void computeBoundingCylinder(BoundingCylinder& cylinder) const = 0; | |||
|
|||
/** \brief Get a clone of this body, but one that is located at the pose \e pose */ | |||
BodyPtr cloneAt(const Eigen::Isometry3d& pose) const | |||
BodyPtr cloneAt(const Eigen::Isometry3d& pose) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we mixing code changes with porting changes? Is this actually necessary for ROS2?
include/geometric_shapes/bodies.h
Outdated
@@ -197,7 +197,7 @@ class Body | |||
pose \e pose and has possibly different passing and scaling: \e | |||
padding and \e scaling. This function is useful to implement | |||
thread safety, when bodies need to be moved around. */ | |||
virtual BodyPtr cloneAt(const Eigen::Isometry3d& pose, double padding, double scaling) const = 0; | |||
virtual BodyPtr cloneAt(const Eigen::Isometry3d& pose, double padding, double scaling); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
@@ -35,16 +35,16 @@ | |||
#ifndef GEOMETRIC_SHAPES_SHAPE_EXTENTS_ | |||
#define GEOMETRIC_SHAPES_SHAPE_EXTENTS_ | |||
|
|||
#include <shape_msgs/SolidPrimitive.h> | |||
#include <shape_msgs/Mesh.h> | |||
#include "shape_msgs/msg/solid_primitive.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to switch from <> to ""?
<name>geometric_shapes</name> | ||
<version>0.6.1</version> | ||
<version>0.6.2</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should keep ROS and ROS2 versions separate. In the RQT port we changed all of the ROS2 versions to 1.0.0 although I am not sure that is ideal
…t#101) * consolidate finding absolute paths for ASSIMP_LIBRARIES * Add comments. Co-Authored-By: seanyen <seanyen@microsoft.com>
Fix cmakelists to compile against other executables and fixing tests
reformat it slightly as well. This commit allows linking in OS X but requires console_bridge to be installed from sources. Note that console_bridge_vendor is a shim over console_bridge and still requires the installation of the later through its sources.
Fix packages.xml
Note to maintainers: Please do not merge in this branch, create a ros2 branch and modify this PR.
With the changes provided, package compiles in a ROS 2 workspace.
Pending items:
For completeness, warnings pending of review: