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

make time_optimal_trajectory_generation harder to misuse #2624

Merged
merged 7 commits into from Jan 3, 2024

Conversation

marioprats
Copy link
Contributor

This change hardens the Path and Trajectory APIs in the trajectory_processing package, by checking for a couple of conditions that are not well handled by the algorithm:

  1. Paths that make a 180 deg. turn are not supported by the current implementation, but the API would still return a valid trajectory (even if it is actually invalid). That's now checked when creating a Path, and the path won't be constructed if it makes a 180 deg. turn.
  2. Paths with a max_deviation of 0.0 (default) create trajectories that are not valid, even if isValid() returns true. This is because such paths don't have blending at the corners, and therefore their parameterizations are not differentiable. The algorithm completes and isValid() returns true, but the resulting trajectory has inconsistent velocities and accelerations.

These conditions are now checked and handled appropriately. A path_tolerance of 0.0 is no longer an option. The default path_tolerance is now consistent between Path and TimeOptimalTrajectoryGeneration.
To make this possible I had to change the API to use builder methods (to return error), and make the constructors private.

A side-effect is that Trajectory::isValid() is no longer needed. Trajectory::Create() will return std::nullopt if the trajectory can't be created.

I also changed Path construction to take the waypoints as a std::vector instead of std::list, because std::vector is a more common representation for a path, and should avoid requiring the user to make the conversion (and a copy) in most cases. This is a bit unrelated to the original issue but I made the change since I was changing the API anyway.

Finally, adds test for the failing cases.

Closes #2600

Copy link

codecov bot commented Dec 20, 2023

Codecov Report

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

Comparison is base (978bd4c) 51.00% compared to head (8f3f9d0) 50.68%.
Report is 1 commits behind head on main.

❗ Current head 8f3f9d0 differs from pull request most recent head bf436c3. Consider uploading reports for the commit bf436c3 to get more accurate results

Files Patch % Lines
...cessing/src/time_optimal_trajectory_generation.cpp 85.72% 13 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2624      +/-   ##
==========================================
- Coverage   51.00%   50.68%   -0.32%     
==========================================
  Files         387      386       -1     
  Lines       32308    32151     -157     
==========================================
- Hits        16476    16293     -183     
- Misses      15832    15858      +26     

☔ 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.

Thanks!

@sjahr sjahr enabled auto-merge (squash) January 3, 2024 08:51
@sjahr sjahr merged commit cdd8d14 into moveit:main Jan 3, 2024
9 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TOTG not respecting acceleration limits
2 participants