Add primitive shapes widgets

[rhaschke]

Rebase of #2137 required due to conflicting changes in #2142. Great work, @jrgnicho! I was missing this feature several times in the past. In a cleanup commit, I tried to unify the UI to insert primitive objects / meshes.

[v4hn]

Scrolling through your "Unify" commit, this looks good and improves user experience further.
I'm a bit amused to see how you tend to fix your nagging requests for changes by throwing more changes after them, so that no one can argue against them anymore 🤡 (talking about the "mesh from URL" change of course)

I would approve, but I did not try this out, so I will leave it to someone else. Great to finally get primitives support in the GUI!

[codecov]

Codecov Report

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

@@            Coverage Diff             @@
##           master    #2198      +/-   ##
==========================================
+ Coverage   57.69%   57.70%   +0.01%     
==========================================
  Files         326      326              
  Lines       25661    25663       +2     
==========================================
+ Hits        14805    14809       +4     
+ Misses      10856    10854       -2     

Impacted Files	Coverage Δ
...veit_core/robot_model/src/floating_joint_model.cpp	45.65% <0.00%> (-3.27%)	⬇️
...ipulation/pick_place/src/manipulation_pipeline.cpp	76.41% <0.00%> (-0.95%)	⬇️
...nning_scene_monitor/src/planning_scene_monitor.cpp	69.89% <0.00%> (-0.66%)	⬇️
...raint_samplers/src/default_constraint_samplers.cpp	82.54% <0.00%> (-0.37%)	⬇️
moveit_core/robot_state/src/robot_state.cpp	47.00% <0.00%> (-0.30%)	⬇️
...meterization/work_space/pose_model_state_space.cpp	82.99% <0.00%> (+0.68%)	⬆️
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp	78.92% <0.00%> (+2.24%)	⬆️
moveit_core/robot_model/src/joint_model_group.cpp	61.81% <0.00%> (+2.42%)	⬆️
...e/src/parameterization/model_based_state_space.cpp	73.10% <0.00%> (+2.75%)	⬆️

---

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 0274ea9...85f62d8. Read the comment docs.

[rhaschke]

I'm a bit amused to see how you tend to fix your nagging requests for changes

I should have pushed Felix to join forces with Jorge and come up with a unified solution in the first place, yes.
But, I didn't notice #2137 before.

[rhaschke]

I just noticed, that the changes a user applies to the PlanningScene through this GUI are local to the rviz plugin only and not (yet) propagated to the move_group's central planning scene. I suggest doing so, because the added collision objects are not considered for planning otherwise. I'm working on this right now. So long, I marked this PR as WIP.
Does anyone have any objections, @v4hn, @henningkayser, @felixvd?

If we don't agree on this, we definitely need to update the query robot states after an object was attached or detached. Otherwise they don't notice, and the behaviour is inconsistent even locally in rviz!
This could be done by a call to MotionPlanningDisplay::onSceneMonitorReceivedUpdate.

[rhaschke]

Actually, I have chosen a middle ground solution now: Changes to the local planning scene are handled locally only, but are explicitly noticed (enabling the Publish button). Only before planning, the user is asked whether to publish all local changes if not yet done. This explicitly brings the fact to the user's attention that rviz maintains its own planning scene.

As I added a lot of independent commits now, this PR should be merged as a proper merge commit.

[v4hn]

Yes, the need to press "publish scene" after changing the scene was not obvious and improving this is a good idea.
Even worse, I'm not sure what happens when a new global scene is received between changing something and publishing it.
I suspect additional issues there.

Thanks for working on it.

[felixvd]

Thanks for working on this! I love the warning before planning without publishing the scene. I think I would have had no objections even to automatically publishing changes, but this way the user is familiarized with the "Publish to update" concept.

I left comments, but some general issues:

• On two 18.04 machines (containers) I tested, the + - X icons did not appear. If the "Add", "Del" etc. text is a fallback, the button is not big enough to display it.
• The Size spinbox fields seem a little wide. Is there a way to make them smaller? It looks like there is a little dead space overall, but it might just be Qt.

Apart from that, if we're overhauling the GUI we might as well include #2061 :)
And remove the '.00' from the "Planning Attempts" option (it's an integer).
And figure out why the panel is so wide.

[felixvd]

Even worse, I'm not sure what happens when a new global scene is received between changing something and publishing it.

That seems like an argument to keep the plugin's copy and the move_group node's PlanningScene in sync. I wouldn't mind that either.

[rhaschke]

That seems like an argument to keep the plugin's copy and the move_group node's PlanningScene in sync. I wouldn't mind that either.

This would be rather slow: Updates would need to be sent to the move_group's scene and the received back in rviz' scene.

[rhaschke]

Thanks, @felixvd, for your comments.

On two 18.04 machines (containers) I tested, the + - X icons did not appear. If the "Add", "Del" etc. text is a fallback, the button is not big enough to display it.

Maybe, we need to embed the icons into the plugin? These are default theme icons and should™ be available.

The Size spinbox fields seem a little wide. Is there a way to make them smaller? It looks like there is a little dead space overall, but it might just be Qt.

Qt computes the minimal size of those spinboxes by the maximal width of numbers that can be potentially entered into them.
Thus, the width is determined by min and max values, which are 99?

Apart from that, if we're overhauling the GUI we might as well include #2061 :)

I will have a short look.

And remove the '.00' from the "Planning Attempts" option (it's an integer).

That was already done in #2076?!?

And figure out why the panel is so wide.

I investigated yesterday: The width essentially boils down to the width of the "object position" spinboxes. As stated above, their width is determined by the min/max values, which were -1000...1000 and now are -999...999. So, I already saved 3x1 digits.

[rhaschke]

I have set the default tab to "Planning" now. Fixes #2061.

[felixvd]

I don't know what is wrong with the icons either, but in my local container and in moveit/moveit:melodic-source it looks like this:

I like the new size, but now those buttons are very non-descript. It's not clear what 3 numbers and "Box" will do. On thinking about it further, this UI element should really have a header. How about something like this? "Add/remove scene object" would also be good:

If we can fix those two issues this should be good to merge. Looks great already.

Some unrelated UI notes:

• Objects cannot be unselected in the Current Scene Objects box. Seems like there are only workarounds for this.
• In the "Change Object Pose and Scale" box, clicking on the slider bar (rather than dragging the slider) does not update the object scale for me. Is this only on my end? It looks like clicking does not trigger valueChanged for some reason. I didn't find a workaround on first googling.

That was already done in #2076?!?

No idea what happened there, it does look good for me now too.

[simonschmeisser]

+1 from me for directly synchronizing, I don't think users need to know that there are separate copies and might actually trigger planning by separate means? The update rate back to rviz could be increased ...

[felixvd]

Finally got to test it, works like a charm, approved to the moon and back.