-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fix: Changed cloned_multi_tb3_simulation_launch.py file to conform with common ROS 2 launch syntax #4811
base: main
Are you sure you want to change the base?
Conversation
5e6024e
to
d843e30
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅ |
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'm not sure this is the best way to handle the problem. I think what makes more sense is to have the parsing nav2_common
utility be a proper launch substitution https://github.com/ros2/launch_ros/tree/rolling/launch_ros/launch_ros/substitutions. I think that would make this work better and be in the intended launch file's declarative style (rather than trying to get around it using opaque functions). If we're going to try to circumnavigate declarative programming, I think we might be better off using the current styling instead since its more clear to new developers and is equally as circumnavigated
I'll have a look at this. Thanks for the feedback |
dd71279
to
9b31d75
Compare
@TannerGilbert, your PR has failed to build. Please check CI outputs and resolve issues. |
9460f4c
to
a4f19d7
Compare
@TannerGilbert, your PR has failed to build. Please check CI outputs and resolve issues. |
@SteveMacenski sorry for the delay. I had a look at how to write a custom substitution class and tried to refactor the ParseMultiRobotPose accordingly. Still, as a dict is needed in the launchfile to the bringup_cmd_group list I wasn't fully able to remove the OpaqueFunction. Maybe my ROS knowledge is insufficient. I would be glad for any feedback and sorry for the inconvenience. |
OK - we can use the opaque function method! |
Please rebase / pull in main and that should make CI turn over. TF2 had some API changes we fixed over the weekend |
…mmon ROS 2 launch syntax and thereby allow for including it in other launch files as expected Signed-off-by: Tanner, Gilbert <[email protected]>
Signed-off-by: Tanner, Gilbert <[email protected]> chore: Changed formatting to conform to flake8 linting rules II Signed-off-by: Tanner, Gilbert <[email protected]> chore: Changed formatting to conform to flake8 linting rules III Signed-off-by: Tanner, Gilbert <[email protected]> chore: Changed formatting to conform to flake8 linting rules IV Signed-off-by: Tanner, Gilbert <[email protected]>
Signed-off-by: Tanner, Gilbert <[email protected]>
Signed-off-by: Tanner, Gilbert <[email protected]>
Signed-off-by: Tanner, Gilbert <[email protected]>
5e06fdf
to
9f4ef45
Compare
@TannerGilbert, your PR has failed to build. Please check CI outputs and resolve issues. |
…rgument_for_cloned_multi_tb3
I updated the PR based on the change request. Thanks for all the feedback and patience. |
Basic Info
Description of contribution in a few bullet points
Description of documentation updates required from your changes
None
Future work that may be required in bullet points
I would prefer that the headless and SLAM options are added to this script similar to how it's handled in the tb3_simulation_launch.py file
For Maintainers: