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

rhaschke
Copy link
Collaborator

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
@rhaschke rhaschke deleted the rviz-cartesian branch December 6, 2018 13:54
@felixvd
Copy link
Member

felixvd 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
Copy link
Collaborator Author

@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");
Copy link
Member

Choose a reason for hiding this comment

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

SUCCEEDED

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

felixvd 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants