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

New moveit_kinematics package consolidates IK solvers #247

Merged
merged 3 commits into from Sep 28, 2016

Conversation

Projects
None yet
4 participants
@davetcoleman
Member

davetcoleman commented Sep 23, 2016

Addresses the versioning issue for moveit_ikfast and the idea proposed in #67

Because we are just moving plugins this does not break any dependencies.

I have changed no code for any of the plugins, simply moved them to a new location within the moveit repo. This will allow us to release moveit_ikfast in I/J/K, and therefore should be cherry-picked to all three branches.

I have more plans for the ikfast plugin, but will save them for another PR.

@gavanderhoorn

This comment has been minimized.

Show comment
Hide comment
@gavanderhoorn

gavanderhoorn Sep 24, 2016

Member

Impopular maybe, but I'm not convinced that placing moveit_ikfast together with all the other plugins is a good idea. For my reasoning, see #67 (comment) and #67 (comment).

Member

gavanderhoorn commented Sep 24, 2016

Impopular maybe, but I'm not convinced that placing moveit_ikfast together with all the other plugins is a good idea. For my reasoning, see #67 (comment) and #67 (comment).

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Sep 26, 2016

Member

Ready to merge

Member

davetcoleman commented Sep 26, 2016

Ready to merge

Show outdated Hide outdated moveit_kinematics/ikfast_kinematics_plugin/README.md
@@ -0,0 +1,9 @@
# MoveIt! IKFast

This comment has been minimized.

@jrgnicho

jrgnicho Sep 27, 2016

Contributor

This file shows as if it was added rather than moved, not sure that it can be easily fixed but it be nice if it kept the history.

@jrgnicho

jrgnicho Sep 27, 2016

Contributor

This file shows as if it was added rather than moved, not sure that it can be easily fixed but it be nice if it kept the history.

This comment has been minimized.

@davetcoleman

davetcoleman Sep 27, 2016

Member

ive restored the file to its original state so that it can be tracked

@davetcoleman

davetcoleman Sep 27, 2016

Member

ive restored the file to its original state so that it can be tracked

Show outdated Hide outdated moveit_kinematics/ikfast_kinematics_plugin/README.md
Generates a IKFast kinematics plugin for MoveIt using [OpenRave](http://openrave.org/) generated cpp files.
Tested on ROS Hydro and greater with Catkin using OpenRave 0.8 with a 6dof and 7dof robot arm manipulator. Does not work with greater than 7dof.

This comment has been minimized.

@jrgnicho

jrgnicho Sep 27, 2016

Contributor

I know this PR is about rearranging packages but I'm not sure that this line still makes sense to keep as it has to have been tested for this code to make it into the main development branch.

@jrgnicho

jrgnicho Sep 27, 2016

Contributor

I know this PR is about rearranging packages but I'm not sure that this line still makes sense to keep as it has to have been tested for this code to make it into the main development branch.

This comment has been minimized.

@davetcoleman

davetcoleman Sep 27, 2016

Member

this should be changed/discussed in a different PR.

@davetcoleman

davetcoleman Sep 27, 2016

Member

this should be changed/discussed in a different PR.

Show outdated Hide outdated moveit_kinematics/package.xml
<run_depend>moveit_core</run_depend>
<run_depend>pluginlib</run_depend>
<run_depend>actionlib</run_depend>
<run_depend>angles</run_depend>

This comment has been minimized.

@jrgnicho

jrgnicho Sep 27, 2016

Contributor

Is angles not a build dependency also?

@jrgnicho

jrgnicho Sep 27, 2016

Contributor

Is angles not a build dependency also?

This comment has been minimized.

@jrgnicho

jrgnicho Sep 27, 2016

Contributor

I feel that openrave should be listed as a run dependency since the ikfast script won't be able to do anything without the solution generated by openrave.

@jrgnicho

jrgnicho Sep 27, 2016

Contributor

I feel that openrave should be listed as a run dependency since the ikfast script won't be able to do anything without the solution generated by openrave.

This comment has been minimized.

@davetcoleman

davetcoleman Sep 27, 2016

Member

good catch on the angles run_depend, it should not be there (removed)

@davetcoleman

davetcoleman Sep 27, 2016

Member

good catch on the angles run_depend, it should not be there (removed)

This comment has been minimized.

@davetcoleman

davetcoleman Sep 27, 2016

Member

openrave definitely should not be added as a dependency

  • it would be huge install requirement imposed on all moveit users
  • they haven't kept up releasing with the latest Ubuntu versions AFAIK
  • openrave is only needed once during header file generation, after which a generated ikfast plugin can be used indefinitely without openrave
  • our documentation explains to users how to manually install openrave
@davetcoleman

davetcoleman Sep 27, 2016

Member

openrave definitely should not be added as a dependency

  • it would be huge install requirement imposed on all moveit users
  • they haven't kept up releasing with the latest Ubuntu versions AFAIK
  • openrave is only needed once during header file generation, after which a generated ikfast plugin can be used indefinitely without openrave
  • our documentation explains to users how to manually install openrave

@davetcoleman davetcoleman referenced this pull request Sep 27, 2016

Closed

Kinetic Release #18

19 of 19 tasks complete

@davetcoleman davetcoleman added this to the Kinetic Release milestone Sep 27, 2016

@jrgnicho

This comment has been minimized.

Show comment
Hide comment
@jrgnicho

jrgnicho Sep 27, 2016

Contributor

I agree with you on the dependency burden that it would cause. Since there
are instructions for installing openrave then it's ok. We should just make
sure that those instructions still work on kinetic and ubuntu 16.04.
On Sep 26, 2016 10:16 PM, "Dave Coleman" notifications@github.com wrote:

@davetcoleman commented on this pull request.

In moveit_kinematics/package.xml
#247:

  • <buildtool_depend>catkin</buildtool_depend>
  • <build_depend>moveit_core</build_depend>
  • <build_depend>pluginlib</build_depend>
  • <build_depend>actionlib</build_depend>
  • <build_depend>eigen</build_depend>
  • <build_depend>moveit_ros_planning</build_depend>
  • <run_depend>moveit_core</run_depend>
  • <run_depend>pluginlib</run_depend>
  • <run_depend>actionlib</run_depend>
  • <run_depend>angles</run_depend>

openrave definitely should not be added as a dependency

  • it would be huge install requirement imposed on all moveit users
  • they haven't kept up releasing with the latest Ubuntu versions AFAIK
  • openrave is only needed once during header file generation, after
    which a generated ikfast plugin can be used indefinitely without openrave
  • our documentation explains to users how to manually install openrave


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#247, or mute the thread
https://github.com/notifications/unsubscribe-auth/ADILZlFHzNS5kFFvARMUojC3D345LAQwks5quKbHgaJpZM4KFSyz
.

Contributor

jrgnicho commented Sep 27, 2016

I agree with you on the dependency burden that it would cause. Since there
are instructions for installing openrave then it's ok. We should just make
sure that those instructions still work on kinetic and ubuntu 16.04.
On Sep 26, 2016 10:16 PM, "Dave Coleman" notifications@github.com wrote:

@davetcoleman commented on this pull request.

In moveit_kinematics/package.xml
#247:

  • <buildtool_depend>catkin</buildtool_depend>
  • <build_depend>moveit_core</build_depend>
  • <build_depend>pluginlib</build_depend>
  • <build_depend>actionlib</build_depend>
  • <build_depend>eigen</build_depend>
  • <build_depend>moveit_ros_planning</build_depend>
  • <run_depend>moveit_core</run_depend>
  • <run_depend>pluginlib</run_depend>
  • <run_depend>actionlib</run_depend>
  • <run_depend>angles</run_depend>

openrave definitely should not be added as a dependency

  • it would be huge install requirement imposed on all moveit users
  • they haven't kept up releasing with the latest Ubuntu versions AFAIK
  • openrave is only needed once during header file generation, after
    which a generated ikfast plugin can be used indefinitely without openrave
  • our documentation explains to users how to manually install openrave


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#247, or mute the thread
https://github.com/notifications/unsubscribe-auth/ADILZlFHzNS5kFFvARMUojC3D345LAQwks5quKbHgaJpZM4KFSyz
.

@gavanderhoorn

This comment has been minimized.

Show comment
Hide comment
@gavanderhoorn

gavanderhoorn Sep 27, 2016

Member

@jrgnicho wrote:

I agree with you on the dependency burden that it would cause. Since there are instructions for installing openrave then it's ok. We should just make sure that those instructions still work on kinetic and ubuntu 16.04.

OpenRAVE installation on anything but Ubuntu 12.04 is notoriously difficult / involved (see the many installation related issues on the OpenRAVE tracker). I've used a Docker image in the past to avoid all that.

Related: rdiankov/openrave#394.

Member

gavanderhoorn commented Sep 27, 2016

@jrgnicho wrote:

I agree with you on the dependency burden that it would cause. Since there are instructions for installing openrave then it's ok. We should just make sure that those instructions still work on kinetic and ubuntu 16.04.

OpenRAVE installation on anything but Ubuntu 12.04 is notoriously difficult / involved (see the many installation related issues on the OpenRAVE tracker). I've used a Docker image in the past to avoid all that.

Related: rdiankov/openrave#394.

@jrgnicho

This comment has been minimized.

Show comment
Hide comment
@jrgnicho

jrgnicho Sep 28, 2016

Contributor

@gavanderhoorn I now remember running into this issue with OpenRave a while ago. @davetcoleman it feels to me that there isn't much of a point in having the ikfast scripts in indigo, jade or kinetic if they can't be used unless a rather involved workaround is used as suggested by @gavanderhoorn . The alternative to not having ikfast is to grab the ik generating code from OpenRave and build it into moveit_kinematics, I realize this would be non trivial but it's perhaps the right approach.

Contributor

jrgnicho commented Sep 28, 2016

@gavanderhoorn I now remember running into this issue with OpenRave a while ago. @davetcoleman it feels to me that there isn't much of a point in having the ikfast scripts in indigo, jade or kinetic if they can't be used unless a rather involved workaround is used as suggested by @gavanderhoorn . The alternative to not having ikfast is to grab the ik generating code from OpenRave and build it into moveit_kinematics, I realize this would be non trivial but it's perhaps the right approach.

@gavanderhoorn

This comment has been minimized.

Show comment
Hide comment
@gavanderhoorn

gavanderhoorn Sep 28, 2016

Member

I've had some success using personalrobotics/ros-openrave.

Not sure whether we want to depend on an outside image, but it might be an idea.

The alternative to not having ikfast is to grab the ik generating code from OpenRave and build it into moveit_kinematics, I realize this would be non trivial but it's perhaps the right approach.

I'm not sure that would be feasible. I've seen people install just enough to get the kinematics solver generation working, but extracting it would be a something else entirely.

Member

gavanderhoorn commented Sep 28, 2016

I've had some success using personalrobotics/ros-openrave.

Not sure whether we want to depend on an outside image, but it might be an idea.

The alternative to not having ikfast is to grab the ik generating code from OpenRave and build it into moveit_kinematics, I realize this would be non trivial but it's perhaps the right approach.

I'm not sure that would be feasible. I've seen people install just enough to get the kinematics solver generation working, but extracting it would be a something else entirely.

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Sep 28, 2016

Member

@k-okada just recently updated our tutorials for Ubuntu 14.04 using the newly released binary packages of OpenRave, so it is working in indigo and hopefully soon 16.04. You can also still install OpenRave from source for 16.04, so there is value in having the ikfast code.

grab the ik generating code from OpenRave and build it into moveit_kinematics

This would be extremely hard (I started on that task years ago), as Rosen's coding style is difficult and he also uses a different robot model format (Collada).

We've long been happy with the moveit_ikfast package, but since the repo merge we haven't been able to release it because its version number was too high (3.x). This is the workaround suggested by @v4hn . I wasn't trying to debate the existence of moveit_ikfast in this PR, but merely move it from its current location in moveit/moveit_ikfast.

Member

davetcoleman commented Sep 28, 2016

@k-okada just recently updated our tutorials for Ubuntu 14.04 using the newly released binary packages of OpenRave, so it is working in indigo and hopefully soon 16.04. You can also still install OpenRave from source for 16.04, so there is value in having the ikfast code.

grab the ik generating code from OpenRave and build it into moveit_kinematics

This would be extremely hard (I started on that task years ago), as Rosen's coding style is difficult and he also uses a different robot model format (Collada).

We've long been happy with the moveit_ikfast package, but since the repo merge we haven't been able to release it because its version number was too high (3.x). This is the workaround suggested by @v4hn . I wasn't trying to debate the existence of moveit_ikfast in this PR, but merely move it from its current location in moveit/moveit_ikfast.

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Sep 28, 2016

Member

Note Travis is passing for shadow-fixed, so should be merged even though #257 hasn't been merged in

Member

davetcoleman commented Sep 28, 2016

Note Travis is passing for shadow-fixed, so should be merged even though #257 hasn't been merged in

@jrgnicho

This comment has been minimized.

Show comment
Hide comment
@jrgnicho

jrgnicho Sep 28, 2016

Contributor

@davetcoleman sounds like there are viable ways to run the openrave ik generator from what you described. I'm ok with the changes here, however shouldn't travis be entirely error free before merging?

Contributor

jrgnicho commented Sep 28, 2016

@davetcoleman sounds like there are viable ways to run the openrave ik generator from what you described. I'm ok with the changes here, however shouldn't travis be entirely error free before merging?

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Sep 28, 2016

Member

In this case no - Travis will not pass on the non-shadow-fixed build until the next build farm sync. Click on "Details" next to the failed build, above, and you'll see the shadow-fixed passed.

If someone would merge #257 Travis would pass, also...

But I'd rather just merge this now

Member

davetcoleman commented Sep 28, 2016

In this case no - Travis will not pass on the non-shadow-fixed build until the next build farm sync. Click on "Details" next to the failed build, above, and you'll see the shadow-fixed passed.

If someone would merge #257 Travis would pass, also...

But I'd rather just merge this now

@v4hn v4hn merged commit c922860 into ros-planning:kinetic-devel Sep 28, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Sep 28, 2016

Member

thanks for the cleanup @davetcoleman!
Could you please also update the links on the website?

Member

v4hn commented Sep 28, 2016

thanks for the cleanup @davetcoleman!
Could you please also update the links on the website?

@davetcoleman

This comment has been minimized.

Show comment
Hide comment

@davetcoleman davetcoleman deleted the davetcoleman:kinetic-kinematic-pkg branch Sep 29, 2016

@davetcoleman davetcoleman restored the davetcoleman:kinetic-kinematic-pkg branch Sep 29, 2016

130s added a commit to 130s/aubo_robot that referenced this pull request Dec 29, 2016

130s added a commit to 130s/aubo_robot that referenced this pull request Dec 29, 2016

@davetcoleman davetcoleman deleted the davetcoleman:kinetic-kinematic-pkg branch Dec 31, 2016

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