Add footprint expansion to graceful controller#5779
Add footprint expansion to graceful controller#5779SteveMacenski merged 16 commits intoros-navigation:mainfrom
Conversation
7b2b2c7 to
0dd0cfc
Compare
Codecov Report❌ Patch coverage is
... and 20 files with indirect coverage changes 🚀 New features to boost your workflow:
|
e5cdded to
e7ddd44
Compare
c3196a8 to
437f71f
Compare
|
It looks like this has some severe conflicts that need to be adjusted before I can review. The GitHub UX won't even let me see the conflicts. You may be better off reopening on a new branch perhaps (but check first that its not easy) |
Signed-off-by: Tatsuro Sakaguchi <tatsuro.sakaguchi@g.softbank.co.jp>
Signed-off-by: Tatsuro Sakaguchi <tatsuro.sakaguchi@g.softbank.co.jp>
Signed-off-by: Tatsuro Sakaguchi <tatsuro.sakaguchi@g.softbank.co.jp>
Signed-off-by: Tatsuro Sakaguchi <tatsuro.sakaguchi@g.softbank.co.jp>
Signed-off-by: Tatsuro Sakaguchi <tatsuro.sakaguchi@g.softbank.co.jp>
Signed-off-by: Tatsuro Sakaguchi <tatsuro.sakaguchi@g.softbank.co.jp>
437f71f to
b7089c8
Compare
|
@SteveMacenski It seemed to conflict test codes with #5446, so I rebased and fixed it. |
|
@mikeferguson would you be open to review this PR for footprint expansion based on velocity in the graceful controller? |
SteveMacenski
left a comment
There was a problem hiding this comment.
Overall, looks straight forward and solid to me from a first look!
|
@Tacha-S sorry for the delay - small requests are made :- ) |
Signed-off-by: Tatsuro Sakaguchi <tatsuro.sakaguchi@g.softbank.co.jp>
Signed-off-by: Tatsuro Sakaguchi <tatsuro.sakaguchi@g.softbank.co.jp>
SteveMacenski
left a comment
There was a problem hiding this comment.
I think what's left is documentation. The new parameters need to be included in the graceful controller's configuration guide + entry into the migration guide to highlight this new feature!
| params_.footprint_scaling_linear_vel = node->declare_or_get_parameter( | ||
| plugin_name_ + ".footprint_scaling_linear_vel", 0.5); | ||
| params_.footprint_scaling_factor = node->declare_or_get_parameter( | ||
| plugin_name_ + ".footprint_scaling_factor", 0.0); |
There was a problem hiding this comment.
Should we set this as default behavior on? If so, what is a reasonable value to set this as?
There was a problem hiding this comment.
Yes, I think it would be fine to enable it by default.
When I tested it, I used a value of 1.0,
which allows the footprint to expand up to twice its original size.
This may depend on the robot’s size and speed, but in general, the footprint_scaling_factor of 1.0 seems reasonable as a default value.
There was a problem hiding this comment.
Twice the size seems a bit much, no? Twice the size over what velocity - I guess the scale of speed is relevant.
There was a problem hiding this comment.
That’s a fair point.
If the goal is to account for positional instability at higher speeds, then expanding the footprint up to twice its size even at maximum velocity may indeed be excessive.
I would also agree with a configuration where most robots only need an expansion of up to about 50%, or even less (around 20–30%), to be sufficiently safe.
For reference, my initial setting was based on the example shown toward the end of the parameter list in the graceful_controller README.
There was a problem hiding this comment.
Lets do 25%, I think that's a reasonable trade off by default!
Thanks for iterating. I think update the defaults to use this + in the configuration guide mention that 1.0 means double (and that this can be thought of as the % increase w.r.t. full velocity). That's useful user tuning context to share
| double target_yaw = tf2::getYaw(transformed_plan.poses.back().pose.orientation); | ||
| if ( | ||
| std::fabs(angles::shortest_angular_distance(path_heading_at_goal, target_yaw)) <= | ||
| params_->final_rotation_tolerance) |
There was a problem hiding this comment.
Is final orientation tolerance just the tolerance for the goal orientation? If so, then we should use the value from the goal checker.
Can you explain this block of code and why its useful here for documentation?
Is this in another controller(s)?
There was a problem hiding this comment.
This is slightly different from the goal-reaching condition.
The final_rotation_tolerance represents the allowable angular difference used to decide whether the approach to the goal can be replaced with a straight-line trajectory.
This is useful for preventing the local planner from independently deciding to move straight toward the goal even when an avoidance path has already been generated near the goal.
In other words, final_rotation_tolerance helps ensure that the planner respects the computed avoidance behavior in the vicinity of the goal rather than oversimplifying the motion.
There was a problem hiding this comment.
Its common though for prefer_final_rotation to be used with algorithms like NavFn. These have the path to the goal and then the goal orientation itself be discontinuous to the path. It just drives to the goal using gradient descent and then sets the goal orientation at the goal pose for an in-place rotation or DWB/MPPI/Graceful to deal with the details. This would make it so that if the approach orientation vs goal orientation is too high, it bypasses the feature -- which I think are decoupled problems/features.
The prefer_final_rotation is the parameter to make it so that the robot will drive directly to the pose and then rotate in place to the final orientation smoothly rather than try to bake it into the curve. I think whether the orientation is set to be continuous from the path leading up to it or discontinuous, the intent of the parameter is to go directly to the pose regardless of the difference.
Is this an issue you were noticing with shortcutting corners at the end of the path? I think that is a consequence of setting prefer_final_rotation = true.
I agree that I would like to find a way to potentially bypass it in the case that we're really shortcutting a corner, I'm just not sure that this is the right way. I suppose we could compare on approach to the goal the path generated from the direct-approach vs he spiral curve and use some comparison function to decide which to use in that situation. For example, looking at length vs cost, or checking if the cost is very close to max cost but the curve one is much safer. Then, we use the safer one for that iteration. Then once we get past the corner, we would be able to go back to the straight line behavior. This would involve some surgery but would be relatively easy for me to review.
If you agree and think that we can do this as a follow up PR, I think this would make sense to be something we chat about (and a very good next contribution!) and implement separate of this work so we can get this PR in. I think its very useful and I know I've been leaving you hanging for some time -- so I want to give you a win 😄 I dont believe (?) this is critical for the footprint expansion part of the work, right?
There was a problem hiding this comment.
I agree.
For this PR, it makes sense to focus only on the footprint expansion and keep the scope limited.
I’ve reverted the changes related to shortcutting behavior, and I’ve also updated the default value of the scaling factor.
For the shortcutting behavior, I’ll follow up with a separate PR and propose an improvement along the lines you suggested (e.g., comparing direct-approach vs curved paths near the goal and selecting the safer option when appropriate).
This reverts commit d3b7ca1. Signed-off-by: Tatsuro Sakaguchi <tatsuro.sakaguchi@g.softbank.co.jp>
This reverts commit b7089c8. Signed-off-by: Tatsuro Sakaguchi <tatsuro.sakaguchi@g.softbank.co.jp>
This reverts commit 75578aa. Signed-off-by: Tatsuro Sakaguchi <tatsuro.sakaguchi@g.softbank.co.jp>
This reverts commit d6977d2. Signed-off-by: Tatsuro Sakaguchi <tatsuro.sakaguchi@g.softbank.co.jp>
Signed-off-by: Tatsuro Sakaguchi <tatsuro.sakaguchi@g.softbank.co.jp>
Signed-off-by: Tatsuro Sakaguchi <tatsuro.sakaguchi@g.softbank.co.jp>
SteveMacenski
left a comment
There was a problem hiding this comment.
I think just now the docs for the new parameters and migration guide to reference this new feature and we're good to go!
|
@Tacha-S pull in main to make CI pass. I want to check the code coverage metrics before merging |
…xpansion-main Signed-off-by: Tatsuro Sakaguchi <tatsuro.sakaguchi@g.softbank.co.jp>
Signed-off-by: Tatsuro Sakaguchi <tatsuro.sakaguchi@g.softbank.co.jp>
|
Thanks so much for this feature addition, I'm sure it'll be useful for many Nav2 users! Any interest in other improvements to Graceful from the ticket (or otherwise) or to other parts of the stack? I'd be happy to help find other places to contribute :-) |
Basic Info
Description of contribution in a few bullet points
final_rotation_toleranceto theprefer_final_rotationfeature.Description of documentation updates required from your changes
Description of how this change was tested
I was tested in a real environment that included open spaces, narrow corridors, and corners.
Future work that may be required in bullet points
For Maintainers:
backport-*.