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

rviz plugin: joints tab #1308

Merged
merged 12 commits into from
Nov 28, 2019
Merged

Conversation

rhaschke
Copy link
Contributor

@rhaschke rhaschke commented Jan 15, 2019

I was missing for a long time the possibility to directly actuate joints in the rviz motion planning plugin.
This PR adds a new tab "Joints" to the panel, which lists all joints of the current move group and allows to operate them via sliders within their bounds. For unbounded joints, an editing field will be displayed.
Additionally to this, sliders are displayed to navigate the redundant space of the robot. If there is only a single redundant DoF, the approach works quite nice already (see the Panda video).
For multiple redundant DoFs, we need some more work to nicely align nullspace directions between different frames.
redundancy-exploration

This PR also introduces methods harmonizePositions() in JointModel and RobotState that map joint angles of revolute joints that are outside the boundaries back into the feasible range by adding / subtracting multiples of 2pi. Such a function was implemented in several IK plugins before - I have seen it in LMA and IKFast...

@simonschmeisser
Copy link
Contributor

Looks nice! The redundancy slider is not completely obvious to me however. Maybe move it below the joints list with a clear visual separation and a header? Will this slider also appear in singularities?

@rhaschke
Copy link
Contributor Author

Maybe move it below the joints list with a clear visual separation and a header?

To emphasize the different semantics of the NS sliders, I deliberately placed them next to the joint sliders and not below. If there are many joints, the NS sliders wouldn't be visible as well.

Will this slider also appear in singularities?

Yes, of course. As an example, have a look at the Panda arm in zero pose, which has two nullspace dimensions (because three joint axes are aligned).

@davetcoleman
Copy link
Member

This is a great feature, well done!

Maybe move it below the joints list with a clear visual separation and a header?

I agree with @simonschmeisser the current UI layout isn't the most intuitive. We need some kind of label or icon indicating what the vertical slider does and how its different than the other sliders. I like his idea of having it below.

Also, the highlighting of a particular joint row is confusing to me - can we disable that behavior? In the gif it mislead my initial understanding of what the redundant slider does.

Not necessary in this PR but just a future suggestion: in the original arm_navigation project we had circular sliders overlayed on each joint that allowed this same per-joint functionality. It was pretty slick and was basically a simplified interactive marker:
image

@gavanderhoorn
Copy link
Contributor

@davetcoleman wrote:

Not necessary in this PR but just a future suggestion: in the original arm_navigation project we had circular sliders overlayed on each joint that allowed this same per-joint functionality. It was pretty slick and was basically a simplified interactive marker:
image

Something like this: awesomebytes/move_joints_interactive?

It's not perfect, but is the closest to how arm_navigation did it I've found so far.

@rhaschke
Copy link
Contributor Author

I like his idea of having the sliders below.

Ok, moved them below and added a label now.

The highlighting of a particular joint row is confusing to me - can we disable that behavior?

This is the standard highlighting of the selected joint row (which stays highlighted even if focus moves somewhere else). @davetcoleman, what kind of behaviour do you want to have?

... in the original arm_navigation project we had circular sliders overlayed on each joint ...

I like the idea to have joint sliders, but only if there is no IK solver available / no Cartesian marker.
Otherwise the multitude of markers will become overwhelming / confusing too. Having Cartesian markers on all end-effectors (finger tips) of our bimanual Shadow hand setup as well as two markers on the arm end-effectors, we already sum up to 14 markers:
bimanual

Also, choosing the right size and position of the joint markers is tricky...

@davetcoleman
Copy link
Member

Something like this: awesomebytes/move_joints_interactive?

Yes exactly. I think this behavior should be an optional view that you wouldn't want at the same time as the normal goal pose interactive marker.

This whole interface @rhaschke is adding could likely be in its own Display panel, rather than inside the moveit one. What do you think about that @rhaschke? Or are you using a lot of IK dependencies that would be bulky to load in a different Rviz plugin?

@rhaschke
Copy link
Contributor Author

I think this behavior should be an optional view that you wouldn't want at the same time as the normal goal pose interactive marker.

I agree. I was thinking of this mode as a default when no Cartesian pose marker is available. Using the marker menu, we could switch between these modes.

This whole interface @rhaschke is adding could likely be in its own Display panel.

I don't agree. This interface was originally intended to monitor the joint values (particularly closeness to joint limits) of the start and goal query states. Hence, it needs to be integrated with the current display.

@davetcoleman
Copy link
Member

It concerns me we now need scroll bars to access all the tabs in the motion planning plugin:
image

I think this new functionality could be a separate panel.

I'm really excited with this PR playing around with the joints:
image

However:

  • The slider interface is very hard to use. It took me a while to realize you have to double click to gain control of the slider bars. Once 'enabled', you don't need to hold down the mouse. But you do have to keep the mouse hovering inside the progress bar box. Why did you choose this non-standard GUI approach?
  • The nullspace exploration bar would make you think it represents the min and max range of the free joint. In fact all it really is doing is providing a left and right button without any known min or max. I think two buttons with arrows would be more intuitive e.g. < >

moveit_core/robot_model/src/revolute_joint_model.cpp Outdated Show resolved Hide resolved
{
ui_->joints_view_->setModel(model);
ui_->joints_view_label_->setText(
QString("Group joints of %1 state").arg(model == start_state_model_.get() ? "start" : "goal"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
QString("Group joints of %1 state").arg(model == start_state_model_.get() ? "start" : "goal"));
QString("Manual Joint Adjustment of %1 State:").arg(model == start_state_model_.get() ? "Start" : "Goal"));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As said, I see this primarily as a display and not as an adjustment tool.
Why do you want to change case of all worlds?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I ran your code locally it looked sloppy without capitalization, as I see this as a section title

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, you presented this primarily as a adjustment tool in the description of this PR:

I was missing for a long time the possibility to directly actuate joints in the rviz motion planning plugin.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Touché. But, actually I started from a display and it turned into an interaction later...
No matter. I don't like to switch to "normal" sliders for the following reasons:

  • Continuous or unbounded joints cannot be handled with a slider in a meaningful way. For them, an input field is necessary.
  • Using a model-view approach is much more flexible here.

I could have a look into how to improve the interaction though. I remember there are several modes available for a view to enter the editor of an item. But that's nothing I can promise anytime soon. MoveIt is already consuming way to much of my time and there definitely more pressing issues in MoveIt.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just have a QT Group (perhaps encapsulated as a widget) with horizontal layout with a label widget and traditional slider widget? The slider could still reflect the status of the joints via update calls to adjust them automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's exactly what I suggested, although there is nothing like a Qt Group. Do you mean QGroupBox? I guess, we don't want to have it's frame.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the look-and-feel is not yet optimal. But, just to clarify the situation: If we don't drop the idea of the model-view approach (which I will not do, somebody else is cordially invited to do so though), we cannot show the sliders all the time. If editing is triggered, the display will change from a progress bar to a slider (+ label), which I think is strange too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think QGroupBox is what I meant, from the top of my head. I'm fairly certain you can disable the frame via the properties as I've done that before.

I'm not sure why you are saying the model-view approach is preventing you from having a better UI. Why could there not be a callback to auto-adjust the sliders as the robot moves, which is disabled if the user clicks on the sliders?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QGroupBox is intended to group buttons, not arbitrary widgets, but yeah might work for this purpose.

The model-view approach distinguishes between display and editing. Usually, there is only one editor (slider) open in a view at any time. If that's fine for you, great. If you want to see all the sliders all the time, we need a completely different approach, namely creating all those slider widgets as you do, e.g. in setup assistant.

@rhaschke
Copy link
Contributor Author

It concerns me we now need scroll bars to access all the tabs in the motion planning plugin

Not at my side. Seems like you use a wider system font...
I wouldn't care too much to have scroll bars though.

The slider interface is very hard to use. Why did you choose this non-standard GUI approach?

In first place this is a TableView and not a gui interaction. As I said, primarily I wanted to monitor the joint values (instead of changing them).

The nullspace exploration bar would make you think it represents the min and max range of the free joint.

There is no min and max written. Why would you think so? By the way, there is nothing like a "free joint", nor "nullspace coordinates". There is only a non-linear manifold which you can navigate.

In fact all it really is doing is providing a left and right button without any known min or max. I think two buttons with arrows would be more intuitive e.g. < >

No, that's not true. It's a velocity control slider: The more far to the left/right you move, the faster the robot moves in the nullspace. That's not possible with simple buttons.

@davetcoleman
Copy link
Member

davetcoleman commented Jan 23, 2019

No, that's not true. It's a velocity control slider: The more far to the left/right you move, the faster the robot moves in the nullspace. That's not possible with simple buttons.

I observed it closer and I've confirmed it does move faster, my bad. It can easily be missed, though.

I think this is a great contribution and don't want to hold it up too long from being merged. However, unlike more internal code cleanliness and layout, its much less desirable to be iterating on user-facing UIs and I'd like to get that right now. From a product manager perspective, I'd like to see the individual joint slider interactions cleaned up before this gets merged

@rhaschke rhaschke added enhancement help wanted please consider improving this request! more work needed Everyone is invited to contribute labels Jan 24, 2019
@rhaschke
Copy link
Contributor Author

I tweaked the view settings a bit to get rid of the double-click experience (f4d33d2). However, mouse interaction still feels strange, as the progress-bar sliders reacts w/o mouse button being pressed and there is another click+release needed to finish the editor.
I also quickly added a QSlider as an editor (4ea2c87). Here usage is even worse: you cannot "leave" the editor other than selecting something else.
Maybe indeed, we need to drop the idea of using the model-view approach but use classic widgets instead. I don't have cycles for this though and everybody taking over is very welcome.

@davetcoleman
Copy link
Member

I re-ran this locally and I appreciate the improved UI with slider widget. However, the robot doesn't move until the mouse looses focus from the slider bar section. This is still unintuitive...

Copy link
Member

@davetcoleman davetcoleman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of good work was put into this PR, yet it has sat dormant for too long. @rhaschke as long as you are open to us further tweaking the user interaction in the future, I think we should move forward with merging this asap.

@davetcoleman davetcoleman changed the base branch from melodic-devel to master November 21, 2019 01:09
@davetcoleman
Copy link
Member

I changed the branch to master.

@felixvd
Copy link
Contributor

felixvd commented Nov 26, 2019

I'm excited about this no matter if the sliders behave a little unintuitively. Would love to see it merged.

@rhaschke
Copy link
Contributor Author

I've put some more effort into this to get a more intuitive/immediate mouse behaviour.

@rhaschke rhaschke removed help wanted please consider improving this request! more work needed Everyone is invited to contribute labels Nov 28, 2019
@codecov-io
Copy link

codecov-io commented Nov 28, 2019

Codecov Report

Merging #1308 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1308   +/-   ##
=======================================
  Coverage   47.53%   47.53%           
=======================================
  Files         290      290           
  Lines       22932    22932           
=======================================
  Hits        10901    10901           
  Misses      12031    12031

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 a1c0ade...c982526. Read the comment docs.

@rhaschke rhaschke added the more work needed Everyone is invited to contribute label Nov 28, 2019
- get rid of PercentageRole
- use Q_PROPERTY for editor
@rhaschke rhaschke merged commit acead73 into moveit:master Nov 28, 2019
@rhaschke rhaschke deleted the rviz-plugin-joints-tab branch November 28, 2019 21:02
rhaschke added a commit to ubi-agni/moveit that referenced this pull request Nov 28, 2019
sjahr pushed a commit to sjahr/moveit that referenced this pull request Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement more work needed Everyone is invited to contribute
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants