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

[Python] Add RetimeTrajectory to RobotTrajectory #2411

Merged
merged 7 commits into from Oct 11, 2023

Conversation

JensVanhooydonck
Copy link
Contributor

Description

This adds the RetimeTrajectory to the RobotTrajectory binding. This was available in the ROS1 movegroup python interface. This feature allows to plan a trajectory and change the trajectory speeds (velocity and acceleration values).

Currently the two algoritms inside trajectory_processing where added: Ruckig Trajectory Smoothing and Time Optimal Trajectory Generation.

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@codecov
Copy link

codecov bot commented Oct 7, 2023

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (38668bf) 50.37% compared to head (8833f75) 50.37%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2411      +/-   ##
==========================================
- Coverage   50.37%   50.37%   -0.00%     
==========================================
  Files         385      385              
  Lines       31815    31829      +14     
==========================================
+ Hits        16025    16031       +6     
- Misses      15790    15798       +8     
Files Coverage Δ
.../moveit_core/robot_trajectory/robot_trajectory.cpp 77.15% <100.00%> (+4.73%) ⬆️
...ore/trajectory_processing/src/trajectory_tools.cpp 0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@sjahr sjahr 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 your contribution! While I like the idea of having this functionality I think there a are some things that should be reconsidered:

  • Rather than having a single retimeTrajectory() function that does not expose specific paramters of the individual algorithms + you can give it a non-existing algorithm name, I'd prefer having two more specific functions for example:
addTOTGTimeParameterization(std::shared_ptr<robot_trajectory::RobotTrajectory>& self, const std::string& algorithm,
                      const double& velocity_scaling_factor, const double& acceleration_scaling_factor);
                      
applyRuckigSmoothing(std::shared_ptr<robot_trajectory::RobotTrajectory>& self, const std::string& algorithm,
                      const double& velocity_scaling_factor, const double& acceleration_scaling_factor, const bool mitigate_overshoot = false,
                                     const double overshoot_threshold = 0.01);
  • These functions shouldn't be implemented in the python bindings package but as free functions in moveit_core/trajectory_processing (to avoid a circular dependency between RobotTrajectory and the time parameterization algorithms) e.g. here

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.

I do like the idea to abstract away trajectory_processing in python, so per se I don't think it's wrong to have the functionality as methods of RobotTrajectory. It's important to provide full support for the involved parameters though and the moveit method was quite a hack in the MoveGroupInterface wrapper class.

No strong feelings on free functions in separate namespace vs the methods, though some maintainers clearly prefer the functional style.

Copy link
Contributor

@sjahr sjahr left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@sjahr sjahr requested a review from v4hn October 10, 2023 16:00
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.

Looks good assuming it's tested.

@JensVanhooydonck
Copy link
Contributor Author

Looks good assuming it's tested.

Tested both functions already in my python script.

@sjahr sjahr enabled auto-merge (squash) October 11, 2023 06:38
@sjahr sjahr merged commit e7577ca into moveit:main Oct 11, 2023
7 of 8 checks passed
@henningkayser henningkayser added the backport-iron Mergify label that triggers a PR backport to Iron label Oct 24, 2023
mergify bot pushed a commit that referenced this pull request Oct 24, 2023
* [Python] Add RetimeTrajectory to RobotTrajectory

* Split retime trajecotry in multiple functions
Moved logic to trajectory_tools
Added Docstrings

* Removed retime function from python binding

---------

Co-authored-by: Sebastian Jahr <sebastian.jahr@picknik.ai>
(cherry picked from commit e7577ca)
henningkayser pushed a commit that referenced this pull request Oct 24, 2023
)

* [Python] Add RetimeTrajectory to RobotTrajectory

* Split retime trajecotry in multiple functions
Moved logic to trajectory_tools
Added Docstrings

* Removed retime function from python binding

---------

Co-authored-by: Sebastian Jahr <sebastian.jahr@picknik.ai>
(cherry picked from commit e7577ca)

Co-authored-by: Jens Vanhooydonck <jensvanhooydonck@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-iron Mergify label that triggers a PR backport to Iron
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants