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

select planning group in planning tab and simplify selecting goal and start states #1198

Merged
merged 3 commits into from Nov 27, 2018

Conversation

Projects
None yet
4 participants
@2scholz
Copy link
Contributor

2scholz commented Nov 13, 2018

Fixes #1069.

The request consists of multiple commits.

The first one fixes rviz crashing when the planning group is changed while executing a trajectory.
The problem was the destruction of a MoveGroupInterface that also holds the SimpleActionClient used to execute the trajectory. The proposed solution here creates a local shared pointer of the object for execution to ensure that the object is not destroyed asynchronously.

The other two commits change the gui in the planning tab.
This is the old state of the planning tab:
old_gui

The first addition is a dropdown menu for choosing a planning group:
partial_change
Previously it was only possible to choose the planning group in the MotionPlanning display. The dropdown menu is synchronized with the display property.

The final commit tries to streamline the interface and should improve usability further. It gets rid of the update buttons and makes both dropdown menus for the start and goal state constantly visible:
full_change

The choice is instantly confirmed when it is clicked in the dropdown menu. As a consequence repeatedly updating random states is now a bit more complicated, but still possible by simply clicking on that option repeatedly.

@v4hn

@v4hn

v4hn approved these changes Nov 13, 2018

Copy link
Member

v4hn left a comment

Yes! That's so much more usable! Thank you a lot for improving this @2scholz !

@rhaschke
Copy link
Contributor

rhaschke left a comment

Generally I approve this. Great job!
There is only a small nitpick to further improve the code.

void MotionPlanningFrame::fillStateSelectionOptions()
{
ui_->start_state_selection->clear();
ui_->goal_state_selection->clear();
ui_->start_state_combo_box->blockSignals(true);

This comment has been minimized.

@rhaschke

rhaschke Nov 13, 2018

Contributor

There is a nice utility class taking care of unblocking signals automatically: QSignalBlocker.

ui_->start_state_combo_box->blockSignals(false);
ui_->goal_state_combo_box->blockSignals(false);
ui_->start_state_combo_box->setCurrentIndex(2); // default to 'current'
ui_->goal_state_combo_box->setCurrentIndex(2); // default to 'current'

This comment has been minimized.

@rhaschke

rhaschke Nov 13, 2018

Contributor

Did you changed the default intentionally here?

This comment has been minimized.

@2scholz

2scholz Nov 16, 2018

Author Contributor

This was indeed an intentional change.
The goal state is always the current state after the planning group was changed and I think the combo box should reflect this.

ui_->goal_state_combo_box->blockSignals(false);
ui_->start_state_combo_box->setCurrentIndex(2); // default to 'current'
ui_->goal_state_combo_box->setCurrentIndex(2); // default to 'current'
ui_->planning_group_combo_box->setCurrentText(QString::fromStdString(group)); // currently chosen group

This comment has been minimized.

@rhaschke

rhaschke Nov 13, 2018

Contributor

Why do you need to touch the planning_group_combo_box?
Semantically this doesn't fit into fillStateSelectionOptions().

This comment has been minimized.

@2scholz

2scholz Nov 16, 2018

Author Contributor

This is needed to update the combo box, whenever the planning group is changed via the display property.

I deleted this line and replaced the functionality by calling fillPlanningGroupOptions() in changePlanningGroupHelper() instead.
Let me know if you would prefer a seperate function that only chooses the correct planning group instead of retrieving the planning groups and refilling the combo box at each change.

This comment has been minimized.

@rhaschke

rhaschke Nov 22, 2018

Contributor

No. The planning_group_combo_box only needs to be updated once when the RobotModel was loaded. I implemented and pushed the corresponding fix to your PR branch. @2scholz, please have a final look and rebase onto current melodic-devel.

This comment has been minimized.

@2scholz

2scholz Nov 26, 2018

Author Contributor

It makes sense to only fill the options of the planning_group_combo_box once when the RobotModel is loaded.
But this breaks the synchronization between the property in the MotionPlanningDisplay and the drop-down menu in the panel. Whenever the planning group is changed via the property, the drop-down menu does not show the currently selected group anymore.

To fix this I added the method setPlanningGroupText to update the index of the planning_group_combo_box. It is used in the changePlanningGroupHelper.

ui_->start_state_selection->setCurrentIndex(2); // default to 'current'
ui_->goal_state_selection->setCurrentIndex(0); // default to 'random valid'

ui_->start_state_combo_box->blockSignals(false);

This comment has been minimized.

@rhaschke

rhaschke Nov 13, 2018

Contributor

These are obsolete when using QSignalBlocker :-)

@rhaschke
Copy link
Contributor

rhaschke left a comment

Please fix the following uic errors:

/root/ws_moveit/src/moveit/moveit_ros/visualization/motion_planning_rviz_plugin/src/ui/motion_planning_rviz_plugin_frame.ui: Warning: Tab-stop assignment: 'start_state_selection' is not a valid widget.
/root/ws_moveit/src/moveit/moveit_ros/visualization/motion_planning_rviz_plugin/src/ui/motion_planning_rviz_plugin_frame.ui: Warning: Tab-stop assignment: 'use_start_state_button' is not a valid widget.
/root/ws_moveit/src/moveit/moveit_ros/visualization/motion_planning_rviz_plugin/src/ui/motion_planning_rviz_plugin_frame.ui: Warning: Tab-stop assignment: 'goal_state_selection' is not a valid widget.
/root/ws_moveit/src/moveit/moveit_ros/visualization/motion_planning_rviz_plugin/src/ui/motion_planning_rviz_plugin_frame.ui: Warning: Tab-stop assignment: 'use_goal_state_button' is not a valid widget.
@rhaschke

This comment has been minimized.

Copy link
Contributor

rhaschke commented Nov 14, 2018

As a consequence repeatedly updating random states is now a bit more complicated, but still possible by simply clicking on that option repeatedly [twice].

This is indeed more laborious now. However, I didn't used random postures much in the past...

@2scholz 2scholz force-pushed the 2scholz:rviz_ui_simplification branch 2 times, most recently from ebc920b to 188f82c Nov 16, 2018

@rhaschke

This comment has been minimized.

Copy link
Contributor

rhaschke commented Nov 26, 2018

@2scholz Please have a look at #1198 (comment)

@rhaschke rhaschke referenced this pull request Nov 26, 2018

Closed

201811 release #1225

11 of 12 tasks complete
fix rviz crash when changing the planning group while executing
changing the planning group destroyed the MoveGroupInterface which
also destroyed the SimpleActionClient used to execute the trajectory.
A shared pointer is now used to ensure that the object exists until the
end of the execution of the trajectory.

@2scholz 2scholz force-pushed the 2scholz:rviz_ui_simplification branch 2 times, most recently from 63a608b to 76d4c33 Nov 26, 2018

@@ -260,6 +260,12 @@ void MotionPlanningFrame::fillPlanningGroupOptions()
ui_->planning_group_combo_box->addItem(QString::fromStdString(group_name));
}

void MotionPlanningFrame::setPlanningGroupText()
{
const QSignalBlocker planning_group_blocker(ui_->planning_group_combo_box);

This comment has been minimized.

@rhaschke

rhaschke Nov 26, 2018

Contributor

I think, you don't need to block signals here. setCurrentText only sends a new signal, when the text actually changed.

@rhaschke

This comment has been minimized.

Copy link
Contributor

rhaschke commented Nov 27, 2018

@2scholz Can you please also cleanup the commit history? I would like to keep your three initial commits but suggest to squash the fixup commits into the last one.

2scholz added some commits Nov 9, 2018

add planning group combo box to planning tab
simplifies choosing a planning group.
The group is chosen via dropdown menu.

The combo box is synchronized with the one in the MotionPlanning display
under Planning Request, so that both boxes will always show the same
value.
simplify choosing start and goal state
The update buttons were removed and the goal and start state are now
updated directly after an option was clicked in the combo box.
Random states can be updated by clicking on that choice repeatedly.

The combo boxes were previously in a QToolBox where only one of the
selections was visible at a time.
Because the removal of the update buttons saves a lot of space the
QToolBox was removed as well, so that both combo boxes are visible at
all times.

@2scholz 2scholz force-pushed the 2scholz:rviz_ui_simplification branch from 76d4c33 to cafe7f2 Nov 27, 2018

@2scholz

This comment has been minimized.

Copy link
Contributor Author

2scholz commented Nov 27, 2018

I removed the signal blocker in setPlanningGroupText and cleaned up the commit history.

@rhaschke rhaschke merged commit 15e52cd into ros-planning:melodic-devel Nov 27, 2018

1 check passed

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

This comment has been minimized.

Copy link
Member

davetcoleman commented Dec 3, 2018

@2scholz thanks for this contribution!

Can you please update the melodic tutorials to reflect the new location of the planning group option? See

In Planning Request, change the Planning Group to panda_arm.

https://ros-planning.github.io/moveit_tutorials/doc/quickstart_in_rviz/quickstart_in_rviz_tutorial.html

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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.