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.

Sign up for GitHub

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

Jump to bottom

Ability to hide visualized robot states #55

Merged
merged 9 commits into from
Aug 14, 2019
Merged

Ability to hide visualized robot states #55

merged 9 commits into from
Aug 14, 2019

Conversation

davetcoleman
Copy link
Member

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.

This was referenced Aug 7, 2019
Closed
Merged
Closed
rhaschke
rhaschke reviewed Aug 9, 2019
View reviewed changes
Copy link
Contributor

@rhaschke rhaschke left a 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.

msg/DisplayRobotState.msg Outdated Show resolved Hide resolved
msg/DisplayRobotState.msg Outdated Show resolved Hide resolved
msg/DisplayRobotState.msg Outdated Show resolved Hide resolved
davetcoleman and others added 4 commits August 13, 2019 14:21
@davetcoleman @rhaschke
Update msg/DisplayRobotState.msg
039f7d6 
Co-Authored-By: Robert Haschke <rhaschke@users.noreply.github.com>
@davetcoleman @rhaschke
Update msg/DisplayRobotState.msg
0a560ca 
Co-Authored-By: Robert Haschke <rhaschke@users.noreply.github.com>
@davetcoleman
Update DisplayRobotState.msg
ac1b372
@davetcoleman
Update DisplayRobotState.msg
aae565f
@davetcoleman
Copy link
Member Author

Reworded comment

@JafarAbdi JafarAbdi mentioned this pull request Aug 14, 2019
Merged
@rhaschke
Copy link
Contributor

Sorry for being stubborn regarding the comments. Maybe we can have a third opinion from, e.g. @v4hn?
I also fixed the Travis issue: bool should be lower case.

@v4hn
Copy link
Contributor

v4hn commented Aug 14, 2019

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.
It's definitely true this does not have to be limited to RViz, but it is limited to arbitrary visualizations, that's what the message name says.
Of course someone can abuse the message, but that's nothing we need to account for in the documentation.

@rhaschke I also renamed the variable again. You +1'ed Dave's comment there, so I guess you accidentally renamed it again.

rhaschke
rhaschke reviewed Aug 14, 2019
View reviewed changes
Copy link
Contributor

@rhaschke rhaschke left a 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
Copy link
Contributor

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.

Copy link
Contributor

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
Copy link
Contributor

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.

Copy link
Contributor

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.

@v4hn
Copy link
Contributor

v4hn commented Aug 14, 2019

I think we slowly converge. ;)

I don't like the wording "this state" here, as - if hide is true - there is no need to provide a RobotState at all.

Technically you always provide a RobotState message. It might be empty though...

I went back to hide as I agree with your reasoning there.
Giving the example of RViz seems alright, though not strictly necessary.

I removed your second comment line because it seems to imply that you can send an empty DisplayRobotState with hide=true to show a previously send state.
This might seem convenient at first, but would add more state than necessary to the topic (which e.g. latched topics cannot maintain). I would prefer to keep the semantics within one message, saying "here is (or might be) a robot state with highlights, but do not visualize it."

@v4hn v4hn added the wip label Aug 14, 2019
rhaschke
rhaschke approved these changes Aug 14, 2019
View reviewed changes
Copy link
Contributor

@rhaschke rhaschke left a 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 😉

@davetcoleman davetcoleman removed the wip label Aug 14, 2019
@davetcoleman davetcoleman changed the title WIP Ability to hide visualized robot states Ability to hide visualized robot states Aug 14, 2019
@davetcoleman
Copy link
Member Author

You guys are totally bike-shedding this!

I don't care enough to debate further, this works for me.

@davetcoleman davetcoleman merged commit 108a4f1 into moveit:master Aug 14, 2019
@davetcoleman davetcoleman deleted the hide_robot_state branch August 14, 2019 18:31
@rhaschke
Copy link
Contributor

You guys are totally bike-shedding this!

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.

@v4hn
Copy link
Contributor

v4hn commented Aug 15, 2019 •
edited
Loading

You guys are totally bike-shedding this!

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.

@JafarAbdi JafarAbdi mentioned this pull request Aug 15, 2019
Merged
6 tasks
@JafarAbdi JafarAbdi mentioned this pull request Aug 27, 2019
Closed
@tylerjw tylerjw mentioned this pull request May 8, 2020
Closed
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@v4hn v4hn v4hn left review comments

@rhaschke rhaschke rhaschke approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@davetcoleman @rhaschke @v4hn