Clean up Rviz plugin, fix mesh scaling #2142

felixvd · 2020-06-06T13:38:10Z

Description

While reviewing #2137, I realized that the Rviz panel has an option to import meshes that I have never discovered, because the buttons and dialog windows are not very instructive.

This PR should clarify the interface as shown in the screenshot below, and adds a dialog to fix STL files that are encoded with mm units. Sadly, scaling the mesh did not work as I expected, as described here. I will remove the WIP when that's fixed.

Checklist

Required by CI: Code is auto formatted using clang-format
Extend the tutorials / documentation reference
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

codecov · 2020-06-06T14:10:32Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #2142      +/-   ##
==========================================
- Coverage   57.97%   57.96%   -0.02%     
==========================================
  Files         327      327              
  Lines       25676    25676              
==========================================
- Hits        14885    14882       -3     
- Misses      10791    10794       +3

Impacted Files	Coverage Δ
.../ompl_interface/src/detail/constrained_sampler.cpp	`46.15% <0.00%> (-17.95%)`	⬇️
...meterization/work_space/pose_model_state_space.cpp	`82.31% <0.00%> (-0.69%)`	⬇️
...raint_samplers/src/default_constraint_samplers.cpp	`82.54% <0.00%> (-0.37%)`	⬇️
moveit_core/robot_state/src/robot_state.cpp	`47.39% <0.00%> (+0.09%)`	⬆️
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp	`79.37% <0.00%> (+0.44%)`	⬆️
...nning_scene_monitor/src/planning_scene_monitor.cpp	`70.55% <0.00%> (+0.44%)`	⬆️
...on/pick_place/src/approach_and_translate_stage.cpp	`73.60% <0.00%> (+0.79%)`	⬆️

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 961b2e2...840e75d. Read the comment docs.

v4hn · 2020-06-08T11:30:59Z

as shown in the screenshot below

There are no screenshots and CI failed. :)

felixvd · 2020-06-08T13:21:35Z

I mixed up the screenshots and only uploaded the one in the other thread. I fixed the OP.

I think the new commit fixes the build warning, but I don't see the warning locally either way. Do I have to enable something to show it? catkin config doesn't have any --no-warnings settings or similar that I can see. I did run clang-format and clang-tidy before submitting the PR but apparently that didn't catch it.

rhaschke · 2020-06-08T13:31:07Z

We are building MoveIt on Travis with flags
CXXFLAGS="-Wall -Wextra -Wwrite-strings -Wunreachable-code -Wpointer-arith -Wredundant-decls". To see the same issues locally, you should use the same flags.

felixvd · 2020-06-08T18:37:23Z

Thanks. Is that worth adding to the doc somewhere?

mamoll · 2020-06-24T16:43:19Z

💯 This is a great idea! I ran into this issue myself when I was trying to create a realistic planning scene by populating it with some meshes I had downloaded. Besides scaling, you could also add some logic for positioning: if the bounding box of the imported mesh doesn't overlap with the bounding box of the current planning scene, ask the user whether the imported mesh should be centered in the current scene.

rhaschke

I don't see the need to renaming all the code symbols. However, I see your point that the UI should be improved.

rhaschke · 2020-06-25T08:48:05Z

...tion_planning_rviz_plugin/include/moveit/motion_planning_rviz_plugin/motion_planning_frame.h

@@ -172,8 +172,8 @@ private Q_SLOTS:
  void onClearOctomapClicked();

  // Scene Objects tab
-  void importFileButtonClicked();
-  void importUrlButtonClicked();
+  void import3DObjectFromFileButtonClicked();


IMO, you should revert renamings of all (internal) symbols. At most, I agree with importFrom[File|Url]...
If you want to clarify (for the user) that a single object (instead of a full scene) is imported, provide a tooltip to the corresponding buttons.

I'd go for "Object" over "3DObject", but I would like to separate a saved Scene from a saved Object more clearly. Is there a reason not to be descriptive?

I think the fact that these functions are closely related and somewhat easy to confuse is hinted at by computeLoadSceneButton being defined in motion_planning_frame_objects.cpp instead of motion_planning_frame_scenes.cpp . I assume the separation was introduced later during development.

I agree that we should distinguish scene and object loading/saving if needed. But can we actually import a whole scene?
If so, the corresponding symbols should be renamed to scene as well?
However, if there is only object loading, why should we rename the symbols?

I think it's sufficiently messy to clear up. There are:

import_file_button which connects to importFileButtonClicked and imports a mesh

import_url_button which connects to importURLButtonClicked and imports a mesh

import_scene_text_button which connects to importFromTextButtonClicked and imports a .scene file (containing scene geometry (shapes), but no other info)

load_scene_button which connects to loadSceneButtonClicked which loads a scene from moveit_warehouse (I don't know if this actually works)

load_state_button which connects to loadStateButtonClicked and loads robot states (not necessarily clear to first-time codebase readers that this is only the robot state, not the state of the scene)

load_query_button which loads planning queries, and a few corresponding save buttons.

The symbols are all in the same big motion_planning_frame file, so I think they should be clearer.

As mentioned below, I renamed some more of these.

moveit_ros/visualization/motion_planning_rviz_plugin/src/motion_planning_frame.cpp

moveit_ros/visualization/motion_planning_rviz_plugin/src/motion_planning_frame_objects.cpp

rhaschke · 2020-06-25T08:58:14Z

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

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


An "object" is imported in both cases. Hence, "File" to indicate that we import from file was correct, wasn't it? Please revert and better use tooltips if you want to be more specific.

Suggested change

<string>Import &Object</string>

<string>Import &File</string>

I find the hint that this imports a mesh rather than some other format (a scene? a YAML? some other list of objects?) is hard to discover. I think it should be more visible than a tooltip. How about Import Mesh?

To be clear, the problem is not that the button is incorrect, it's that it doesn't convey what a common user would be looking for or understand at first sight. Reading "Import File" as a new user makes me think "What kind of file? Ugh, I'll look into it later", "Import Mesh" makes me think "Oh cool, now I know I can do that".

The key point is that both buttons essentially import an object mesh, once from a local file, once from an url.
Hence the button names should distinguish file and url.

True, it is a bit of a break in consistency, but I value the discoverability higher. The current state leaves the arguably most important piece of information ("this imports an object mesh") unspoken.

I would prefer to express the main function on the more common button and let the user discover the secondary option via tooltips/dialogue details. E.g. one button "Import Mesh" with tooltip "Import mesh from file" and one button "From URL" with tooltip "Import mesh from URL".

I am making the silent assumption here that importing via URL is uncommon, but I think it's a reasonable one.

The whole GUI block is dealing with objects only as indicated by the frame header "Curren Scene Objects". As said above, changing the button text from file to object obscures the fact, that we will load from a file in contrast to from url.

From my experience that's not enough to indicate what the button does. It could also mean "Import Scene Objects", for example. The word "Mesh" is more important than "File" in my opinion, and would make the button a lot more expressive.

Another argument: This Import File button in the Current Scene Objects frame does not actually import a "Scene Object", it imports a mesh, and the button that actually imports a "Scene Object" (or multiple ones) is Import From Text in the Scene Geometry tab.

I just committed an alternative, will post the pictures below.

Suggested change:

Original below:

I might want to take out the "Scene" from "Scene Objects" and add a tooltip to "Publish Scene".

davetcoleman

Overall I really like this idea.

moveit_ros/visualization/motion_planning_rviz_plugin/src/motion_planning_frame.cpp

rhaschke

In general I approve like this. Would be great to augment file filters as suggested.

rhaschke · 2020-06-27T14:00:18Z

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

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


I still think that file would be better here.

felixvd · 2020-06-28T18:22:14Z

I added the file filter and fixed the scaling problem, which occurred because mesh scaling keeps the mesh's centroid constant, but the mesh centroid was not at the center of the interactive marker. It's fixable as discussed here, but considering that scaling is unlikely to be a very common operation, and there are more things to consider when scaling multi-shape objects, I would probably just leave it as is. For now, meshes imported via the Rviz plugin are shifted so that the center point is at the origin.

edit: And I renamed some more buttons and functions because I thought this is not a good way to be.

felixvd · 2020-06-28T18:43:05Z

Is Travis failing because CHOMP timed out? I tried building with the warnings enabled and ran clang-format and clang-tidy.

rhaschke

Some more remarks regarding the variable renamings...

...tion_planning_rviz_plugin/include/moveit/motion_planning_rviz_plugin/motion_planning_frame.h

moveit_ros/visualization/motion_planning_rviz_plugin/src/motion_planning_frame.cpp

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

felixvd · 2020-06-29T08:40:52Z

The build just passed, looks like it failed on a CHOMP test before.

felixvd · 2020-06-29T12:58:29Z

I fixed the comments, the misnamed button "Filter" and changed "State" to "Robot State" in the frame titles since there was space and it is not self-evident that the tab is about robot states.

Maybe we should stop before we iterate through the whole plugin. My thoughts on changes that could be a good WMD or newcomer issue:

The Stored Scenes tab should either work from local memory, like Stored States, or display a line "Set up a database to use this tab"
I think the Planning tab should be the default one, not Context.
Some functions in motion_planning_frame_objects should be moved to motion_planning_frame_scenes

rhaschke

More comments, sorry.

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

rhaschke · 2020-06-29T13:16:43Z

moveit_ros/visualization/motion_planning_rviz_plugin/src/motion_planning_frame_states.cpp

@@ -245,8 +245,21 @@ void MotionPlanningFrame::removeStateButtonClicked()

 void MotionPlanningFrame::clearStatesButtonClicked()
 {
-  robot_states_.clear();
-  populateRobotStatesList();
+  QMessageBox msg_box;


Why introducing a security message box, if the data is only removed from memory?

Confirmation dialogues in front of destructive operations are just prudent, in my opinion. You never know how many states the user has stored, or if they misunderstood the button. Consider non-native speakers, too.

For me, confirmation dialogues are just annoying 😉

moveit_ros/visualization/motion_planning_rviz_plugin/src/motion_planning_frame.cpp

rhaschke · 2020-06-29T15:38:54Z

moveit_ros/visualization/motion_planning_rviz_plugin/src/motion_planning_frame.cpp

-            break;
-          default:
-            return;  // Pressed cancel, do nothing
+          // 


Please cleanup this comment.

Fixed and rebased

clang-format

Fix comment

rhaschke

Apart from the "Import File" vs. "Import Mesh" button text, I approve.

felixvd · 2020-07-02T12:51:12Z

And I'm in favor of "Import Mesh" and happy to see it merged 😎

v4hn

I thought about proposing to drop the URL import altogether - it's not like click, download, select file is much of a hassle and it's a good idea to have the file stored locally anyway.
But then, there's no harm in having the button there and it keeps the grid symmetry 😎

The current patch looks good to me. I'll merge.

v4hn · 2020-07-02T15:09:13Z

Thanks to both of you for all these iterations

* Clarify object importing * Add suggestion (LARGE_MESH_THRESHOLD) * Add file filter * Add tooltip, clarify names * Fix and clarify names in robot states tab * Add dialogue to Clear button in robot states (cherry picked from commit 5af7258)

…gRequestAdapterChain classes (moveit#2142) * Cleanups * Add documentation and more cleanups * Revert size_t change

felixvd requested review from rhaschke and tylerjw as code owners June 6, 2020 13:38

felixvd mentioned this pull request Jun 6, 2020

scaleAndPadd does not keep the object centered moveit/geometric_shapes#148

Closed

felixvd changed the title ~~WIP: Clarify 3D object import button from Rviz~~ Clarify 3D object import button from Rviz Jun 18, 2020

felixvd changed the title ~~Clarify 3D object import button from Rviz~~ Clarify 3D object import button in Rviz Jun 18, 2020

felixvd changed the title ~~Clarify 3D object import button in Rviz~~ WIP: Clarify 3D object import button in Rviz Jun 18, 2020

rhaschke requested changes Jun 25, 2020

View reviewed changes

davetcoleman reviewed Jun 25, 2020

View reviewed changes

moveit_ros/visualization/motion_planning_rviz_plugin/src/motion_planning_frame.cpp Outdated Show resolved Hide resolved

moveit_ros/visualization/motion_planning_rviz_plugin/src/motion_planning_frame.cpp Outdated Show resolved Hide resolved

rhaschke approved these changes Jun 27, 2020

View reviewed changes

felixvd changed the title ~~WIP: Clarify 3D object import button in Rviz~~ Clarify 3D object import button in Rviz Jun 28, 2020

felixvd changed the title ~~Clarify 3D object import button in Rviz~~ Clarify object import button in Rviz, fix mesh scaling Jun 28, 2020

felixvd closed this Jun 29, 2020

felixvd reopened this Jun 29, 2020

rhaschke reviewed Jun 29, 2020

View reviewed changes

felixvd changed the title ~~Clarify object import button in Rviz, fix mesh scaling~~ Clean up Rviz plugin, fix mesh scaling Jun 30, 2020

felixvd added 3 commits June 30, 2020 12:01

Clarify object importing

573cd56

Add review comments (renaming)

c7340e5

Add suggestion (LARGE_MESH_THRESHOLD)

f9efde3

felixvd added 8 commits June 30, 2020 12:01

Alternative button naming

83c3723

clang-format

Add file filter

2ac7db3

Fix scaling

41ac6b6

Add tooltip, clarify names

6588fff

Add review comments (names, scaling)

9fcfda3

Fix and clarify names in robot states tab

c7bcdb8

Add dialogue to Clear button in robot states

eccc9bf

Simplify scaling and dialogue

840e75d

Fix comment

felixvd force-pushed the clarify-object-importing branch from 2c56b69 to 840e75d Compare June 30, 2020 03:01

rhaschke approved these changes Jun 30, 2020

View reviewed changes

v4hn approved these changes Jul 2, 2020

View reviewed changes

v4hn merged commit 5af7258 into moveit:master Jul 2, 2020

rhaschke mentioned this pull request Jul 6, 2020

Add primitive shapes widgets #2198

Merged

tylerjw mentioned this pull request Jul 18, 2020

July 2020 Melodic Release #2066

Closed

20 tasks

sjahr added a commit to sjahr/moveit that referenced this pull request Jun 21, 2024

Add documentation and cleanups for PlanningRequestAdapter and Plannin…

226f81c

…gRequestAdapterChain classes (moveit#2142) * Cleanups * Add documentation and more cleanups * Revert size_t change

	<string>Import &Object</string>
	<string>Import &File</string>

Clean up Rviz plugin, fix mesh scaling #2142

Clean up Rviz plugin, fix mesh scaling #2142

Conversation

felixvd commented Jun 6, 2020 • edited Loading

Description

Checklist

codecov bot commented Jun 6, 2020 • edited Loading

Codecov Report

v4hn commented Jun 8, 2020

felixvd commented Jun 8, 2020

rhaschke commented Jun 8, 2020 • edited Loading

felixvd commented Jun 8, 2020

mamoll commented Jun 24, 2020

rhaschke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixvd Jun 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixvd Jun 26, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davetcoleman left a comment

Choose a reason for hiding this comment

rhaschke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixvd commented Jun 28, 2020 • edited Loading

felixvd commented Jun 28, 2020

rhaschke left a comment

Choose a reason for hiding this comment

felixvd commented Jun 29, 2020

felixvd commented Jun 29, 2020

rhaschke 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

Choose a reason for hiding this comment

rhaschke left a comment

Choose a reason for hiding this comment

felixvd commented Jul 2, 2020

v4hn left a comment

Choose a reason for hiding this comment

v4hn commented Jul 2, 2020

felixvd commented Jun 6, 2020 •

edited

Loading

codecov bot commented Jun 6, 2020 •

edited

Loading

rhaschke commented Jun 8, 2020 •

edited

Loading

felixvd Jun 25, 2020 •

edited

Loading

felixvd Jun 26, 2020 •

edited

Loading

felixvd commented Jun 28, 2020 •

edited

Loading