feat(PlannerServer): BaseGlobalPlanner plugin accepts viapoints as part of API#5995
Conversation
2e5afd9 to
7833cb2
Compare
SteveMacenski
left a comment
There was a problem hiding this comment.
I think the behavior tree node is missing the new field using the action API update so that users with behavior trees can use this :-)
Otherwise, check out my comment on the ticket. I think this is good, but a contribution demonstrating use of this would be great and be a new capability that is missing (if you're open to it).
Sorry about the delay again.
| const geometry_msgs::msg::PoseStamped & start, | ||
| const geometry_msgs::msg::PoseStamped & goal, | ||
| const nav_msgs::msg::Goals & viapoints, | ||
| std::function<bool()> cancel_checker) |
There was a problem hiding this comment.
I think we should keep 1x version of this function and require that viapoints is given (even if empty)
There was a problem hiding this comment.
I would agree with that but that would necessitate any existing planners to update the function signature to be compatible. In this way existing planners don't need a change unless they intend to use waypoints.
I was thinking of adding it to a deprecation path of some sort and eventually moving to 1x version once people have had enough time to update the function signature. I am not sure what the protocol is for that with Nav2.
There was a problem hiding this comment.
I think this is easy enough, its just updating to add a field const nav_msgs::msg::Goals & /*viapoints*/, and its done. We cannot backport this anyway to another distribution due to this change at all in the header + the server, so we're free to do what we need to do for code cleanliness.
There was a problem hiding this comment.
That's perfect... I'll update it accordingly and check out the BT API
|
@sbadamikar-research any update? The Lyrical freeze is coming up here in a few weeks and I want to get this included! |
|
@SteveMacenski I've been moving cities for the past couple weeks and haven't been as active as I'd like to be. I should have something by EOW |
fb0861c to
633be13
Compare
|
Let me know when you want me to take a look! Also please rebase / pull in main when you have a chance, we did some things to rebalance our CI jobs |
633be13 to
d1217f7
Compare
|
@SteveMacenski The branch is rebased. Please have a look at your convenience. I have another branch where I am working on the planners that I will create a draft PR for you to have a look and provide feedback. |
|
This is good timing, the Lyrical freeze will be in a couple of weeks so we want to get this in ASAP so its in Lyrical, else it would not be backportable after we run the first release due to the API breaking changes |
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 12 files with indirect coverage changes 🚀 New features to boost your workflow:
|
…rt of API The ComputePlanToPose action now incorporates a list of stamped poses as intermediate viapoints that are forwarded to the planner plugin. Signed-off-by: Sanchit B <researchbadamikar@gmail.com>
Signed-off-by: Sanchit B <researchbadamikar@gmail.com>
Pass non-empty viapoints vectors in planner unit tests to exercise the RCLCPP_WARN branch in createPlan() for planners that ignore viapoints (smac_2d, smac_hybrid, smac_lattice, theta_star, navfn). Signed-off-by: Sanchit B <researchbadamikar@gmail.com>
d1217f7 to
aca3e5d
Compare
|
Docs PR is all that's left |
…rt of API (ros-navigation#5995) * feat(PlannerServer): BaseGlobalPlanner plugin accepts viapoints as part of API The ComputePlanToPose action now incorporates a list of stamped poses as intermediate viapoints that are forwarded to the planner plugin. Signed-off-by: Sanchit B <researchbadamikar@gmail.com> * fix(planners): Improved logging of skipped viapoints and cleaned imports Signed-off-by: Sanchit B <researchbadamikar@gmail.com> * test(planners): Add test coverage for viapoint warning logs Pass non-empty viapoints vectors in planner unit tests to exercise the RCLCPP_WARN branch in createPlan() for planners that ignore viapoints (smac_2d, smac_hybrid, smac_lattice, theta_star, navfn). Signed-off-by: Sanchit B <researchbadamikar@gmail.com> --------- Signed-off-by: Sanchit B <researchbadamikar@gmail.com> Signed-off-by: Tony Najjar <tony.najjar@dexory.com>
…rt of API (ros-navigation#5995) * feat(PlannerServer): BaseGlobalPlanner plugin accepts viapoints as part of API The ComputePlanToPose action now incorporates a list of stamped poses as intermediate viapoints that are forwarded to the planner plugin. Signed-off-by: Sanchit B <researchbadamikar@gmail.com> * fix(planners): Improved logging of skipped viapoints and cleaned imports Signed-off-by: Sanchit B <researchbadamikar@gmail.com> * test(planners): Add test coverage for viapoint warning logs Pass non-empty viapoints vectors in planner unit tests to exercise the RCLCPP_WARN branch in createPlan() for planners that ignore viapoints (smac_2d, smac_hybrid, smac_lattice, theta_star, navfn). Signed-off-by: Sanchit B <researchbadamikar@gmail.com> --------- Signed-off-by: Sanchit B <researchbadamikar@gmail.com> Signed-off-by: Tony Najjar <tony.najjar@dexory.com>
The SmacPlannerHybrid and SmacPlannerLattice test files in this branch still called the 3-arg createPlan(start, goal, cancel_checker) signature. On main, BaseGlobalPlanner::createPlan gained a viapoints parameter (commit 51e3125, PR ros-navigation#5995), and main's test files were updated to the 4-arg form. Build Against Released Distributions CI fails on jazzy and kilted because this branch's test files diverged. Mirror main's pattern: declare a per-TEST std::vector<geometry_msgs::msg::PoseStamped> no_viapoints{} alongside dummy_cancel_checker, and pass it as the third argument to all 8 createPlan call sites in test_smac_hybrid.cpp and test_smac_lattice.cpp. No production-code changes. Smoother behaviour under test is unchanged. Signed-off-by: Akihiko Komada <aki1770@gmail.com>
The SmacPlannerHybrid and SmacPlannerLattice test files in this branch still called the 3-arg createPlan(start, goal, cancel_checker) signature. On main, BaseGlobalPlanner::createPlan gained a viapoints parameter (commit 51e3125, PR ros-navigation#5995), and main's test files were updated to the 4-arg form. Build Against Released Distributions CI fails on jazzy and kilted because this branch's test files diverged. Mirror main's pattern: declare a per-TEST std::vector<geometry_msgs::msg::PoseStamped> no_viapoints{} alongside dummy_cancel_checker, and pass it as the third argument to all 8 createPlan call sites in test_smac_hybrid.cpp and test_smac_lattice.cpp. No production-code changes. Smoother behaviour under test is unchanged. Signed-off-by: Akihiko Komada <aki1770@gmail.com>
Basic Info
Description of contribution in a few bullet points
Description of documentation updates required from your changes
Description of how this change was tested
Tests included:
Corresponding PRs:
ros-navigation/docs.nav2.org#904
ros-navigation/navigation2_tutorials#139
Future work that may be required in bullet points
For Maintainers:
backport-*.