search

moveit / moveit_ros

THIS REPO HAS MOVED TO https://github.com/ros-planning/moveit
69 stars 118 forks source link

rviz plugin: stop execution + update of start state to current #713

Closed rhaschke closed 7 years ago

rhaschke commented 7 years ago

This PR combines ideas of #709 and #710 (and would replaces those PRs) to allow

I consider this work-in-progress. @davetcoleman Should we address this comment like suggested?

davetcoleman commented 7 years ago

I think its a good idea to address https://github.com/ros-planning/moveit_ros/pull/709#discussion_r70888457 but I understand if you don't have the cycles for it right now

rhaschke commented 7 years ago

I addressed all comments. Remaining issue is to turn ExecuteService in ExecuteAction and of course deal with #716.

rhaschke commented 7 years ago

Further cleaned up and rebased. Disabling QueryStartState by default as suggested in #710. Relies on #717. For correct working of start-state updates, we need to fix #716 as well. Could be merged now.

davetcoleman commented 7 years ago

+1

davetcoleman commented 7 years ago

since this is a relatively major change (new GUI button) to a stable (indigo) release I think you should announce this on the mailing list

also, please be sure to cherry-pick to jade and kinetic

thanks!

wkentaro commented 7 years ago

@davetcoleman @rhaschke Thanks for working for this!

davetcoleman commented 7 years ago

the thanks goes to @rhaschke !

rhaschke commented 7 years ago

please be sure to cherry-pick to jade and kinetic

Dave, you already merge-committed #718 into jade-devel. I would have loved to use a squashing commit only, i.e. forwarding jade-devel to the cherry-pick branch. Anybody minds to rebase jade-devel there?

The same I would do for kinetic. For me this eases reading the git graph: Merge commits are unique contributions, while cherry-picks could be committed directly. I just had to use a PR to let Travis do the compile checking.

rhaschke commented 7 years ago

since this is a relatively major change (new GUI button) to a stable (indigo) release I think you should announce this on the mailing list

Dear Dave, before announcing on the mailing list, I would like to finish #716, which is a major pre-requisite for #713 to work correctly. Hopefully, I find some time this evening to continue on that. In the same vein, I'm planning to merge those changes to Kinetic only as a bundle.

davetcoleman commented 7 years ago

I just had to use a PR to let Travis do the compile checking.

Please make note of that in your PR if you don't want it actually merged yet.

Dave, you already merge-committed #718 into jade-devel.

Yes, I hadn't seen the PR until after I made that comment

You're announcement timeline sounds good, thanks!