Add primitive shapes widgets #2137

jrgnicho · 2020-06-02T21:19:51Z

Description

Addresses issue #2063

Adds widgets into the moveit motion planning plugin that allows adding primitive shapes into the planning scene

Checklist

[x ] Required by CI: Code is auto formatted using clang-format
Extend the tutorials / documentation reference
Document API changes relevant to the user in the MIGRATION.md notes
Create tests, which fail without this PR reference
[x ] Include a screenshot if changing a GUI
While waiting for someone to review your request, please help review another open pull request to support the maintainers

JafarAbdi · 2020-06-02T21:57:12Z

moveit_ros/visualization/motion_planning_rviz_plugin/src/motion_planning_frame.cpp

+
+  // get selected shape
+  std::string selected_shape = ui_->shapes_combo_box->currentText().toStdString();
+  if (SHAPES_MAP.count(selected_shape) == 0)


You could also use auto selected_shape_it = SHAPES_MAP.find(selected_shape) and test if it's equal to SHAPES_MAP.end() and replace SHAPES_MAP.at(selected_shape); with the object it point to

simonschmeisser

Nice gimmick, would be even cooler if you would show/hide further SpinBoxes for additional dimensions but I guess time is over and it's nice as is (with the minor modifications requested)

simonschmeisser · 2020-06-03T06:13:06Z

moveit_ros/visualization/motion_planning_rviz_plugin/src/motion_planning_frame.cpp

@@ -128,6 +128,7 @@ MotionPlanningFrame::MotionPlanningFrame(MotionPlanningDisplay* pdisplay, rviz::
  connect(ui_->planning_scene_tree, SIGNAL(itemChanged(QTreeWidgetItem*, int)), this,
          SLOT(warehouseItemNameChanged(QTreeWidgetItem*, int)));
  connect(ui_->reset_db_button, SIGNAL(clicked()), this, SLOT(resetDbButtonClicked()));
+  connect(ui_->add_shape_button, &QPushButton::clicked, [this]() { addPrimitiveShape(); });


Suggested change

connect(ui_->add_shape_button, &QPushButton::clicked, [this]() { addPrimitiveShape(); });

connect(ui_->add_shape_button, &QPushButton::clicked, this, &MotionPlanningFrame::addPrimitiveShape);

No need for a lambda here. Also please be careful with this short version as this or other pointers bound in the lambda could be destroyed elsewhere and then crash on calling the lambda. There is a preferred variant with four arguments where the third denotes the scope of the connection and if either sender or receiver are destructed:

connect(ui_->add_shape_button, &QPushButton::clicked, this, [this]() { addPrimitiveShape(); });

But as stated above, this shouldn't be necessary here

@simonschmeisser The this pointer points to the panel itself so I don't see how it could ever get destroyed in this context but I'll make the change

You're right, in this context it won't happen. ui_->shape_button is a child of this and therefore gets destroyed before this and cancels the connection ... just wanted to point out the general danger of that variant of connect()

My main remark was about the number of brackets and the unnecessary lambda however :)

simonschmeisser · 2020-06-03T06:18:33Z

moveit_ros/visualization/motion_planning_rviz_plugin/src/motion_planning_frame.cpp

+  if (SHAPES_MAP.count(selected_shape) == 0)
+  {
+    QMessageBox::warning(this, QString("Unsupported shape"),
+                         QString("The '").append(selected_shape.c_str()).append("' is not supported."));


Suggested change

QString("The '").append(selected_shape.c_str()).append("' is not supported."));

QString("The shape '%1' is not supported.").arg(selected_shape.c_str()));

This improves readability and also allows rearranging if we should ever start translating MoveIt into languages other than English.

JafarAbdi · 2020-06-03T09:12:18Z

Thanks @jrgnicho for the PR, the CI is failing because you have unhandled ShapeType values see, one way to handle it would be

  std::string selected_shape_name = ui_->shapes_combo_box->currentText().toStdString();
  auto selected_shape = SHAPES_MAP.find(selected_shape_name);
  shapes::ShapeType shape_type = (selected_shape != SHAPES_MAP.end()) ? selected_shape->second : shapes::UNKNOWN_SHAPE;
  shapes::ShapeConstPtr shape;
  switch (shape_type)
  {
    case shapes::BOX:
      shape = std::make_shared<shapes::Box>(side_length, side_length, side_length);
      break;
    case shapes::SPHERE:
      shape = std::make_shared<shapes::Sphere>(0.5 * side_length);
      break;
    case shapes::CONE:
      shape = std::make_shared<shapes::Cone>(0.5 * side_length, side_length);
      break;
    case shapes::CYLINDER:
      shape = std::make_shared<shapes::Cylinder>(0.5 * side_length, side_length);
      break;
    default:
      QMessageBox::warning(this, QString("Unsupported shape"),
                           QString("The '").append(selected_shape_name.c_str()).append("' is not supported."));
      return;
  }

AndyZe · 2020-06-03T13:18:18Z

I would like this feature a lot!

jrgnicho · 2020-06-03T15:07:46Z

Thanks @jrgnicho for the PR, the CI is failing because you have unhandled ShapeType values see, one way to handle it would be

  std::string selected_shape_name = ui_->shapes_combo_box->currentText().toStdString();
  auto selected_shape = SHAPES_MAP.find(selected_shape_name);
  shapes::ShapeType shape_type = (selected_shape != SHAPES_MAP.end()) ? selected_shape->second : shapes::UNKNOWN_SHAPE;
  shapes::ShapeConstPtr shape;
  switch (shape_type)
  {
    case shapes::BOX:
      shape = std::make_shared<shapes::Box>(side_length, side_length, side_length);
      break;
    case shapes::SPHERE:
      shape = std::make_shared<shapes::Sphere>(0.5 * side_length);
      break;
    case shapes::CONE:
      shape = std::make_shared<shapes::Cone>(0.5 * side_length, side_length);
      break;
    case shapes::CYLINDER:
      shape = std::make_shared<shapes::Cylinder>(0.5 * side_length, side_length);
      break;
    default:
      QMessageBox::warning(this, QString("Unsupported shape"),
                           QString("The '").append(selected_shape_name.c_str()).append("' is not supported."));
      return;
  }

@JafarAbdi thanks for the tip, the logic prior to the switch statement makes it so that an unhandled shape type can never be assigned but I'll make the change to comply with the CI

… as well

jrgnicho · 2020-06-03T16:54:59Z

I've made the suggested changes and added double spin boxes to set the x, y and z dimensions which get toggled based on the shape

codecov · 2020-06-03T17:30:21Z

Codecov Report

Merging #2137 into master will decrease coverage by 0.34%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2137      +/-   ##
==========================================
- Coverage   57.79%   57.45%   -0.35%     
==========================================
  Files         328      328              
  Lines       25700    25735      +35     
==========================================
- Hits        14854    14786      -68     
- Misses      10846    10949     +103

Impacted Files	Coverage Δ
...ls/include/moveit/py_bindings_tools/gil_releaser.h	`0.00% <0.00%> (-100.00%)`	⬇️
.../include/moveit/py_bindings_tools/py_conversions.h	`0.00% <0.00%> (-86.67%)`	⬇️
...s/include/moveit/py_bindings_tools/serialize_msg.h	`0.00% <0.00%> (-80.00%)`	⬇️
...ove_group_interface/src/wrap_python_move_group.cpp	`31.96% <0.00%> (-11.08%)`	⬇️
...ntal/moveit_jog_arm/src/collision_check_thread.cpp	`80.59% <0.00%> (-9.97%)`	⬇️
.../move_group_interface/src/move_group_interface.cpp	`44.44% <0.00%> (-3.64%)`	⬇️
...veit_experimental/moveit_jog_arm/src/jog_calcs.cpp	`70.93% <0.00%> (-2.05%)`	⬇️
...meterization/work_space/pose_model_state_space.cpp	`82.31% <0.00%> (-0.69%)`	⬇️
...rimental/moveit_jog_arm/src/jog_interface_base.cpp	`72.79% <0.00%> (-0.23%)`	⬇️
moveit_core/planning_scene/src/planning_scene.cpp	`59.47% <0.00%> (ø)`
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ae4318...81c81d3. Read the comment docs.

felixvd

I just noticed that I didn't post this review yesterday. Oops.

If you could change the default values of the sliders, this looks ready to merge for me.

If you have the energy to try adding in the mesh option, that would be amazing and really complete this feature, but if not just open an issue as a reminder. Would be a great addition we shouldn't forget about.

felixvd · 2020-06-03T02:33:28Z

moveit_ros/visualization/motion_planning_rviz_plugin/src/motion_planning_frame.cpp

+{
+  static const double MIN_VAL = 1e-6;
+  static const std::map<std::string, shapes::ShapeType> SHAPES_MAP = {
+    { "box", shapes::BOX }, { "sphere", shapes::SPHERE }, { "cone", shapes::CONE }, { "cylinder", shapes::CYLINDER }


I think we should shoot for the moon and add shapes::MESH while we're at it

Would't that be redundant since there are already import buttons?

I had a closer look, and you're right: the "Import File" button actually accepts an STL file. I just assumed it's a scene file or something, since nothing tells you about the expected format. I'll fix that in a separate PR.

felixvd · 2020-06-03T02:36:08Z

...it_ros/visualization/motion_planning_rviz_plugin/src/ui/motion_planning_rviz_plugin_frame.ui

            <property name="text">
-             <string>Status</string>
+             <string>Import &amp;File</string>


I always wished this would say Import Scene, because it's not clear if the file is an object or a scene.

I don't recall changing this, I made my edits with QT designer so sometimes things get changed without one knowing about it.

felixvd · 2020-06-03T02:47:55Z

...it_ros/visualization/motion_planning_rviz_plugin/src/ui/motion_planning_rviz_plugin_frame.ui

+        </widget>
+       </item>
+       <item row="3" column="1">
+        <widget class="QGroupBox" name="groupBox_4">


How about changing this box to a layout like this? The Load button next to the dropdown menu would be greyed out unless the type is "MESH", and need the tooltip "Choose mesh location". Alternatively, a file dialog could open when "mesh" is selected in the dropdown menu. That would be very smooth!

I didn't want to add a second column as there were some concerns with increasing the panel's height. Furthermore, having yet another "scale" section might be confusing, would't "size" be more appropriate.
See my comments above on the "mesh" option

I drew this when the new section was still on the left. It's fine now, I just don't see a Resolve button to close this thread

felixvd · 2020-06-04T09:25:03Z

...it_ros/visualization/motion_planning_rviz_plugin/src/ui/motion_planning_rviz_plugin_frame.ui

+             <double>0.100000000000000</double>
+            </property>
+            <property name="value">
+             <double>1.000000000000000</double>


Would prefer a default value of 0.1 and a step value of 0.01. It will be a smoother experience and fit the dimensions of the demo/tutorial robot.

I guess it's not really important, I just feel like the default will force users to zoom out when their robot fills most of the screen. 0.2 and 0.05 could be a good compromise.

@felixvd I agree, I'll update those values

felixvd

Nothing to add, this is great.

jrgnicho · 2020-06-07T18:42:50Z

Nothing to add, this is great.

@felixvd thanks for reviewing

jrgnicho · 2020-06-17T14:52:54Z

This should be ready to merge unless someone has additional change requests

rhaschke · 2020-07-06T18:13:05Z

Great work! I file a new PR resolving the conflicts by a rebase in #2198.

jrgnicho added 2 commits June 2, 2020 15:56

added widgets and callbacks for adding geometric primitives

759a53f

added double spin box to set the size

c02a949

jrgnicho requested review from rhaschke and tylerjw as code owners June 2, 2020 21:19

moved primitive shapes widget to the right side of the panel

42964cd

JafarAbdi reviewed Jun 2, 2020

View reviewed changes

simonschmeisser requested changes Jun 3, 2020

View reviewed changes

added spin boxes for x, y and z length values, made suggested changes…

8b9810a

… as well

simonschmeisser approved these changes Jun 3, 2020

View reviewed changes

felixvd approved these changes Jun 4, 2020

View reviewed changes

felixvd mentioned this pull request Jun 6, 2020

Clean up Rviz plugin, fix mesh scaling #2142

Merged

4 tasks

Decreased step and initial values on size double spin boxes

81c81d3

felixvd approved these changes Jun 7, 2020

View reviewed changes

rhaschke mentioned this pull request Jul 6, 2020

Add primitive shapes widgets #2198

Merged

rhaschke closed this Jul 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add primitive shapes widgets #2137

Add primitive shapes widgets #2137

jrgnicho commented Jun 2, 2020

JafarAbdi Jun 2, 2020

simonschmeisser left a comment

simonschmeisser Jun 3, 2020

jrgnicho Jun 3, 2020

simonschmeisser Jun 3, 2020

simonschmeisser Jun 3, 2020

JafarAbdi commented Jun 3, 2020

AndyZe commented Jun 3, 2020

jrgnicho commented Jun 3, 2020

jrgnicho commented Jun 3, 2020

codecov bot commented Jun 3, 2020 •

edited

felixvd left a comment

felixvd Jun 3, 2020 •

edited

jrgnicho Jun 4, 2020 •

edited

felixvd Jun 5, 2020

felixvd Jun 3, 2020

jrgnicho Jun 4, 2020

felixvd Jun 3, 2020

jrgnicho Jun 4, 2020

felixvd Jun 5, 2020 •

edited

felixvd Jun 4, 2020 •

edited

felixvd Jun 4, 2020

jrgnicho Jun 4, 2020

felixvd left a comment

jrgnicho commented Jun 7, 2020

jrgnicho commented Jun 17, 2020

rhaschke commented Jul 6, 2020

	connect(ui_->add_shape_button, &QPushButton::clicked, [this]() { addPrimitiveShape(); });
	connect(ui_->add_shape_button, &QPushButton::clicked, this, &MotionPlanningFrame::addPrimitiveShape);

	QString("The '").append(selected_shape.c_str()).append("' is not supported."));
	QString("The shape '%1' is not supported.").arg(selected_shape.c_str()));

Add primitive shapes widgets #2137

Add primitive shapes widgets #2137

Conversation

jrgnicho commented Jun 2, 2020

Description

Checklist

Choose a reason for hiding this comment

simonschmeisser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JafarAbdi commented Jun 3, 2020

AndyZe commented Jun 3, 2020

jrgnicho commented Jun 3, 2020

jrgnicho commented Jun 3, 2020

codecov bot commented Jun 3, 2020 • edited

Codecov Report

felixvd left a comment

Choose a reason for hiding this comment

felixvd Jun 3, 2020 • edited

Choose a reason for hiding this comment

jrgnicho Jun 4, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixvd Jun 5, 2020 • edited

Choose a reason for hiding this comment

felixvd Jun 4, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixvd left a comment

Choose a reason for hiding this comment

jrgnicho commented Jun 7, 2020

jrgnicho commented Jun 17, 2020

rhaschke commented Jul 6, 2020

codecov bot commented Jun 3, 2020 •

edited

felixvd Jun 3, 2020 •

edited

jrgnicho Jun 4, 2020 •

edited

felixvd Jun 5, 2020 •

edited

felixvd Jun 4, 2020 •

edited