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

Rename 2d Nav Goal to 2d Goal Pose + move_base_simple to goal_pose #455

Merged
merged 3 commits into from
Sep 4, 2019
Merged

Rename 2d Nav Goal to 2d Goal Pose + move_base_simple to goal_pose #455

merged 3 commits into from
Sep 4, 2019

Conversation

SteveMacenski
Copy link
Contributor

Per #444

Move base isn't the used tool for ROS2 navigation, and removing nav from the title makes it a more general waypoint dropping tool.

@@ -13,7 +13,7 @@ Panels:
- Class: rviz_common/Tool Properties
Expanded:
# - /2D Pose Estimate1
- /2D Nav Goal1
- /2D Goal Pose1
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'm not entirely clear if this is necessary, but defensively removed anything "Nav Goal" related

@SteveMacenski
Copy link
Contributor Author

@jacobperron following up on it. I have a Nav2 PR to match ros-navigation/navigation2#1071

@jacobperron jacobperron self-requested a review September 3, 2019 17:37
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

cpplint is complaining about the headers guards in goal_tool.hpp:

#ifndef header guard has wrong style, please use: RVIZ_DEFAULT_PLUGINS__TOOLS__GOAL_POSE__GOAL_TOOL_HPP_

Otherwise, LGTM!

@SteveMacenski
Copy link
Contributor Author

Alt Text

Will fix.

@jacobperron
Copy link
Member

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@jacobperron jacobperron merged commit 88847cb into ros2:ros2 Sep 4, 2019
@jacobperron
Copy link
Member

Thanks for the contribution!

@wjwwood
Copy link
Member

wjwwood commented Sep 5, 2019

Hey guys, I just noticed this touched files in the rviz folder, but that's just legacy. In the future please try to avoid that unless you're migrating code out of the rviz folder into one of the new packages. Hopefully the rviz folder should eventually evaporate once everything is ported.

@SteveMacenski
Copy link
Contributor Author

SteveMacenski commented Sep 5, 2019

Thanks for the heads up @wjwwood. What are the big elements left to port? Is there a migration ticket?

@jacobperron
Copy link
Member

@wjwwood Oops, my bad. I misread the file paths as starting with the name of the repo 🙃

@SteveMacenski You can find a list of things left to port in the README. There are also PRs open for a couple of them (e.g. #429 and #457).

@SteveMacenski
Copy link
Contributor Author

@jacobperron what's the blocker on #429 ? its been sitting around for awhile

@jacobperron
Copy link
Member

@jacobperron what's the blocker on #429 ? its been sitting around for awhile

I'm not sure there's any blocker. We just haven't gotten around to reviewing it.
I can try and make some time.

@SteveMacenski
Copy link
Contributor Author

Oh no pressure from me, I was just wondering if there were some actions I could help with to move it along if there was something to be done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants