-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix: Add PushRosNamespace action to navigation launch file and update… #5300
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
base: main
Are you sure you want to change the base?
Conversation
498d14d
to
ee67675
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅ see 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
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.
Isn't this already applied from
PushROSNamespace(namespace), |
Please clarify what is not currently working using bringup_launch.py
Thank you @SteveMacenski for the clarification and for pointing that out! You're absolutely right From a modularity and reusability perspective, I believe that each launch file, especially core ones like For example, using
These kinds of issues can be ambiguous and hard to trace, especially for users who aren’t aware that Additionally, having the YAML namespace configuration inside I hope this explains the motivation behind the proposed change. Let me know what you think, I’m happy to adjust the approach if needed. |
Ah, you should then push the namespace in the launch file that is launching We want that namespace push there so it applies to all of the launch files of a Nav2 bringup, not just navigation. Moving that to navigation would mean also moving that into the localization, keepout, and speed zone files, creating alot of redundancy. I don't understand what you mean by 'namespace aware' all of these launch files are aware of the namespaces and have them as launch arguments for remapping yaml files. Overall, I think this is not something we would merge. But if we did, you'd have to update all the other launch files since your current PR breaks everyone else by applying double namespacing in navigation 😉 |
… comments for parameter namespacing Signed-off-by: orebai <[email protected]>
Signed-off-by: orebai <[email protected]>
…rity Signed-off-by: orebai <[email protected]>
…er namespacing Signed-off-by: orebai <[email protected]>
Thank you @SteveMacenski for the detailed clarification! To clarify what I meant by “namespace aware”: while it's true that the current launch files accept a For example, during debugging, we sometimes need to launch each launch file in a separate terminal window to easily isolate and read debug messages or errors. In these cases, passing the That’s the motivation behind updating all relevant launch files to explicitly push the namespace, not to break the central approach of Of course, I fully understand the concern about redundancy. The updated version of the PR takes that into account by consistently applying Let me know your thoughts, I’m happy to align further with the project's direction if needed! |
@@ -89,6 +89,7 @@ def generate_launch_description() -> LaunchDescription: | |||
# Nodes launching commands | |||
start_map_server = GroupAction( | |||
actions=[ | |||
PushRosNamespace(namespace), |
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.
Missing start_slam_toolbox_cmd
Pull Request: Fix Namespacing in
navigation_launch.py
Basic Info
Description of contribution in a few bullet points
PushRosNamespace
action tonavigation_launch.py
to ensure proper namespacing behavior.Description of documentation updates required from your changes
Description of how this change was tested
Future work that may be required in bullet points
nav2_bringup
package.For Maintainers:
backport-*
.