-
Notifications
You must be signed in to change notification settings - Fork 128
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
Ability to hide visualized robot states #55
Conversation
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 think this is a useful extension. Please consider the comments.
Co-Authored-By: Robert Haschke <[email protected]>
Co-Authored-By: Robert Haschke <[email protected]>
Reworded comment |
Sorry for being stubborn regarding the comments. Maybe we can have a third opinion from, e.g. @v4hn? |
As the current commit history of this request is messy anyway, I just added another commit, aiming for a middle way. @rhaschke I also renamed the variable again. You +1'ed Dave's comment there, so I guess you accidentally renamed it again. |
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 pushed yet another alternative for wording. See the comments for some arguments.
msg/DisplayRobotState.msg
Outdated
@@ -4,5 +4,6 @@ RobotState state | |||
# Optionally, various links can be highlighted | |||
ObjectColor[] highlight_links | |||
|
|||
# Optionally, hide the robot state (in rviz) if true | |||
bool hide | |||
# Optionally, do not display this state in visualizations |
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 don't like the wording "this state" here, as - if hide is true - there is no need to provide a RobotState at all.
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 optional is kind of misleading here. An array can be empty (and thus optional), but a simple scalar variable cannot be optional. However, the default value of zero (false
) is very useful here 😉
msg/DisplayRobotState.msg
Outdated
bool hide | ||
# Optionally, do not display this state in visualizations | ||
# and hide previously sent states | ||
bool hide_display |
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.
Actually, I changed back to hide
intentionally (but I should have commented).
This variable never stands on its own, but it's always used with a prefix like display_robot_state.hide
. In this context, I think the short name is sufficient.
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.
makes sense and I agree.
I think we slowly converge. ;)
Technically you always provide a I went back to I removed your second comment line because it seems to imply that you can send an empty |
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 like the present state. Hopefully, @davetcoleman can agree as well 😉
You guys are totally bike-shedding this! I don't care enough to debate further, this works for me. |
I think it's very important to carefully choose the message API. It will become extremely hard to change this in the future. Hopefully, we found a good compromise. |
True, but the bikeshed will be used by a few hundred people and break ABI compatibility with kinetic/melodic. In my opinion message definitions are the only place where this level of discussion makes sense. |
This PR is WIP because I haven't implemented the necessary changes in the MoveIt Rviz Plugin for displaying a robot state, but I don't think it would be hard.
This is a useful feature because I've used a workaround for it for years, but the workaround causes issues. The workaround, used in MoveIt Visual Tools here, is to move the robot to a very far away location in Rviz, such that its invisible.
With this new flag, we could remove the need for a virtual_joint that moves the robot, and instead just set a bool.
If there are no issues with this feature, I will eventually implement the rest of the change. Or ask someone else to hopefully implement it for me.