-
Notifications
You must be signed in to change notification settings - Fork 948
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
rviz plugin: joints tab #1308
Conversation
ae28160
to
8ef3438
Compare
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? |
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.
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). |
moveit_core/robot_state/include/moveit/robot_state/robot_state.h
Outdated
Show resolved
Hide resolved
moveit_core/robot_model/include/moveit/robot_model/joint_model.h
Outdated
Show resolved
Hide resolved
moveit_ros/visualization/motion_planning_rviz_plugin/src/motion_planning_frame_joints.cpp
Outdated
Show resolved
Hide resolved
This is a great feature, well done!
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: |
@davetcoleman wrote:
Something like this: awesomebytes/move_joints_interactive? It's not perfect, but is the closest to how |
Ok, moved them below and added a label now.
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?
I like the idea to have joint sliders, but only if there is no IK solver available / no Cartesian marker. Also, choosing the right size and position of the joint markers is tricky... |
8ef3438
to
ca1f14a
Compare
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? |
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.
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. |
moveit_core/robot_model/include/moveit/robot_model/revolute_joint_model.h
Outdated
Show resolved
Hide resolved
...anning_rviz_plugin/include/moveit/motion_planning_rviz_plugin/motion_planning_frame_joints.h
Outdated
Show resolved
Hide resolved
...anning_rviz_plugin/include/moveit/motion_planning_rviz_plugin/motion_planning_frame_joints.h
Outdated
Show resolved
Hide resolved
...anning_rviz_plugin/include/moveit/motion_planning_rviz_plugin/motion_planning_frame_joints.h
Outdated
Show resolved
Hide resolved
...anning_rviz_plugin/include/moveit/motion_planning_rviz_plugin/motion_planning_frame_joints.h
Outdated
Show resolved
Hide resolved
...anning_rviz_plugin/include/moveit/motion_planning_rviz_plugin/motion_planning_frame_joints.h
Outdated
Show resolved
Hide resolved
moveit_ros/visualization/motion_planning_rviz_plugin/src/motion_planning_frame_joints.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")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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")); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Not at my side. Seems like you use a wider system font...
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).
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.
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 |
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. |
4ea2c87
to
2bf863b
Compare
... to display joint values of recently modified start/goal state
- immediately start editing when item is clicked once - drop indentation in first column
2bf863b
to
921fdb9
Compare
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... |
There was a problem hiding this 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.
I changed the branch to master. |
I'm excited about this no matter if the sliders behave a little unintuitively. Would love to see it merged. |
626bbca
to
64f282f
Compare
I've put some more effort into this to get a more intuitive/immediate mouse behaviour. |
Codecov Report
@@ 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.
|
- get rid of PercentageRole - use Q_PROPERTY for editor
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.
This PR also introduces methods
harmonizePositions()
inJointModel
andRobotState
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...