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

Added combo box to setup assistant for default planner #658

Merged

Conversation

Projects
None yet
5 participants
@akgoins
Copy link
Contributor

commented Oct 18, 2017

Adds a combo box to the MoveIt! Setup Assistant to choose a default planner. PR fixes issue #637.
moveit_default_planner1

@rhaschke
Copy link
Collaborator

left a comment

@akgoins Thank you very much for your contribution.
Overall this looks very good, but I have a few comments:
The default planner is not related to kinematics, please adapt variables names, comments, and code placement to consider that fact.
How do you handle manual changes to the ompl_planning.yaml? Are they overwritten (silently)?

@@ -71,6 +71,7 @@ static const int DEFAULT_KIN_SOLVER_ATTEMPTS_ = 3;
struct GroupMetaData
{
std::string kinematics_solver_; // Name of kinematics plugin to use
std::string kinematics_default_planner_; // Name of the default planner to use

This comment has been minimized.

Copy link
@rhaschke

rhaschke Oct 18, 2017

Collaborator

The planner is not related to kinematics. Please rename variable to default_planner_ and move it to a separate section.

@@ -302,6 +302,76 @@ bool MoveItConfigData::outputOMPLPlanningYAML(const std::string& file_path)

emitter << YAML::Value << YAML::BeginMap;

std::vector<OMPLPlannerDescription> planner_des = getOMPLPlanners();

This comment has been minimized.

Copy link
@rhaschke

rhaschke Oct 18, 2017

Collaborator

Why did you moved this code block around? This makes it harder to see the actual change.

This comment has been minimized.

Copy link
@akgoins

akgoins Nov 8, 2017

Author Contributor

I did not move this block, but rather I took the list of OMPL planners out and made a separate function to generate the list. I guess Git thought this was the right diff to show rather than showing an even larger block of OMPL planners being copied and pasted elsewhere.

I created the getOMPLPlanners() function so that I could get the list of planners to populate the default planner combo box. I looked everywhere, but this was the only location that contained the OMPL planner list, and the function outputOMPLPlanningYAML() did not return the list, but wrote it to a YAML file.

@@ -74,6 +74,12 @@ GroupEditWidget::GroupEditWidget(QWidget* parent, moveit_setup_assistant::MoveIt
kinematics_solver_field_->setMaximumWidth(400);
form_layout->addRow("Kinematic Solver:", kinematics_solver_field_);

// Kinematic default planner

This comment has been minimized.

Copy link
@rhaschke

rhaschke Oct 18, 2017

Collaborator

As said above, the default planner is not related to kinematics. Please cleanup variables and comments and move the combobox to the end of the kinematics block.

if (index == -1)
{
QMessageBox::warning(this, "Missing Default Planner",
QString("Unable to find the default planner '").append(default_planner.c_str()).append("'. "));

This comment has been minimized.

Copy link
@rhaschke

rhaschke Oct 18, 2017

Collaborator

Please use default Qt-Syntax: QString("Unable to find the default planner '%1'").arg(default_planner)

std::vector<OMPLPlannerDescription> planners = config_data_->getOMPLPlanners();
for (int i = 0; i < planners.size(); ++i)
{
std::string planner_name = planners[i].name_ + "kConfigDefault";

This comment has been minimized.

Copy link
@rhaschke

rhaschke Oct 18, 2017

Collaborator

The literal "kConfigDefault" should be better added in getOMPLPlanners(). Now, this literal is introduced twice with the risk of later changing only one of them...

@@ -74,6 +74,7 @@ class GroupEditWidget : public QWidget
QLabel* title_; // specify the title from the parent widget
QLineEdit* group_name_field_;
QComboBox* kinematics_solver_field_;
QComboBox* kinematics_default_planner_field_;

This comment has been minimized.

Copy link
@rhaschke

rhaschke Oct 18, 2017

Collaborator

Variable name.

@@ -1089,6 +1089,8 @@ bool PlanningGroupsWidget::saveGroupScreen()
// Get a reference to the supplied strings
const std::string& group_name = group_edit_widget_->group_name_field_->text().toStdString();
const std::string& kinematics_solver = group_edit_widget_->kinematics_solver_field_->currentText().toStdString();
const std::string& kinematics_default_planner =
group_edit_widget_->kinematics_default_planner_field_->currentText().toStdString();

This comment has been minimized.

Copy link
@rhaschke

rhaschke Oct 18, 2017

Collaborator

Variable name.

fs::path ompl_yaml_path = config_data_->config_pkg_path_;
ompl_yaml_path /= "config/ompl_planning.yaml";

if (!config_data_->inputOMPLYAML(ompl_yaml_path.make_preferred().native().c_str()))

This comment has been minimized.

Copy link
@rhaschke

rhaschke Oct 18, 2017

Collaborator

If the file isn't there, I think that's not an issue. I vote for removing this verbose messagebox.
The more crucial point is to consider manual changes made to ompl_planning.yaml since the last use of setup assistant and to not overwrite those changes accidentally. We have a warning mechanism for this here.

@davetcoleman

This comment has been minimized.

Copy link
Member

commented Oct 19, 2017

This is very useful! However graphically I'd prefer (as the original author of the Setup Assistant) if you now used two QT GroupBox widgets to separate the kinematics configurations from the planner selection.

@v4hn

This comment has been minimized.

Copy link
Member

commented Nov 23, 2017

This patch will need a bit more work before we can merge it.

  • ompl_planning.yaml is overwritten without highlighting it as changed. This is ok if all unchanged items in the file are retained as-is, but at least longest_valid_segment_fraction, likely also other things are not retained.
  • The configuration options for the planning group should be separated via GroupBox into "Kinematics" and "OMPL Planning"
  • currently the plugin writes entries like type: geometric::SBLkConfigDefault for each planner configuration. These are wrong and should be type: geometric::SBL

Possibly other things...

@davetcoleman

This comment has been minimized.

Copy link
Member

commented Mar 29, 2018

@akgoins ping - I think this PR is a great idea and I'd like to see it polished off and merged. Are the remaining requests clear to you?

@rhaschke

This comment has been minimized.

Copy link
Collaborator

commented Apr 5, 2018

@akgoins Instead of merging, you should rebase your PR branch onto the current remote master.

@akgoins akgoins force-pushed the akgoins:setup_assistant_default_planner branch from 53dc348 to 5bfde31 Apr 5, 2018

@akgoins

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2018

@davetcoleman, can someone review this? I think it is ready to be merged, but I don't know if I fully understand the first issue that @v4hn mentioned. When I ran it, it gave me a warning when the ompl_planning.yaml file was changed externally and that it was going to be overwritten.

@rhaschke
Copy link
Collaborator

left a comment

I approve your changes to the GUI, but the handling of an existing ompl_planning.yaml is not sufficient yet. Currently you are only reading the default planners from this file, but then write the file from scratch in the end, thus overwriting all custom settings made by users.

I think you should:

  1. Load and keep the full YAML document.
  2. Extract the default planners (as you did).
  3. On creation of new groups, add corresponding default configs to the YAML document structure.
  4. On deletion of groups, remove the corresponding config settings from the YAML document.
  5. When displaying the ComboBox for default planner selection, fill it with the set of available planners (for this group) in the YAML document.
  6. On final file writing: Save the (adapted) YAML document structure.
{
std::string planner;
parse(group_it->second, "default_planner_config", planner);
int pos = planner.find_last_not_of("kConfigDefault");

This comment has been minimized.

Copy link
@rhaschke

rhaschke May 10, 2018

Collaborator

I suggest not to cut this kConfigDefault suffix. Usually it's this name, but people may change it.

@@ -74,6 +74,7 @@ struct GroupMetaData
double kinematics_solver_search_resolution_; // resolution to use with solver
double kinematics_solver_timeout_; // solver timeout
int kinematics_solver_attempts_; // solver attempts
std::string default_planner_; // Name of the default planner to use

This comment has been minimized.

Copy link
@rhaschke

rhaschke May 10, 2018

Collaborator

I think, instead of saving a copy of the default planner here, you should store a reference into the YAML document structure, which then allows to access all details.

@davetcoleman

This comment has been minimized.

Copy link
Member

commented May 10, 2018

Currently you are only reading the default planners from this file, but then write the file from scratch in the end, thus overwriting all custom settings made by users.

@rhaschke Overwriting user-custom values is actually how almost all of the setup assistant currently behaves so, while your suggestion would be nice, I don't think its a hard requirement to have this PR merged in

@rhaschke

This comment has been minimized.

Copy link
Collaborator

commented May 10, 2018

I don't agree, that we should accept overwriting the ompl settings. There are a lot of settings in these files, which can be easily retained following the proposed approach. If you don't know, how to realize the proposed changes, @akgoins, please tell.

@rhaschke rhaschke self-assigned this May 10, 2018

@rhaschke rhaschke merged commit 6ab2e36 into ros-planning:kinetic-devel May 26, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

rhaschke added a commit to ubi-agni/moveit that referenced this pull request May 26, 2018

@gavanderhoorn

This comment has been minimized.

Copy link
Member

commented Jun 19, 2018

Could this PR be responsible for the (potential) regression reported in moveit: param server is missing planner configurations on ROS Answers?

I expect the author there to open an issue about the problem he ran into btw.

dg-shadow added a commit to shadow-robot/moveit that referenced this pull request Jul 30, 2018

@mlautman mlautman referenced this pull request Aug 30, 2018

Merged

removing regression introduced in #658 #1040

2 of 6 tasks complete

mlautman added a commit that referenced this pull request Aug 30, 2018

mlautman added a commit that referenced this pull request Sep 6, 2018

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.