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 sensor_manager of moveit_core to ROS 2.0 #11

Merged
merged 1 commit into from
Feb 20, 2019

Conversation

vmayoral
Copy link
Contributor

No description provided.

@@ -1 +1 @@
install(DIRECTORY include/ DESTINATION ${CATKIN_GLOBAL_INCLUDE_DESTINATION})
install(DIRECTORY include/ DESTINATION include)
Copy link
Member

Choose a reason for hiding this comment

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

@mlautman is this best practice? should we ask dirk?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

As stated somewhere else, I believe GNUInstallDirs is the better approach here - from a maintainer's point of few at least.
@dirk-thomas I believe you were involved with the migration instructions, right?
Did you know about that cmake module or did you deliberately ignore it?
The only possible shortcoming I can see is that you might end up with lib<arch> folders instead of lib depending on the context

Copy link
Contributor

Choose a reason for hiding this comment

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

Using GNUInstallDirs or not is pretty much orthogonal to the migration instructions. You can either use it or not - in ROS 1 as well as in ROS 2 packages.

Copy link
Member

Choose a reason for hiding this comment

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

It sounds like switching to GNUInstallDirs is outside the scope of a first-pass moveit port to ROS2, and @vmayoral 's approach is good

#include <moveit_msgs/RobotTrajectory.h>
#include <geometry_msgs/PointStamped.h>
#include <moveit_msgs/msg/robot_trajectory.hpp>
#include <geometry_msgs/msg/point_stamped.hpp>
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, being rather ignorant of the current state of ROS2, I really wonder what should be the beautiful upside of changing includes like this, apart from making huge code-bases incompatible...

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC, the rational is that this way, it's possible to have services, actions and msgs that have the same name (but I'm not 100% certain).

Copy link
Member

Choose a reason for hiding this comment

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

^ that is correct. For example in ROS1 we had the standard of adding "Action" at the end of every action message type to avoid conflicts in messages or services, but it was decided (I have no opinion) to enforce this more rigorously through folders and namespaces

@davetcoleman davetcoleman merged commit 5c0a336 into moveit:master Feb 20, 2019
mlautman pushed a commit that referenced this pull request Mar 8, 2019
MikeWrock pushed a commit to MikeWrock/moveit2 that referenced this pull request Aug 15, 2022
* working move_group_interface tutorial

* splitting out prerequisites

* updating images, video link, and a goal pose

* adding author

* fixing second youtube link

* removing second video link
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

5 participants