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

Select time parametrization algorithm in retime_trajectory #1508

Merged
merged 3 commits into from Jul 4, 2019

Conversation

mmurooka
Copy link
Contributor

Description

Enable to select Time Parametrization Algorithm via algorithm argument in retime_trajectory method of Python MoveGroupCommander.

Ref: Time Parametrization Algorithms:
http://docs.ros.org/melodic/api/moveit_tutorials/html/doc/time_parameterization/time_parameterization_tutorial.html#time-parameterization-algorithms

Example use case:
https://github.com/mmurooka/moveit_tutorials/blob/time-parametrization-tutorial-master/demo_rs007n/scripts/demo_adept_motion_rs007n.py#L93-L102

(This PR contains #1506 .)

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extended the tutorials / documentation, if necessary reference
  • Include a screenshot if changing a GUI
  • Document API changes relevant to the user in the moveit/MIGRATION.md notes
  • Created tests, which fail without this PR reference
  • Decide if this should be cherry-picked to other current ROS branches
  • While waiting for someone to review your request, please consider reviewing another open pull request to support the maintainers

Copy link
Contributor

@v4hn v4hn left a comment

Choose a reason for hiding this comment

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

Please address the feedback.

I believe we need to make the time parametrization configurable for every place it might be called, so cleaning this up would be more involved...

Copy link
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

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

Thank you for these changes.
Could you please add a simplistic unit test files that runs all types of time parametrization?

@mmurooka
Copy link
Contributor Author

mmurooka commented Jul 2, 2019

@v4hn Fixed the source code according to the review.
@bmagyar Added the unit test.

@v4hn v4hn added the awaits 2nd review one maintainer approved this request label Jul 3, 2019
@BryceStevenWilley BryceStevenWilley merged commit d00ef75 into moveit:melodic-devel Jul 4, 2019
@welcome
Copy link

welcome bot commented Jul 4, 2019

Congrats on getting your first MoveIt! pull request merged and improving open source robotics!

BryceStevenWilley pushed a commit to BryceStevenWilley/moveit that referenced this pull request Jul 4, 2019
* Python wrapper: add acceleration_scaling_factor argument to retime_trajectory.

* wrap_python_move_group.cpp, move_group.py: add algorithm option to retime_trajectory in pyhon interface of move_group.

* moveit_commander: add python_time_parameterization test.
rhaschke pushed a commit that referenced this pull request Jul 4, 2019
* Python wrapper: add acceleration_scaling_factor argument to retime_trajectory.

* wrap_python_move_group.cpp, move_group.py: add algorithm option to retime_trajectory in pyhon interface of move_group.

* moveit_commander: add python_time_parameterization test.
@mmurooka mmurooka deleted the select-retime-alg branch July 5, 2019 09:58
@@ -489,7 +491,8 @@ class MoveGroupInterfaceWrapper : protected py_bindings_tools::ROScppInitializer
}

std::string retimeTrajectory(const std::string& ref_state_str, const std::string& traj_str,
double velocity_scaling_factor)
double velocity_scaling_factor, double acceleration_scaling_factor,
std::string algorithm)
Copy link
Member

Choose a reason for hiding this comment

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

This has caused a Travis breakage on master, it looks like it got through because this was committed to melodic first, then cherry-picked?

Copy link
Member

Choose a reason for hiding this comment

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

Fix: #1589

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. Sorry for the inconvenience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaits 2nd review one maintainer approved this request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants