Fix SMAC smoother footprint check #6257
Conversation
Signed-off-by: Alyss <u7288133@anu.edu.au>
Signed-off-by: Alyss <u7288133@anu.edu.au>
Signed-off-by: Alyss <u7288133@anu.edu.au>
8fda9c0 to
e52f072
Compare
Codecov Report❌ Patch coverage is
... and 33 files with indirect coverage changes 🚀 New features to boost your workflow:
|
SteveMacenski
left a comment
There was a problem hiding this comment.
But good first go at it!
| const std::vector<geometry_msgs::msg::Point> & footprint = | ||
| std::vector<geometry_msgs::msg::Point>(), |
There was a problem hiding this comment.
There's no caller of this passing in the footprint using this new API.
ALso if footprint == std::vector<geometry_msgs::msg::Point>() then you know you're using radius, so the second argument is redundant. Just pass in footprint to smooth() when this API is used only when needed
| // validate update is admissible, only checks cost if a valid costmap pointer is provided | ||
| float cost = 0.0; | ||
| if (costmap) { | ||
| if (costmap && use_radius) { |
There was a problem hiding this comment.
Here and below: I think we both can agree that you may be able to do better to integrate this more cleanly.
If under if (costmap) you can then check if footprint == vector<Point>() to know whether to use single point or footprint cost checker more concisely. You can also store the FootprintCollisionChecker as a class member so you don't reconstruct I think.
You can also reorder calling updateApproximatePathOrientations before we try to collision check in general so its always just done for us instead of waiting til the end.
I think there's a few things you can do to make this higher quality and less redundant
Signed-off-by: Alyss <u7288133@anu.edu.au>
Signed-off-by: Alyss <u7288133@anu.edu.au>
Basic Info
#includeDescription of contribution in a few bullet points
(Note on scope proposed in the original issue: this pr does not try to redesign the smoother to retain "true" orientation information or enforce min-turning-radius feasibility during smoothing. The goal here is fixing the immediate false-negative collision case for non-circular footprints, preserving existing smoother structure.)
Code change:
nav2_smac_planner/include/nav2_smac_planner/smoother.hpp)Smoother::smooth()andSmoother::smoothImpl()to optionally acceptfootprintanduse_radius(whether bot is radius based or not)nav2_smac_planner/src/smoother.cpp): kept original centre-cell cost check for circular footprint; added a non circular footprint validation path ifcostmapis a valid pointer &&!use_radiusDescription of documentation updates required from your changes
No updates required. This is only bug fix.
Description of how this change was tested
The test isolates the smoother's own collision-validation behavior:
footprint_checker.footprintCostAtPose()to be safefalse.Future work that may be required in bullet points
no know follow up work
Test Details & Results
To visualize the footprint geometry a bit, r denotes the robot centre, `
To reproduce the regression test:
then (replace --packages-select with --packages-up-to nav2_smac_planner if dependencies not installed yet)
Does this fit with the original scope? Let me know if more changes are needed or if you would like to see additional testing / screenshot from the local run
For Maintainers:
backport-*.