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

Add time-optimal trajectory parameterization (continues #809) #1365

Merged

Conversation

Projects
None yet
6 participants
@henningkayser
Copy link
Contributor

commented Feb 26, 2019

This is an effort to finish PR #809 by @mikeferguson for integrating the time-optimal trajectory parameterization method (published here).

The branch is retargeted to melodic and I implemented a function for computing acceleration values (the original version doesn't do this).

Left TODOs:

  • process pending review by @davetcoleman
  • extend API to support tolerance and resample parameters
  • debug edge cases mentioned in the original comments section

- Sponsored by Acutronic and PickNik -

Time-optimal Trajectory Parameterization

totg

Iterative Spline Parameterization

isp

Iterative Parabolic Time Parameterization

iptp

@davetcoleman

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

Thanks @henningkayser !

extend API to support tolerance and resample parameters

Is this feature necessary for this first PR? I'd rather just get the very stale PR merged so it gets into moveit2, even if its not feature complete. We don't want huge PRs.

@mikeferguson

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

@henningkayser thanks for picking this up and getting it across the line!

@mikeferguson

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

I'd also make one recommendation to maintainers -- don't squash this before merging, so that somebody can still figure out the changes we made from the original codebase (in case somebody wants to later import patches or whatnot).

@henningkayser henningkayser changed the title WIP: Add time-optimal trajectory parameterization (continues #809) Add time-optimal trajectory parameterization (continues #809) Feb 27, 2019

@henningkayser

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

@davetcoleman I addressed your requested changes from #809. However, I would suggest we don't separate the classes into different files. Since we don't have any other code depending on it, separate files would just make the package more complicated without any real upside.

I also added some improvements to the implementation:

  • The timestamp of the first waypoint is set to 0 (instead of a certain dt before)
  • The last waypoint is always sampled from the last point in the processed path, fixing occasional non-zero end velocities
  • trajectories that only contain the same waypoints are handled (fixes issue with planning to the current state)
  • computeTimeStamps is now a member function of a 'TimeOptimalTrajectoryGeneration' class that allows setting tolerance and sampling parameters in the constructor

@hartmanndennis can you confirm that the issues you described are fixed?

I'd also make one recommendation to maintainers -- don't squash this before merging, so that somebody can still figure out the changes we made from the original codebase (in case somebody wants to later import patches or whatnot).

I agree with @mikeferguson but I should probably squash my commits before merging.

@davetcoleman

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

However, I would suggest we don't separate the classes into different files.

No problem

@davetcoleman

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

@henningkayser can you squash some of your commits together, but not the original author's commit per @mikeferguson ?

@henningkayser henningkayser force-pushed the henningkayser:melodic-devel-pr_totg branch from 27f7efd to ce9b36e Mar 6, 2019

@henningkayser henningkayser force-pushed the henningkayser:melodic-devel-pr_totg branch from ce9b36e to 7653af0 Mar 6, 2019

@henningkayser

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

@henningkayser can you squash some of your commits together, but not the original author's commit per @mikeferguson ?

Done

@hartmanndennis

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

@hartmanndennis can you confirm that the issues you described are fixed?

Both issues seem to be fixed. Great Work!

@rhaschke rhaschke merged commit 3d32f9a into ros-planning:melodic-devel Mar 7, 2019

1 check passed

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

@henningkayser henningkayser referenced this pull request Mar 7, 2019

Closed

Add time-optimal trajectory parameterization plugin #809

1 of 5 tasks complete
@davetcoleman

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

Whoo hoo! @henningkayser I think it would be worth announcing this new feature on Discourse, giving credit also to @mikeferguson and original author Tobias Kuntz. Could you do that, including a pointer to the new tutorial?

@mikeferguson

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

Thanks for getting this across the line (and creating plots to show just how much better this algorithm is).

@hartmanndennis

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

Will this be cherry-picked to kinetic?

@rhaschke

This comment has been minimized.

Copy link
Collaborator

commented Mar 8, 2019

Will this be cherry-picked to kinetic?

I would say no. Maintaining both branches is a lot of work, and a subset of maintainers is now focussing on MoveIt for ROS2 already. You can compile MoveIt Melodic on Kinetic, if you like to have those changes in Kinetic.

@gavanderhoorn

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

@hartmanndennis: please also note that as nice as it is, TOTG is not jerk-limited, so if you plan on using it with KUKA robots over RSI, I don't expect it -- but I would really like to -- solve all issues there.

@henningkayser henningkayser deleted the henningkayser:melodic-devel-pr_totg branch Mar 8, 2019

@davetcoleman

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

+1 to compiling the master branch with your ROS Kinetic environment - its compatible!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.