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

Add new constructor to cosmtap2DROS #3222

Merged
merged 3 commits into from
Sep 26, 2022
Merged

Add new constructor to cosmtap2DROS #3222

merged 3 commits into from
Sep 26, 2022

Conversation

tonynajjar
Copy link
Contributor

@tonynajjar tonynajjar commented Sep 23, 2022 •
edited
Loading

Basic Info

Info Please fill out this column
Ticket(s) this addresses (3213)
Primary OS tested on Ubuntu
Robotic platform tested on Gazebo

Description of contribution in a few bullet points

  • Added a new constructor in cosmtap2DROS to allow setting namespace and other options from launch file

Description of documentation updates required from your changes

Future work that may be required in bullet points

For Maintainers:

  • Check that any new parameters added are updated in navigation.ros.org
  • Check that any significant change is added to the migration guide
  • Check that any new features OR changes to existing behaviors are reflected in the tuning guide
  • Check that any new functions have Doxygen added
  • Check that any new features have test coverage
  • Check that any new plugins is added to the plugins page
  • If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

@mergify
Copy link
Contributor

mergify bot commented Sep 23, 2022

@tonynajjar, please properly fill in PR template in the future. @SteveMacenski, use this instead.

  • Check that any new parameters added are updated in navigation.ros.org
  • Check that any significant change is added to the migration guide
  • Check that any new features OR changes to existing behaviors are reflected in the tuning guide
  • Check that any new functions have Doxygen added
  • Check that any new features have test coverage
  • Check that any new plugins is added to the plugins page
  • If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

@tonynajjar
Copy link
Contributor Author

@SteveMacenski I'm not entirely sure I understood what you meant in the comment. Please let me know if I'm far off

@SteveMacenski SteveMacenski linked an issue Sep 23, 2022 that may be closed by this pull request
costmap_2d_node: enable setting namespace from launch files #3213
Closed
tonynajjar
tonynajjar commented Sep 23, 2022
View reviewed changes
Copy link
Contributor Author

@tonynajjar tonynajjar left a comment •
edited
Loading

Choose a reason for hiding this comment

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

2 questions:

Can we now remove the (3-years old) TODO from @orduno? I'm not sure what is meant with Pass a sub-node instead of creating a new node

Should this constructor be the default for the standalone node?

@SteveMacenski
Copy link
Member

Should this new constructor be used in the node main file?

@tonynajjar
Copy link
Contributor Author

Should this new constructor be used in the node main file?

Jinx 😁 I would think so yes, it can be confusing when the name and namespaces are not being set from the launch file

@SteveMacenski
Copy link
Member

Just waiting on the main updates and I'll merge. Please prepare a navigation.ros.org PR to detail the change in the migration guide

@tonynajjar
Copy link
Contributor Author

tonynajjar commented Sep 23, 2022 •
edited
Loading

Just waiting on the main updates and I'll merge

Ah ok I was waiting for confirmation. main updated

Please prepare a navigation.ros.org PR to detail the change in the migration guide

You mean to detail the changes needed in the launch files for people using the standalone node?

@SteveMacenski
Copy link
Member

Detail that the costmap standalone node has changed, and yes, if any changes are needed to get the previous behavior, what those are

@tonynajjar tonynajjar mentioned this pull request Sep 24, 2022
Merged
@SteveMacenski SteveMacenski merged commit 7e181f4 into ros-navigation:main Sep 26, 2022
jwallace42 pushed a commit to jwallace42/navigation2 that referenced this pull request Dec 14, 2022
Add new constructor to cosmtap2DROS (ros-navigation#3222)
c1f837d 
* Add new constructor

* fix plugins

* Use new constructor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SteveMacenski SteveMacenski SteveMacenski 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.

costmap_2d_node: enable setting namespace from launch files
2 participants
@tonynajjar @SteveMacenski