changing pose topics to goal_pose#1071
Conversation
mkhansenbot
left a comment
There was a problem hiding this comment.
I approve but let's not merge until the Rviz PR is merged
|
This brings up the issue - should we deprecate the topic based navigation completely? At one point we had discussed doing so and this warning was added to the code: |
|
This is a good point. It depends if we'd like to keep maintaining a goal_pose tool in our rviz plugin package https://github.com/ros-planning/navigation2/blob/master/nav2_rviz_plugins/src/goal_tool.cpp or not. If not, then we should keep and remove the warning. I don't know off hand the initial thought for the action-based goal pose tool, can you shed some light on that? |
|
The purpose in adding an action was to get the benefits of actions:
The tool is obviously a nice, easy way to send the action and be able to cancel it via Rviz. We should really upstream the tool to the standard Rviz plugins. We have kept the 'legacy' ability to send a goal via a topic also, as that is 'simple' to do from a command line. The question is whether we should deprecate it or not. |
|
Well I'm confused, the goal pose tool doesn't have the ability to cancel or give feedback, its a button click and a drag. Are you referring to the little pop up window that appears (and that has feedback?) - popups aren't ideal, I thought that was just someone's debug tool that was left behind, or are you talking about something else? That goal pose tool shouldn't be upstream-action-ized, because its not just used by navigation. That's a big reason I'm changing the topics to something more generic. Its just a 2D waypoint dropping tool, which we don't have to use or support. That's the motivating question I'm posing: do we want to support it or not? We shouldn't have both the tool plugins Edit: I think we should support it so that the default rviz toolset still works and delete the action-ized goal pose tool we have here. If we want to make a plugin for action, it should be a panel so there's a set of buttons to select for cancel/set/etc, the pop-up is an unideal hack |
|
This is a good conversation to have - I don't think there's a clear right or wrong answer, its just about what we want to do / support. |
Yes, that allows you to cancel the goal, but it doesn't have feedback. That could be added at a future date if someone wanted to but I don't think it's very useful in Rviz. However, ability to receive feedback in code via using and action is useful, which is why we added the navigate to pose action. We don't want to remove the action or the Rviz tool. I was just asking if we should keep supporting the topic as well, even though it can't be cancelled. I think the answer is 'Yes we should', in which case we should remove the deprecation warning. |
|
I dont think we should be supporting both since then there's 2 pretty much identical tools and not necessarily clear to a user the difference. That pop-up thing is not ideal. There's a number of plugin types in rviz to utilize, if a new window is required, then its likely the wrong plugin type was used. It should probably be a panel so it can have a dashboard of buttons to interact with the action and display feedback. If there's a navigation action panel and the rviz pose tool separately I think that's OK since there's a clear separation of uses. Popup to cancel isn't great, but a popup to cancel that also provides information? That falls squarely in the "this should be a panel" camp to me. Add that "startup / shutdown button" and its a one-stop-shop Nav2 panel |
|
We agree that the pop-up is not ideal. Unfortunately there was no way using that plug-in type to change the button after it's pressed to a 'cancel' button. We could go with a different plug in type, similar to the "startup" and "shutdown" buttons for the lifecycle_manager. I believe the reason that wasn't done was to try to replace the topic based tool with the new one and the topic based tool is of this plugin type.
We could do this if it makes things more clear to users. |
|
I'm confused, that plugin type for the startup/shutdown is just a panel with 1 button. How is that different to change the name to "cancel"? The panel is just a Qt plugin for which you can do whatever you want with or add any number of buttons or tools to. Here's an example https://github.com/SteveMacenski/slam_toolbox#rviz-plugin |
|
There are 2 different tools we're talking about here:
I was told that they were different types of plug-ins and the first one couldn't replace the button with the cancel button directly. I'm not a Qt plugin expert though, so there may be a way. If there is and someone submits a PR to change that, great. I think the other option you're suggesting is basically to move that button into the other panel. That might make sense too. We could add other enhancements as well such as cancelling all goals and/or monitoring lifecycle nodes. See issues #1067 and #1068 Let's remove the rest of the Rviz tool discussion from this PR. I think that's a good topic but should be done in either those other issues, or in a new issue to capture all the requirements. The question I asked was about keeping support for the topic based goal (now goal_pose in this PR). I think based on your answers, you do want to keep it. |
|
Agreed |
|
I just want to clarify a couple things since there were changes since #944 was merged:
This is no longer accurate. The current Nav2Goal tool tip sends a QSignal containing the goal pose. No popup is created anymore.
The panel button currently changes to "Cancel" and will trigger a cancel on the goal handle when pressed while a Nav2Pose goal is currently in progress. It will switch back to "Shutdown" after a cancel or when the goal is complete. I think feedback could be easily integrated from the nav2 rviz panel since it holds the client goal handle. Also, I'm ok with keeping the pose topic for backwards compatability/specific use cases. |
|
Got it @bpwilcox I'll submit a PR in the morning to removing the deprecation warning |
|
Thanks @bpwilcox for the update, I hadn't tried the new Rviz plugin yet, I've been using the dashing release binaries. |
Reflecting the fact move base doesnt exist and this ticket in RVIZ ros2/rviz#444