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 seed trajectories to MotionPlanRequest #46

Merged
merged 1 commit into from
Oct 9, 2019

Conversation

bmagyar
Copy link
Member

@bmagyar bmagyar commented Jul 7, 2018

Related to moveit/moveit#804

Everything is up for debate.

My points so far:

  • I used the JointOrCartesianTrajectory to avoid confusion regarding the contents of the message. Ideally trajectory_msgs could host this message.
  • I chose geometry_msgs/TransformStamped to describe the points of a Cartesian trajectory to allow for any base frame to be used to define any point. This allows for multiple seeds even for different end effector(s) although leaves some questions regarding the properness of this arbitrary point.
  • Something like this could be added to a conversion library to ease moving from-to the different spaces using the IK solver present.
  • I added the seed_trajectories field to MotionPlanRequest.

Once we get to a good common ground I could try setting up a demo.
FYI: @rhaschke @davetcoleman @jrgnicho

msg/JointOrCartesianTrajectory.msg Outdated Show resolved Hide resolved
msg/MotionPlanRequest.msg Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@davetcoleman
Copy link
Member

Thanks for starting this conversation!!

@bmagyar
Copy link
Member Author

bmagyar commented Jul 13, 2018

Thanks @davetcoleman for your feedback, you've brought up great point.

I'll keep adding commits to this PR to maintain a visible progression for anyone who wants to join in.

Should we also add tolerances for JointTrajectory or only the Cartesian part?

Copy link
Member Author

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

Technically this is incorrect & inconsistent but didn't want to define Jerk.

@bmagyar
Copy link
Member Author

bmagyar commented Jul 13, 2018

This version follows the pattern of JointTrajectory and JointTrajectoryPoint but I also included tolerances in the point.

Copy link
Member

@davetcoleman davetcoleman left a comment

Choose a reason for hiding this comment

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

Looks much better!

msg/CartesianTrajectory.msg Outdated Show resolved Hide resolved
msg/CartesianTrajectoryPoint.msg Outdated Show resolved Hide resolved
msg/CartesianTrajectoryPoint.msg Outdated Show resolved Hide resolved
@bmagyar
Copy link
Member Author

bmagyar commented Jul 23, 2018

The thing I don't like about using Pose types for pose_tolerance is that it makes it hard to reason about a quaternion... A Twist would be more intuitive but then we are mixing apples and oranges...
What is your take on this?

@bmagyar
Copy link
Member Author

bmagyar commented Aug 5, 2018

Ping

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.

Sorry for the timeout and thanks for the mail ping on this @bmagyar .
I added my feedback.

Before we merge this, I believe there should also be a pull-request that actually makes use of it.
Otherwise this will just end up being dead code, just like the trajectory_constraints field.

msg/GenericTrajectory.msg Outdated Show resolved Hide resolved
msg/CartesianTrajectory.msg Show resolved Hide resolved
msg/CartesianTrajectory.msg Show resolved Hide resolved
msg/CartesianTrajectoryPoint.msg Outdated Show resolved Hide resolved
msg/CartesianTrajectoryPoint.msg Outdated Show resolved Hide resolved
@bmagyar
Copy link
Member Author

bmagyar commented Sep 29, 2018

I shall build an example once this PR is ready to be merged to avoid having to update that one with every change here. I think the best candidate to add it to will be CHOMP and a unit test for that.

@davetcoleman
Copy link
Member

I like the idea of having an example implementation!

@davetcoleman
Copy link
Member

@jschleicher as we discussed in person, I think you'd have valuable input into the needs for this message and cartesian planning with moveit

msg/CartesianTrajectory.msg Outdated Show resolved Hide resolved
msg/CartesianTrajectoryPoint.msg Outdated Show resolved Hide resolved
msg/CartesianTrajectoryPoint.msg Outdated Show resolved Hide resolved
msg/GenericTrajectory.msg Show resolved Hide resolved
msg/CartesianTrajectory.msg Show resolved Hide resolved
msg/MotionPlanRequest.msg Show resolved Hide resolved
@jschleicher
Copy link

Probably this heads into another direction than the original scope of this PR: I'd like to use MotionPlanRequestMessage for cartesian paths as well and encode the type of planning in the plannerId (and maybe PlanningOptions). That would supersede computeCartesianPath and put the functionality into a PlannerManager (what we did in pilz_industrial_motion). For Sequences of (Cartesian or joint-space or mixed) waypoints we use an array of MotionPlanRequests, see pilz_industrial_motion package.

For circular arcs we need an additional helper point (circle center), that we "hacky" encode in path_constraints, maybe that field could be generalized as well (see python pilz_robot_programming )

To sum it up we want to

  • plan to a (joint or cartesian) goal and have a cartesian linear path (nothing new)
  • plan through multiple such points with different planner (sub-)types ("Sequence")
  • blend in between the subparts of a sequence
  • add constraints that the trajectory doesn't need to plan through (but keep a specified distance in the case of a circle)

@bmagyar
Copy link
Member Author

bmagyar commented Dec 17, 2018

Hi @jschleicher , I agree with you. The scope of this PR is to pass in an initial/seed trajectory to start an optimization-based planner from. The things you suggest however are somewhat related and will be partly enabled by this PR however I think we should follow it up with an extension to PlanningOptions which could determine the interpretation of the cartesian trajectory that was passed in such that:

  • plan through points (new flag)
  • determine how to blend between these point-to-point plans (new flag)
  • add no-go areas (could be a new set of constraints that are enabled when following points, or when a new flag is enabled)

@davetcoleman
Copy link
Member

@bmagyar we (@ommmid ) are working on TrajOpt and need a way to pass in a seed joint-space trajectory. This PR is perfect for that, do you think you could address the remaining feedback so we can merge this? A lot of it is simply documentation requests

@davetcoleman davetcoleman changed the base branch from melodic-devel to master August 7, 2019 20:44
@davetcoleman
Copy link
Member

I've just targeted this to the master branch.

@bmagyar
Copy link
Member Author

bmagyar commented Aug 8, 2019

Thanks @davetcoleman , I'll move forward with addressing the comments but unfortunately due to impeding deadlines regarding my thesis I won't be able to supply an example. It sound like the TrajOpt effort would be perfectly fit for this purpose, right?

@ommmid
Copy link

ommmid commented Aug 8, 2019

@bmagyar yes, that is exactly what we need in trajopt.

@davetcoleman
Copy link
Member

friendly ping?

msg/GenericTrajectory.msg Outdated Show resolved Hide resolved
msg/CartesianTrajectory.msg Outdated Show resolved Hide resolved
msg/CartesianTrajectoryPoint.msg Outdated Show resolved Hide resolved
@mlautman mlautman mentioned this pull request Sep 19, 2019
@bmagyar
Copy link
Member Author

bmagyar commented Sep 25, 2019

ok, I am finally out of the grasp of PhD work, let's get this baby delivered!
I'll resolve what was done, whoever wishes to review, feel free to re-open if I closed something by mistake.

@mlautman @davetcoleman can we aim to merge this by Friday?

Copy link
Contributor

@BryceStevenWilley BryceStevenWilley left a comment

Choose a reason for hiding this comment

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

LGTM, seems like the only part left to resolve is how to state which trajectory should be used in a Generic trajectory (if you need to make a statement like that at all).

CMakeLists.txt Show resolved Hide resolved
msg/GenericTrajectory.msg Show resolved Hide resolved
msg/GenericTrajectory.msg Outdated Show resolved Hide resolved
Copy link

@mlautman mlautman left a comment

Choose a reason for hiding this comment

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

+1 after adding the CartesianPoint.msg to CMakeLists.txt

CMakeLists.txt Show resolved Hide resolved
@bmagyar bmagyar self-assigned this Oct 3, 2019
@bmagyar
Copy link
Member Author

bmagyar commented Oct 3, 2019

All requested changes implemented, commits squashed 👍

@mlautman mlautman merged commit bd04cde into moveit:master Oct 9, 2019
@tylerjw tylerjw mentioned this pull request May 8, 2020
18 tasks
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.

None yet

7 participants