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

mgi: add execution methods for moveit_msgs::RobotTrajectory #1955

Merged
merged 1 commit into from
Mar 16, 2020

Conversation

v4hn
Copy link
Contributor

@v4hn v4hn commented Mar 16, 2020

I just reviewed moveit/moveit_tutorials#468 and
thought this would be a nicer way to execute Cartesian trajectories...

@v4hn v4hn requested a review from rhaschke as a code owner March 16, 2020 10:21
Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Instead of introducing two new function variants, what about introducing a single (implicit) constructor of Plan from RobotTrajectory? This way, it should be possible to pass a RobotTrajectory to the existing functions.

@v4hn
Copy link
Contributor Author

v4hn commented Mar 16, 2020

Actually, if you want to unify them, I would propose to simply deprecate the old execute method.
The method actually only ever looks at the trajectory_ field...
I'll implement the changes and provide a deprecated wrapper for the old method.

This eliminates the common question how to execute a planned Cartesian trajectory
and provides a useful API either way.
@v4hn v4hn force-pushed the pr-master-mgi-execute-trajectory-msg branch from 031be8d to 12a77eb Compare March 16, 2020 13:03
@v4hn
Copy link
Contributor Author

v4hn commented Mar 16, 2020

After thinking about it a bit more, I decided to leave the old interfaces around without deprecation.
They are convenient wrappers when working with the plan method and in use all over the place.
It feels stupid to deprecate a one-line convenient wrapper in a class meant to make access convenient.

@codecov-io
Copy link

Codecov Report

Merging #1955 into master will increase coverage by 0.04%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1955      +/-   ##
==========================================
+ Coverage   49.83%   49.87%   +0.04%     
==========================================
  Files         314      314              
  Lines       24720    24726       +6     
==========================================
+ Hits        12319    12332      +13     
+ Misses      12401    12394       -7
Impacted Files Coverage Δ
...moveit/move_group_interface/move_group_interface.h 36.84% <ø> (ø) ⬆️
.../move_group_interface/src/move_group_interface.cpp 22.38% <0%> (-0.15%) ⬇️
...veit_experimental/moveit_jog_arm/src/jog_calcs.cpp 73.98% <0%> (+4.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b171a01...12a77eb. Read the comment docs.

@v4hn v4hn merged commit a26ffa9 into moveit:master Mar 16, 2020
@felixvd
Copy link
Contributor

felixvd commented Mar 17, 2020

It makes a lot of sense for the move group to execute a trajectory instead of a plan. I like this.

v4hn added a commit to v4hn/moveit that referenced this pull request Mar 30, 2020
)

This eliminates the common question how to execute a planned Cartesian trajectory
and provides a useful API either way.
v4hn added a commit to v4hn/moveit that referenced this pull request Mar 31, 2020
)

This eliminates the common question how to execute a planned Cartesian trajectory
and provides a useful API either way.
v4hn added a commit to v4hn/moveit that referenced this pull request Mar 31, 2020
)

This eliminates the common question how to execute a planned Cartesian trajectory
and provides a useful API either way.
sjahr pushed a commit to sjahr/moveit that referenced this pull request Jun 21, 2024
* Generate Doxygen Tag

* Install tagfile in output directory

* Fix problematic override for Doxygen linking
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

4 participants