Content-Length: 609361 | pFad | https://github.com/moveit/geometric_shapes/pull/96

CD Port to ROS 2 by vmayoral · Pull Request #96 · moveit/geometric_shapes · GitHub
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

Port to ROS 2 #96

Closed
wants to merge 23 commits into from
Closed

Conversation

vmayoral
Copy link

@vmayoral vmayoral commented Feb 10, 2019

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:

  • Review warnings
  • Port testing

For completeness, warnings pending of review:

In file included from /Users/victor/ros2_moveit_ws/src/geometric_shapes/src/shapes.cpp:39:
In file included from /usr/local/include/octomap/octomap.h:37:
In file included from /usr/local/include/octomap/OcTree.h:38:
In file included from /usr/local/include/octomap/OccupancyOcTreeBase.h:506:
/usr/local/include/octomap/OccupancyOcTreeBase.hxx:116:108: warning: unused parameter 'maxrange' [-Wunused-parameter]
  void OccupancyOcTreeBase<NODE>::insertPointCloudRays(const Pointcloud& pc, const point3d& origen, double maxrange, bool lazy_eval) {
                                                                                                           ^
In file included from /Users/victor/ros2_moveit_ws/src/geometric_shapes/src/shapes.cpp:40:
/usr/local/include/console_bridge/console.h:70:88: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
  console_bridge::log(__FILE__, __LINE__, console_bridge::CONSOLE_BRIDGE_LOG_WARN,  fmt, ##__VA_ARGS__)
                                                                                       ^
/Users/victor/ros2_moveit_ws/src/geometric_shapes/src/shapes.cpp:265:34: warning: unused parameter 'scale' [-Wunused-parameter]
void OcTree::scaleAndPadd(double scale, double padd)
                                 ^
/Users/victor/ros2_moveit_ws/src/geometric_shapes/src/shapes.cpp:265:48: warning: unused parameter 'padd' [-Wunused-parameter]
void OcTree::scaleAndPadd(double scale, double padd)
                                               ^
In file included from /Users/victor/ros2_moveit_ws/src/geometric_shapes/src/shapes.cpp:40:
/usr/local/include/console_bridge/console.h:70:88: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
  console_bridge::log(__FILE__, __LINE__, console_bridge::CONSOLE_BRIDGE_LOG_WARN,  fmt, ##__VA_ARGS__)
                                                                                       ^
/Users/victor/ros2_moveit_ws/src/geometric_shapes/src/shapes.cpp:270:33: warning: unused parameter 'scale' [-Wunused-parameter]
void Plane::scaleAndPadd(double scale, double padding)
                                ^
/Users/victor/ros2_moveit_ws/src/geometric_shapes/src/shapes.cpp:270:47: warning: unused parameter 'padding' [-Wunused-parameter]
void Plane::scaleAndPadd(double scale, double padding)
                                              ^
In file included from /Users/victor/ros2_moveit_ws/src/geometric_shapes/src/mesh_operations.cpp:49:
/usr/local/include/assimp/scene.h:240:1: warning: 'aiScene' defined as a struct here but previously declared as a class [-Wmismatched-tags]
struct aiScene
^
/Users/victor/ros2_moveit_ws/src/geometric_shapes/include/geometric_shapes/mesh_operations.h:45:1: note: did you mean struct here?
class aiScene;
^~~~~
struct
In file included from /Users/victor/ros2_moveit_ws/src/geometric_shapes/src/body_operations.cpp:39:
/usr/local/include/console_bridge/console.h:67:90: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
  console_bridge::log(__FILE__, __LINE__, console_bridge::CONSOLE_BRIDGE_LOG_ERROR, fmt, ##__VA_ARGS__)
                                                                                         ^
In file included from /Users/victor/ros2_moveit_ws/src/geometric_shapes/src/shape_operations.cpp:45:
/usr/local/include/console_bridge/console.h:70:88: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
  console_bridge::log(__FILE__, __LINE__, console_bridge::CONSOLE_BRIDGE_LOG_WARN,  fmt, ##__VA_ARGS__)
                                                                                       ^
/usr/local/include/console_bridge/console.h:67:90: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
  console_bridge::log(__FILE__, __LINE__, console_bridge::CONSOLE_BRIDGE_LOG_ERROR, fmt, ##__VA_ARGS__)
                                                                                         ^
/usr/local/include/console_bridge/console.h:67:88: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
  console_bridge::log(__FILE__, __LINE__, console_bridge::CONSOLE_BRIDGE_LOG_ERROR, fmt, ##__VA_ARGS__)
                                                                                       ^
/Users/victor/ros2_moveit_ws/src/geometric_shapes/src/shape_operations.cpp:158:49: warning: unused parameter 'shape_msg' [-Wunused-parameter]
  void operator()(const shape_msgs::msg::Plane& shape_msg) const
                                                ^
In file included from /Users/victor/ros2_moveit_ws/src/geometric_shapes/src/shape_operations.cpp:45:
/usr/local/include/console_bridge/console.h:67:90: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
  console_bridge::log(__FILE__, __LINE__, console_bridge::CONSOLE_BRIDGE_LOG_ERROR, fmt, ##__VA_ARGS__)
                                                                                         ^
/Users/victor/ros2_moveit_ws/src/geometric_shapes/src/shape_operations.cpp:205:60: warning: unused parameter 'shape_msg' [-Wunused-parameter]
  Eigen::Vector3d operator()(const shape_msgs::msg::Plane& shape_msg) const
                                                           ^
In file included from /Users/victor/ros2_moveit_ws/src/geometric_shapes/src/shape_operations.cpp:45:
/usr/local/include/console_bridge/console.h:67:90: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
  console_bridge::log(__FILE__, __LINE__, console_bridge::CONSOLE_BRIDGE_LOG_ERROR, fmt, ##__VA_ARGS__)
                                                                                         ^
/usr/local/include/console_bridge/console.h:67:90: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
/usr/local/include/console_bridge/console.h:67:90: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
7 warnings generated.
/Users/victor/ros2_moveit_ws/src/geometric_shapes/src/bodies.cpp:131:67: warning: unused parameter 'verbose' [-Wunused-parameter]
bool bodies::Sphere::containsPoint(const Eigen::Vector3d& p, bool verbose) const
                                                                  ^
In file included from /Users/victor/ros2_moveit_ws/src/geometric_shapes/src/mesh_operations.cpp:46:
/usr/local/include/console_bridge/console.h:67:88: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
  console_bridge::log(__FILE__, __LINE__, console_bridge::CONSOLE_BRIDGE_LOG_ERROR, fmt, ##__VA_ARGS__)
                                                                                       ^
/usr/local/include/console_bridge/console.h:70:88: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
  console_bridge::log(__FILE__, __LINE__, console_bridge::CONSOLE_BRIDGE_LOG_WARN,  fmt, ##__VA_ARGS__)
                                                                                       ^
/usr/local/include/console_bridge/console.h:67:90: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
  console_bridge::log(__FILE__, __LINE__, console_bridge::CONSOLE_BRIDGE_LOG_ERROR, fmt, ##__VA_ARGS__)
                                                                                         ^
/usr/local/include/console_bridge/console.h:70:90: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
  console_bridge::log(__FILE__, __LINE__, console_bridge::CONSOLE_BRIDGE_LOG_WARN,  fmt, ##__VA_ARGS__)
                                                                                         ^
/usr/local/include/console_bridge/console.h:70:90: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
/usr/local/include/console_bridge/console.h:70:88: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
  console_bridge::log(__FILE__, __LINE__, console_bridge::CONSOLE_BRIDGE_LOG_WARN,  fmt, ##__VA_ARGS__)
                                                                                       ^
/usr/local/include/console_bridge/console.h:70:90: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
  console_bridge::log(__FILE__, __LINE__, console_bridge::CONSOLE_BRIDGE_LOG_WARN,  fmt, ##__VA_ARGS__)
                                                                                         ^
/usr/local/include/console_bridge/console.h:70:90: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
/usr/local/include/console_bridge/console.h:70:90: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
/usr/local/include/console_bridge/console.h:67:90: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
  console_bridge::log(__FILE__, __LINE__, console_bridge::CONSOLE_BRIDGE_LOG_ERROR, fmt, ##__VA_ARGS__)
                                                                                         ^
/Users/victor/ros2_moveit_ws/src/geometric_shapes/src/bodies.cpp:264:69: warning: unused parameter 'verbose' [-Wunused-parameter]
bool bodies::Cylinder::containsPoint(const Eigen::Vector3d& p, bool verbose) const
                                                                    ^
/Users/victor/ros2_moveit_ws/src/geometric_shapes/src/bodies.cpp:317:99: warning: unused parameter 'max_attempts' [-Wunused-parameter]
bool bodies::Cylinder::samplePointInside(random_numbers::RandomNumberGenerator& rng, unsigned int max_attempts,
                                                                                                  ^
/Users/victor/ros2_moveit_ws/src/geometric_shapes/src/bodies.cpp:477:64: warning: unused parameter 'verbose' [-Wunused-parameter]
bool bodies::Box::containsPoint(const Eigen::Vector3d& p, bool verbose) const
                                                               ^
/Users/victor/ros2_moveit_ws/src/geometric_shapes/src/bodies.cpp:665:71: warning: unused parameter 'verbose' [-Wunused-parameter]
bool bodies::ConvexMesh::containsPoint(const Eigen::Vector3d& p, bool verbose) const
                                                                      ^
In file included from /Users/victor/ros2_moveit_ws/src/geometric_shapes/src/bodies.cpp:40:
/usr/local/include/console_bridge/console.h:70:88: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
  console_bridge::log(__FILE__, __LINE__, console_bridge::CONSOLE_BRIDGE_LOG_WARN,  fmt, ##__VA_ARGS__)
                                                                                       ^
/Users/victor/ros2_moveit_ws/src/geometric_shapes/src/mesh_operations.cpp:633:13: warning: unused function 'writeFloatToSTL' [-Wunused-function]
inline void writeFloatToSTL(char*& ptr, float data)
            ^
/usr/local/include/console_bridge/console.h:67:90: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
  console_bridge::log(__FILE__, __LINE__, console_bridge::CONSOLE_BRIDGE_LOG_ERROR, fmt, ##__VA_ARGS__)
                                                                                         ^
/usr/local/include/console_bridge/console.h:67:90: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
2 warnings generated.
8 warnings generated.
12 warnings generated.
8 warnings generated.

@rhaschke rhaschke changed the base branch from melodic-devel to ros2 February 10, 2019 15:55
@rhaschke
Copy link
Contributor

Directed at ros2 branch.

@vmayoral
Copy link
Author

Ported a few bits of the build testing infrastructure but it requires further work. Help is welcome @rhaschke, @davetcoleman, @gavanderhoorn and others.

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

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.

Copy link
Member

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()
Copy link
Member

Choose a reason for hiding this comment

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

enable tests

Copy link
Contributor

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

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 ?

Copy link
Contributor

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).

Copy link
Contributor

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

@davetcoleman davetcoleman requested a review from mlautman March 6, 2019 05:25
@mlautman mlautman changed the base branch from ros2 to crystal-devel March 7, 2019 22:14
CMakeLists.txt Outdated
# find_package(ament_cmake_gtest REQUIRED)
# # Unit tests
# add_subdirectory(test)
# endif()
Copy link
Contributor

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

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

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?

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

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?

@@ -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);
Copy link
Contributor

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

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

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

seanyen and others added 8 commits April 2, 2019 21:43
…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.
@rhaschke
Copy link
Contributor

Closing in favor of #118 / #122.

@rhaschke rhaschke closed this Nov 17, 2019
@vmayoral
Copy link
Author

@rhaschke tons of changes at #122 that removed authorship. E.g. @ahcorde contributed with 25255f3. This is wrong.

@rhaschke rhaschke mentioned this pull request Nov 17, 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.









ApplySandwichStrip

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier!      Saves Data!


--- a PPN by Garber Painting Akron. With Image Size Reduction included!

Fetched URL: https://github.com/moveit/geometric_shapes/pull/96

Alternative Proxies:

Alternative Proxy

pFad Proxy

pFad v3 Proxy

pFad v4 Proxy