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

add list of default capabilities #359

Merged
merged 1 commit into from Nov 16, 2016

Conversation

Projects
None yet
3 participants
@v4hn
Member

v4hn commented Nov 15, 2016

Functionality in MoveIt!'s move_group node is implemented in terms of
MoveGroupCapability plugins. But until now the choice of which plugins
are loaded was solely left to the user who in many cases is not familiar
with these plugins at all. This entails that whenever we introduce or change
a basic capability we have to annoy the user with warnings on startup that
bug her/him to change and/or update his move_group.launch file.

This commit changes the startup behavior of move_group to load a specified
list of default capabilities and provides a new ROS parameter disable_capabilities
that can be used to disable default plugins.

I'm unsure about whether or not we should backport this to indigo/jade.
Personally I'm mildly in favor of backporting.

Fixes #125.

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Nov 15, 2016

Member

Format checking failed and I expected that:

 static const char* DEFAULT_CAPABILITIES[] = {
-   "move_group/MoveGroupCartesianPathService",
-   "move_group/MoveGroupKinematicsService",
-   "move_group/MoveGroupCartesianPathService",
-   "move_group/MoveGroupExecuteTrajectoryAction",
-   "move_group/MoveGroupKinematicsService",
-   "move_group/MoveGroupMoveAction",
-   "move_group/MoveGroupPickPlaceAction",
-   "move_group/MoveGroupPlanService",
-   "move_group/MoveGroupQueryPlannersService",
-   "move_group/MoveGroupStateValidationService",
-   "move_group/MoveGroupGetPlanningSceneService",
-   "move_group/ApplyPlanningSceneService",
-   "move_group/ClearOctomapService",
+  "move_group/MoveGroupCartesianPathService", "move_group/MoveGroupKinematicsService",
+  "move_group/MoveGroupCartesianPathService", "move_group/MoveGroupExecuteTrajectoryAction",
+  "move_group/MoveGroupKinematicsService", "move_group/MoveGroupMoveAction", "move_group/MoveGroupPickPlaceAction",
+  "move_group/MoveGroupPlanService", "move_group/MoveGroupQueryPlannersService",
+  "move_group/MoveGroupStateValidationService", "move_group/MoveGroupGetPlanningSceneService",
+  "move_group/ApplyPlanningSceneService", "move_group/ClearOctomapService",
 };

Now I could change that, but I kinda disagree with the formatter here.
@davetcoleman do we really want such lists to group multiple items on each line?

This is independent of the actual content of this request though, so feel free to ignore this error during review.

Member

v4hn commented Nov 15, 2016

Format checking failed and I expected that:

 static const char* DEFAULT_CAPABILITIES[] = {
-   "move_group/MoveGroupCartesianPathService",
-   "move_group/MoveGroupKinematicsService",
-   "move_group/MoveGroupCartesianPathService",
-   "move_group/MoveGroupExecuteTrajectoryAction",
-   "move_group/MoveGroupKinematicsService",
-   "move_group/MoveGroupMoveAction",
-   "move_group/MoveGroupPickPlaceAction",
-   "move_group/MoveGroupPlanService",
-   "move_group/MoveGroupQueryPlannersService",
-   "move_group/MoveGroupStateValidationService",
-   "move_group/MoveGroupGetPlanningSceneService",
-   "move_group/ApplyPlanningSceneService",
-   "move_group/ClearOctomapService",
+  "move_group/MoveGroupCartesianPathService", "move_group/MoveGroupKinematicsService",
+  "move_group/MoveGroupCartesianPathService", "move_group/MoveGroupExecuteTrajectoryAction",
+  "move_group/MoveGroupKinematicsService", "move_group/MoveGroupMoveAction", "move_group/MoveGroupPickPlaceAction",
+  "move_group/MoveGroupPlanService", "move_group/MoveGroupQueryPlannersService",
+  "move_group/MoveGroupStateValidationService", "move_group/MoveGroupGetPlanningSceneService",
+  "move_group/ApplyPlanningSceneService", "move_group/ClearOctomapService",
 };

Now I could change that, but I kinda disagree with the formatter here.
@davetcoleman do we really want such lists to group multiple items on each line?

This is independent of the actual content of this request though, so feel free to ignore this error during review.

@rhaschke

This comment has been minimized.

Show comment
Hide comment
@rhaschke

rhaschke Nov 15, 2016

Contributor

+1.
Regarding clang-formatting, I had similar issues. The solution, I found is to manually turn off/on clang formatting by an appropriate comment in the code:

// clang-format off
... some untouched code
// clang-format on
Contributor

rhaschke commented Nov 15, 2016

+1.
Regarding clang-formatting, I had similar issues. The solution, I found is to manually turn off/on clang formatting by an appropriate comment in the code:

// clang-format off
... some untouched code
// clang-format on
@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Nov 15, 2016

Member

@rhaschke beat me to the response - I occasionally disagree with clang and use those commands, yes. Documented: ros-planning/moveit.ros.org#106

Member

davetcoleman commented Nov 15, 2016

@rhaschke beat me to the response - I occasionally disagree with clang and use those commands, yes. Documented: ros-planning/moveit.ros.org#106

add list of default capabilities
Functionality in MoveIt!'s move_group node is implemented in terms of
MoveGroupCapability plugins. But until now the choice of which plugins
are loaded was solely left to the user who in many cases is not familiar
with these plugins at all. This entails that whenever we introduce or change
a basic capability we have to annoy the user with warnings on startup that
bug him to change and/or update his move_group.launch file.

This commit changes the startup behavior of move_group to load a specified
list of default capabilities and provides a new ROS parameter `disable_capabilities`
that can be used to disable default plugins.

Fixes #125.
@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Nov 15, 2016

Member

On Tue, Nov 15, 2016 at 07:58:53AM -0800, Dave Coleman wrote:

@rhaschke beat me to the response - I occasionally disagree with clang and use those commands, yes. Documented: ros-planning/moveit.ros.org#106

Good to know, thanks @rhaschke!

I updated the commit

Member

v4hn commented Nov 15, 2016

On Tue, Nov 15, 2016 at 07:58:53AM -0800, Dave Coleman wrote:

@rhaschke beat me to the response - I occasionally disagree with clang and use those commands, yes. Documented: ros-planning/moveit.ros.org#106

Good to know, thanks @rhaschke!

I updated the commit

@davetcoleman

Looks great!

@@ -41,6 +41,25 @@
namespace move_group
{
// These capabilities are loaded unless listed in disable_capabilities
// clang-format off
static const char* DEFAULT_CAPABILITIES[] = {

This comment has been minimized.

@davetcoleman

davetcoleman Nov 15, 2016

Member

is char* really the best option here? MoveIt! almost always uses std::string - and this structure gets inserted into an std::string set

@davetcoleman

davetcoleman Nov 15, 2016

Member

is char* really the best option here? MoveIt! almost always uses std::string - and this structure gets inserted into an std::string set

This comment has been minimized.

@v4hn

v4hn Nov 15, 2016

Member

I call bike shed. I don't see how this makes any difference.

Incidentally, I used vector<string> before, but this requires braced-initializer-lists from C++11 and I decided to write this standard-agnostic because I'm unsure whether to backport it.
So I changed it to full C-style structure instead.

@v4hn

v4hn Nov 15, 2016

Member

I call bike shed. I don't see how this makes any difference.

Incidentally, I used vector<string> before, but this requires braced-initializer-lists from C++11 and I decided to write this standard-agnostic because I'm unsure whether to backport it.
So I changed it to full C-style structure instead.

This comment has been minimized.

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Nov 15, 2016

Member

Backporting: the adverse affect I see is that users who purposefully removed a capability in the past, for example because they created their own custom version, will suddenly have it re-enabled during a routine update. But I've never heard of anyone doing this and am not sure what use-cases there are for it. I think if we announce this feature on the mailing list with a small warning, its safe to backport.

Member

davetcoleman commented Nov 15, 2016

Backporting: the adverse affect I see is that users who purposefully removed a capability in the past, for example because they created their own custom version, will suddenly have it re-enabled during a routine update. But I've never heard of anyone doing this and am not sure what use-cases there are for it. I think if we announce this feature on the mailing list with a small warning, its safe to backport.

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Nov 16, 2016

Member

ok, merging.
I will create a pull-request to backport this to indigo

Member

v4hn commented Nov 16, 2016

ok, merging.
I will create a pull-request to backport this to indigo

@v4hn v4hn merged commit dabe57f into ros-planning:kinetic-devel Nov 16, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

v4hn added a commit to v4hn/moveit that referenced this pull request Nov 16, 2016

add list of default capabilities (#359)
Functionality in MoveIt!'s move_group node is implemented in terms of
MoveGroupCapability plugins. But until now the choice of which plugins
are loaded was solely left to the user who in many cases is not familiar
with these plugins at all. This entails that whenever we introduce or change
a basic capability we have to annoy the user with warnings on startup that
bug him to change and/or update his move_group.launch file.

This commit changes the startup behavior of move_group to load a specified
list of default capabilities and provides a new ROS parameter `disable_capabilities`
that can be used to disable default plugins.

Fixes #125.

v4hn added a commit to v4hn/moveit that referenced this pull request Nov 17, 2016

add list of default capabilities (#359)
Functionality in MoveIt!'s move_group node is implemented in terms of
MoveGroupCapability plugins. But until now the choice of which plugins
are loaded was solely left to the user who in many cases is not familiar
with these plugins at all. This entails that whenever we introduce or change
a basic capability we have to annoy the user with warnings on startup that
bug him to change and/or update his move_group.launch file.

This commit changes the startup behavior of move_group to load a specified
list of default capabilities and provides a new ROS parameter `disable_capabilities`
that can be used to disable default plugins.

Fixes #125.

davetcoleman added a commit that referenced this pull request Nov 17, 2016

add list of default capabilities (#359) (#366)
Functionality in MoveIt!'s move_group node is implemented in terms of
MoveGroupCapability plugins. But until now the choice of which plugins
are loaded was solely left to the user who in many cases is not familiar
with these plugins at all. This entails that whenever we introduce or change
a basic capability we have to annoy the user with warnings on startup that
bug him to change and/or update his move_group.launch file.

This commit changes the startup behavior of move_group to load a specified
list of default capabilities and provides a new ROS parameter `disable_capabilities`
that can be used to disable default plugins.

Fixes #125.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment