Skip to content

Deprecate ParseMultiRobotPose with launch.actions.ForEach #5000

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
Merged
merged 3 commits into from
Mar 21, 2025
Merged

Deprecate ParseMultiRobotPose with launch.actions.ForEach #5000

merged 3 commits into from
Mar 21, 2025

Conversation

@leander-dsouza
Copy link
Contributor

@leander-dsouza leander-dsouza commented Mar 21, 2025

Basic Info

Info Please fill out this column
Ticket(s) this addresses #4990
Primary OS tested on Ubuntu
Robotic platform tested on Tested using the default devcontainer
Does this PR contain AI generated software? No

Description of contribution in a few bullet points

  • Modified multi-robot string to add a name and pose key for each robot.

    • From:

      robots:='robot1={x: 1.0, y: 1.0, yaw: 1.0}'

    • To:

      robots:='{name: 'robot1', pose: {x: 1.0, y: 1.0, yaw: 0.0}}'

    I have taken inspiration from the boilerplate code in #802.

  • Deprecated ParseMultiRobotPose throughout the codebase and have used launch.actions.ForEach instead.

  • Added a function to calculate the number of robots in the string to preserve the earlier behaviour.

Description of documentation updates required from your changes

  • The migration guide from Jazzy to Kilted needs to be updated for multi-robot launch file to use the updated robot string structure.

Description of how this change was tested

  • Ran the launch file on its own, and verified the two robots being executed.
  • Ran colcon test locally and found no errors.

Future work that may be required in bullet points

For Maintainers:

  • Check that any new parameters added are updated in docs.nav2.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
@leander-dsouza leander-dsouza marked this pull request as ready for review March 21, 2025 18:06
@leander-dsouza
Copy link
Contributor Author

leander-dsouza commented Mar 21, 2025

@SteveMacenski I am not sure why geometry_msgs is not getting imported correctly. I assume the cache restore is faulty.
@SteveMacenski
Copy link
Member

SteveMacenski commented Mar 21, 2025

@leander-dsouza are you on the latest version of the main branch? It might be that you're really out of date. If that doesn't work, there are 3 instances of v31 in this file https://github.com/ros-navigation/navigation2/blob/main/.circleci/config.yml, change them to v32 to break the cache
SteveMacenski
SteveMacenski requested changes Mar 21, 2025
View reviewed changes
Copy link
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

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

Thanks for much for this, I love using new features in ROS 2 😄
@SteveMacenski SteveMacenski linked an issue Mar 21, 2025 that may be closed by this pull request
Replace OpaqueFunction + ParseMultiRobotPose with launch.actions.ForEach #4990
Closed
christophebedard
christophebedard reviewed Mar 21, 2025
View reviewed changes
nav2_bringup/launch/cloned_multi_tb3_simulation_launch.py
import yaml


def count_robots(context: LaunchContext) -> list[LogInfo]:
Copy link

@christophebedard christophebedard Mar 21, 2025

Choose a reason for hiding this comment

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

It's kind of unfortunate that you have to do this and parse the config string yourself. What if the ForEach action itself logged the number of iterations?
Copy link
Member

@SteveMacenski SteveMacenski Mar 21, 2025

Choose a reason for hiding this comment

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

That would be great!
@leander-dsouza
Removed ParseMultiRobotPose and introduced ForEach instead.
8da1ce4 
Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>
@leander-dsouza
Deprecated ParseMultiRobotPose throughout the codebase.
40a9353 
Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>
@leander-dsouza
Bump version to break cache.
8ad7329 
Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>
SteveMacenski
SteveMacenski approved these changes Mar 21, 2025
View reviewed changes
Copy link
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

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

Waiting on CI!
@codecov
Copy link

codecov bot commented Mar 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

see 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
@SteveMacenski SteveMacenski merged commit 2cd166f into ros-navigation:main Mar 21, 2025
11 checks passed
This was referenced Mar 21, 2025
Merged
Merged
stevedanomodolor pushed a commit to stevedanomodolor/navigation2 that referenced this pull request Apr 29, 2025
@leander-dsouza @stevedanomodolor
Deprecate ParseMultiRobotPose with launch.actions.ForEach (ros-na…
56b8f91 
…vigation#5000)

* Removed ParseMultiRobotPose and introduced ForEach instead.

Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>

* Deprecated ParseMultiRobotPose throughout the codebase.

Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>

* Bump version to break cache.

Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>

---------

Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>
Signed-off-by: stevedanomodolor <stevedan.o.omodolor@gmail.com>
@coderwyvern coderwyvern mentioned this pull request May 1, 2025
Merged
7 tasks
@christophebedard christophebedard mentioned this pull request May 5, 2025
Open
SakshayMahna pushed a commit to SakshayMahna/navigation2 that referenced this pull request Jun 8, 2025
@leander-dsouza @SakshayMahna
Deprecate ParseMultiRobotPose with launch.actions.ForEach (ros-na…
043bc07 
…vigation#5000)

* Removed ParseMultiRobotPose and introduced ForEach instead.

Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>

* Deprecated ParseMultiRobotPose throughout the codebase.

Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>

* Bump version to break cache.

Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>

---------

Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>
Signed-off-by: Sakshay Mahna <sakshum19@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants

@leander-dsouza @SteveMacenski @christophebedard