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

[WIP] Cartesian Tool Path Planning #1946

Closed
wants to merge 9 commits into from

Conversation

sea-bass
Copy link
Contributor

@sea-bass sea-bass commented Feb 13, 2023

Description

This reimplements #377 with the suggestions from review and continues the work to make it a complete feature.

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 Feb 13, 2023

Codecov Report

Base: 50.30% // Head: 50.18% // Decreases project coverage by -0.12% ⚠️

Coverage data is based on head (a1ebdf6) compared to base (bff9600).
Patch coverage: 0.00% of modified lines in pull request are covered.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1946      +/-   ##
==========================================
- Coverage   50.30%   50.18%   -0.12%     
==========================================
  Files         374      374              
  Lines       31358    31438      +80     
==========================================
+ Hits        15772    15773       +1     
- Misses      15586    15665      +79     
Impacted Files Coverage Δ
...de/moveit/ompl_interface/detail/ompl_constraints.h 0.00% <0.00%> (ø)
...mpl/ompl_interface/src/detail/ompl_constraints.cpp 0.00% <0.00%> (ø)
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp 78.04% <0.00%> (-1.13%) ⬇️
...nning_scene_monitor/src/planning_scene_monitor.cpp 45.69% <0.00%> (+0.44%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@sea-bass
Copy link
Contributor Author

sea-bass commented Feb 15, 2023

The code is now at the point where I can pass in a path constraint, it gets instantiated, and we actually get a plan.

However, the plan right now either fails because of tight bounds or succeeds but doesn't honor the tool path at all.

There are lots of OMPL interface nuances that are still mysterious to me, and I may need some help with this to head in the right direction vs. just doing things with trial and error.

@mamoll
Copy link
Contributor

mamoll commented Feb 15, 2023

@zkingston do you have time to take a look?

Copy link
Contributor

@zkingston zkingston left a comment

Choose a reason for hiding this comment

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

Could you elaborate more on the failure case? e.g., when you say the tolerances are too tight and planning fails. I took a quick look through the code.

@sea-bass
Copy link
Contributor Author

Could you elaborate more on the failure case? e.g., when you say the tolerances are too tight and planning fails. I took a quick look through the code.

Thanks for looking at this!

I don't know if I have anything super specific rather than generally trying to understand how this all works. Some pointed questions:

  • What does the bounds_ member correspond to for a constraint like this. Should it be the bounds of the workspace? The bounds for each of the elements in the error function? Something else?
  • Does implementing function() vs calcError() make a difference? What is the right way of creating something that constrains a search space to be within a polyline like this?
  • Should the error be a scalar or a vector?

@zkingston
Copy link
Contributor

  1. I think MoveIt's box constraint bounds here define the region in which EE positions are considered valid (I am not 100% on this). Since your constraint is a sequence of smaller position constraints (maybe you could think of this as a collection of MoveIt's box / position / equality constraints tiled together so that each error region overlaps with each other...), the bounds here you inherit from BaseConstraint probably don't do what you want, you might need to overload the Jacobian.
  2. calcError is a MoveIt thing, function is the OMPL constraint version. I don't know enough about the MoveIt implementation to say.
  3. Vector should work, but if you can get the gradient for scalar that should be fine as well.

@sea-bass
Copy link
Contributor Author

Interesting... so is there a way in OMPL to do a "ConstraintUnion" rather than ConstraintIntersection to tile these together?

@zkingston
Copy link
Contributor

I think an idea similar to what you have currently, but using the NN to store the centers of each constraint region (which tile the path similar to what you have currently), and then evaluating the error / jacobian to the closest constraint may work.

There isn't a way in OMPL to do a "constraint union" like this - I'm not sure if it even is a well-formed concept.

@sea-bass
Copy link
Contributor Author

Thank you for all this input -- these are helpful things to try out!

@sea-bass
Copy link
Contributor Author

sea-bass commented Feb 17, 2023

I tried going down this path of individual box constraints, and have something that runs with more reusable code... but I keep getting this at the end.

[ERROR] [1676674916.342760349] [ompl]: ./src/ompl/base/src/SpaceInformation.cpp:468 - State samplers generate constraint unsatisfying states.

Interestingly enough, if my path has two points, so it's a single box constraint, everything works perfectly and I can do constrained planning in a straight line. But as soon as I add more points, even if it's just splitting a straight line into two, it fails.

Will dig more when I find more time / energy.

Comment on lines +444 to +465
void ToolPathConstraint::function(const Eigen::Ref<const Eigen::VectorXd>& joint_values,
Eigen::Ref<Eigen::VectorXd> out) const
{
const Eigen::Isometry3d eef_pose = forwardKinematics(joint_values);

const auto eef_nearest = pose_nn_.nearest(EigenIsometry3dWrapper(eef_pose));
// std::cout << "Function, nearest: " << eef_nearest.path_idx_ << std::endl;

const auto bc = box_constraints_.at(eef_nearest.path_idx_);
bc->function(joint_values, out);
}

void ToolPathConstraint::jacobian(const Eigen::Ref<const Eigen::VectorXd>& joint_values,
Eigen::Ref<Eigen::MatrixXd> out) const
{
const Eigen::Isometry3d eef_pose = forwardKinematics(joint_values);
const auto eef_nearest = pose_nn_.nearest(EigenIsometry3dWrapper(eef_pose));
// std::cout << "Jacobian, nearest: " << eef_nearest.path_idx_ << std::endl;

const auto bc = box_constraints_.at(eef_nearest.path_idx_);
bc->jacobian(joint_values, out);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are the "magic" functions that map the EEF pose to the nearest box constraint for that path segment and compute the standard function/Jacobian for that box constraints.

I think this is close, except that I think the state sampler needs to be configured.

@github-actions
Copy link

github-actions bot commented Apr 4, 2023

This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete.

@github-actions github-actions bot added the stale Inactive issues and PRs are marked as stale and may be closed automatically. label Apr 4, 2023
@tylerjw
Copy link
Member

tylerjw commented Aug 23, 2023

Closing as this seems to be abandoned. Feel free to re-open if you take it back up.

@tylerjw tylerjw closed this Aug 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Inactive issues and PRs are marked as stale and may be closed automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants