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

change default goal to goal_pose and not just in default rviz #491

Merged
merged 1 commit into from
Dec 18, 2019
Merged

change default goal to goal_pose and not just in default rviz #491

merged 1 commit into from
Dec 18, 2019

Conversation

SteveMacenski
Copy link
Contributor

@SteveMacenski SteveMacenski commented Dec 11, 2019

#455 updated the tool name, registration, default rviz files and documentation.

However the migration guide for Eloquent isn't technically correct (https://index.ros.org/doc/ros2/Releases/Release-Eloquent-Elusor/#rviz). The tool name changed, true, and the topic was changed for the default.rviz file. This is also where /move_base_simple/goal was defined from goal. There never was /move_base_simple/goal in the rviz tool file.

If this is what we're going to say in the migration guide, we should change the actual default hardcoded in the tool to be goal_pose, rather than just a configuration change in the rviz file.

I tried to think of a way to explain this better for the migration guide and decided that its just easier to align with it. If that's what people walked away thinking that PR did exactly, I should follow that.

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.

Since I would think that the values in the default config file are effectively the defaults for the majority of users, I think we can skip backporting this change to Eloquent. If you want, we could clarify on the release page that the change was in the default config file.

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

@SteveMacenski
Copy link
Contributor Author

I agree with your statement. In fact, it wasn't until that was brought to my attention that /move_base_simple/goal wasnt in fact the hardcoded default in the code. It effectively is if you start your custom config from the default config.

I don't think any additional clarification is required. This change will make the updates required so that no additional clarification is necessary. Otherwise, I might have to reword it.

@jacobperron jacobperron merged commit 7f7a481 into ros2:ros2 Dec 18, 2019
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

2 participants