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

motion_planning_rviz_plugin : add CartesianPath planning check box #1238

Merged
merged 8 commits into from Dec 6, 2018

Conversation

Projects
None yet
3 participants
@rhaschke
Copy link
Collaborator

commented Nov 29, 2018

Rebase of #931 for Melodic, with additional cleanup and fixes:

  • remove cartesianPathToggled()
  • reduce code duplication in computeCartesianPlan() and computeJointSpacePlan()
  • fix seg-fault occuring when there is no unique end-effector link
  • save/load status of new checkbox
  • report planning time as requested by @felixvd

@k-okada, please review.

@rhaschke rhaschke merged commit eeab3ae into ros-planning:melodic-devel Dec 6, 2018

1 of 2 checks passed

continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rhaschke rhaschke deleted the ubi-agni:rviz-cartesian branch Dec 6, 2018

@felixvd

This comment has been minimized.

Copy link
Contributor

commented Dec 10, 2018

The previous PR worked on kinetic, no? Should this be cherry-picked? You mentioned that #932 waits for this, but that PR is on kinetic.

@rhaschke

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 10, 2018

@felixvd This PR contains several additional fixes. Looks like you vote for back-porting this?
Personally, I'm reluctant to actively maintain two branches, particularly because melodic-devel also builds on Kinetic.

bool success =
iptp.computeTimeStamps(rt, ui_->velocity_scaling_factor->value(), ui_->acceleration_scaling_factor->value());
ROS_INFO("Computed time stamp %s", success ? "SUCCEDED" : "FAILED");
ROS_INFO("Computing time stamps %s", success ? "SUCCEDED" : "FAILED");

This comment has been minimized.

Copy link
@felixvd

felixvd Dec 10, 2018

Contributor

SUCCEEDED

This comment has been minimized.

Copy link
@rhaschke

rhaschke Dec 10, 2018

Author Collaborator

As this is already merged, could you please file a PR?

double fraction =
move_group_->computeCartesianPath(waypoints, cart_step_size, cart_jump_thresh, trajectory, avoid_collisions);

if (fraction >= 1.0)

This comment has been minimized.

Copy link
@felixvd

felixvd Dec 10, 2018

Contributor

This used to be > 0. With this version, the button fails silently. Is that on purpose?

This comment has been minimized.

Copy link
@rhaschke

rhaschke Dec 10, 2018

Author Collaborator

Yes, this was on purpose: If you cannot reach your target, the planning request should fail.
What do you mean by "fails silently"? It should be written "FAILED" and the partial trajectory should be shown.

This comment has been minimized.

Copy link
@felixvd

felixvd Dec 10, 2018

Contributor

My bad, it will display "Failed" in the UI. I don't see how the partial trajectory would be displayed, but I'll go test first.

@felixvd

This comment has been minimized.

Copy link
Contributor

commented Dec 10, 2018

I don't know what the general guideline is, but I would vote for backporting this particular PR because it affects the out-of-the-box experience for new users and demos. Especially if it compiles without changes on Kinetic (which it looks like).

Maybe @k-okada and @7675t have an opinion about this and #932.

edit: I'm assuming that the kinetic binaries are built from melodic-devel. If not you can ignore what I said.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.