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

rename planning_interface::MoveGroup to planning_interface::MoveGroupInterface #37

Closed
v4hn opened this Issue Aug 11, 2016 · 8 comments

Comments

Projects
None yet
3 participants
@v4hn
Member

v4hn commented Aug 11, 2016

An often observed source of confusion in MoveIt is the class "MoveGroup".

While everyone instantly believes to recognize this as the node that encapsulates most of MoveIt's functionality in ROS, this class actually lives in "planning_interface" and only provides a client interface to interact with the node via ROS-interfaces. It is often believed that most of the class methods call library functions and do not wrap ROS interfaces.

As such it should be renamed "MoveGroupInterface" in kinetic. This includes a number of steps (in order)

  • move move_group.h to move_group_interface.h and rename the class
  • provide a new move_group.h that provides a typedef named MoveGroup to be downward compatible
  • rename the class throughout MoveIt's own code base
  • adjust the tutorials to the new name
  • add a deprecation notice to move_group.h
@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Aug 11, 2016

Member

I want to clarify what you are saying because it was hard to follow. The problem is that there are two files with the name move_group.cpp and one is confusing.

The executable moveit_ros/move_group/src/move_group.cpp is the ROS node that encapsulates most of MoveIt's functionality in ROS. When it is launched it creates a bunch of ROS-msg/service/action interfaces that allow you to do planning, etc. It is the central "brain" in MoveIt!.

The class planning_interface/move_group_interface/src/move_group.cpp is the interface library for interacting with MoveIt! via ROS comm (as opposed to directly as a library). It would be better if it was renamed to match the folder it lives in - move_group_interface.cpp

+1 to @v4hn's idea

Member

davetcoleman commented Aug 11, 2016

I want to clarify what you are saying because it was hard to follow. The problem is that there are two files with the name move_group.cpp and one is confusing.

The executable moveit_ros/move_group/src/move_group.cpp is the ROS node that encapsulates most of MoveIt's functionality in ROS. When it is launched it creates a bunch of ROS-msg/service/action interfaces that allow you to do planning, etc. It is the central "brain" in MoveIt!.

The class planning_interface/move_group_interface/src/move_group.cpp is the interface library for interacting with MoveIt! via ROS comm (as opposed to directly as a library). It would be better if it was renamed to match the folder it lives in - move_group_interface.cpp

+1 to @v4hn's idea

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Aug 11, 2016

Member

I want to clarify what you are saying because it was hard to follow.

Thanks, I'm a bit under the weather today. I see that my explanation was hard to follow...

You got it right.

[OBSOLETE DUE TO EDIT ABOVE]

Two minor corrections:

  • There is only one class MoveGroup. It's just that the loose term "move group" is generally associated with the node and therefore the class should actually be called MoveGroupInterface.
    (moveit_ros/move_group/move_group.cpp actually uses the name MoveGroupExe internally)
  • move_group (the node) is not a library. It's a ROS node. It's as simple as that. :-)
Member

v4hn commented Aug 11, 2016

I want to clarify what you are saying because it was hard to follow.

Thanks, I'm a bit under the weather today. I see that my explanation was hard to follow...

You got it right.

[OBSOLETE DUE TO EDIT ABOVE]

Two minor corrections:

  • There is only one class MoveGroup. It's just that the loose term "move group" is generally associated with the node and therefore the class should actually be called MoveGroupInterface.
    (moveit_ros/move_group/move_group.cpp actually uses the name MoveGroupExe internally)
  • move_group (the node) is not a library. It's a ROS node. It's as simple as that. :-)
@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Aug 11, 2016

Member

Good points. I've updated my previous comment to be a bit more semantically correct.

Member

davetcoleman commented Aug 11, 2016

Good points. I've updated my previous comment to be a bit more semantically correct.

@rhaschke

This comment has been minimized.

Show comment
Hide comment
@rhaschke

rhaschke Aug 16, 2016

Contributor

+1

Contributor

rhaschke commented Aug 16, 2016

+1

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Sep 26, 2016

Member

#251 implements the proposal. Discussion should be continued there.

Member

v4hn commented Sep 26, 2016

#251 implements the proposal. Discussion should be continued there.

@v4hn v4hn closed this Sep 26, 2016

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Sep 26, 2016

Member

I like your checklist, above. It seems this issue should not actually be closed yet

Member

davetcoleman commented Sep 26, 2016

I like your checklist, above. It seems this issue should not actually be closed yet

@v4hn v4hn reopened this Sep 26, 2016

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Sep 26, 2016

Member

you're right, I missed the tutorial updates.
Everything else is addressed in the pull-request

Member

v4hn commented Sep 26, 2016

you're right, I missed the tutorial updates.
Everything else is addressed in the pull-request

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman
Member

davetcoleman commented Oct 26, 2016

Rayman added a commit to tue-robotics/tue_manipulation that referenced this issue Jan 14, 2017

cschindlbeck pushed a commit to cschindlbeck/moveit that referenced this issue Aug 16, 2017

Merge pull request #37 from ferherranz/indigo-devel
Added set_max_velocity_scaling_factor function to move_group.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment