-
Notifications
You must be signed in to change notification settings - Fork 943
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
removing regression introduced in #658 #1040
removing regression introduced in #658 #1040
Conversation
Technically this looks ok, but I'm wondering whether removing the Perhaps @davetcoleman remembers why it was added in the first place. |
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 approve this change. For me, the suffix "kConfigDefault" always looked strange. However, it also acts as a reminder, that one can have multiple different configurations for a given OMPL planner - with different parameters.
@rhaschke wrote:
exactly. The entries in the config files and on the parameter server are not planners, but only sets of configuration items. Removing that suffix makes that slightly less obvious, but perhaps not enough to keep it. |
Please cherry-pick to melodic @mlautman
|
Thanks for this bug fix @mlautman ! |
Description
Thisa regressions introduced in #658 which was raised in issue #955
This stops the setup assistant from appending
kConfigDefault
to planner names and correctly trimskConfigDefault
from existing ompl_planning.yaml files.Checklist