Skip to content

Conversation

@doisyg
Copy link
Contributor

@doisyg doisyg commented Dec 1, 2025


Basic Info

Info Please fill out this column
Ticket(s) this addresses (add tickets here #1)
Primary OS tested on Ubuntu
Robotic platform tested on Dexory bot
Does this PR contain AI generated software? No
Was this PR description generated by AI software? No

Description of contribution in a few bullet points

Lots to discuss on this one ;)
This PR introduces a new goal checker: the AxisGoalChecker (open to a better name). Especially suited for line paths.

It validates a goal based on the distance of the robot to the goal projected on the axis formed by the last 2 poses of the path. Controlled by segment_axis_goal_tolerance.
Hence ignoring how far the robot is from the path.

A second parameter is_overshoot_valid controls if a goal should be validated if the pose of the robot is "past" the goal by segment_axis_goal_tolerance (useful, for instance, in presence of a little localization jumps with very tight segment_axis_goal_tolerance setup).
In case of a path with only 1 pose, this goal checker falls back to a pure distance check.

As this goal checker needs the 2 last poses of the path to compute its final axis, the GoalChecker::isGoalReached API needed to be updated to optionally (if available) pass the pose before the last pose. This is probably the most important part to discuss (and the naming of that before last pose).

Description of documentation updates required from your changes

nav2 docs updates needed

Description of how this change was tested

Multiple days running straight paths ("segments") in a huge warehouses. And then rebased on main.

Future work that may be required in bullet points

Maybe exposing a segment_axis_ortho_goal_tolerance, and discussing naming.

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
  • Should this be backported to current distributions? If so, tag with backport-*.

Guillaume Doisy added 3 commits December 1, 2025 00:07
Signed-off-by: Guillaume Doisy <guillaume@dexory.com>
Signed-off-by: Guillaume Doisy <guillaume@dexory.com>
Signed-off-by: Guillaume Doisy <guillaume@dexory.com>
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.

Beyond anything else; I think this configuration guide entry will need a graphic to explain the value / when it should be used. I don't think this is immediately clear and could be easily misunderstood, so I think that element more than most is key.

I agree on naming. How about (and maybe not any better):

  • segment_axis_goal_tolerance -> Why not just leave as goal_tolerance or whatever it is now? It just has a different meaning for this critic (i.e. computed differently)
  • AxisGoalChecker -> PathApproachAngleGoalChecker?


nav2::declare_parameter_if_not_declared(
node,
plugin_name + ".segment_axis_goal_tolerance", rclcpp::ParameterValue(0.25));
Copy link
Member

Choose a reason for hiding this comment

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

Usual bits on the migration / configuration guide

Guillaume Doisy added 4 commits December 2, 2025 09:48
Signed-off-by: Guillaume Doisy <guillaume@dexory.com>
Signed-off-by: Guillaume Doisy <guillaume@dexory.com>
Signed-off-by: Guillaume Doisy <guillaume@dexory.com>
@doisyg
Copy link
Contributor Author

doisyg commented Dec 2, 2025

segment_axis_goal_tolerance -> Why not just leave as goal_tolerance or whatever it is now? It just has a different meaning for this critic (i.e. computed differently)

Renamed

AxisGoalChecker -> PathApproachAngleGoalChecker?

Still not sure about that one, I will get started with the documentation and see if I get more inspired (I still want to reflect that we check the pregression along an axis or line)

@SteveMacenski
Copy link
Member

Also: maybe instead of giving it the second-to-last point and modifying the API for just that information, how about giving the path in general to the goal checker so that it can use the full path instead (and this particular one just uses the second to last). It also pushes the changes into the specific new plugin rather than the core server - which I like from a separation of concerns perspective.

... I think there's a PR somewhere that actually was making that change some time ago for the goal checker to check on the path length as part of goal checking to deal with path looping. So this is very much aligned. #5446

@SteveMacenski
Copy link
Member

PS @mini-1235

@doisyg
Copy link
Contributor Author

doisyg commented Dec 2, 2025

Also: maybe instead of giving it the second-to-last point and modifying the API for just that information, how about giving the path in general to the goal checker so that it can use the full path instead (and this particular one just uses the second to last). It also pushes the changes into the specific new plugin rather than the core server - which I like from a separation of concerns perspective.

Oh that would be much better indeed !

... I think there's a PR somewhere that actually was making that change some time ago for the goal checker to check on the path length as part of goal checking to deal with path looping. So this is very much aligned. #5446

Nice! Let me know the progress. And I will pick up this PR once #5446 will be done then. Putting it to draft in the meantime. Also because there may be some trigonometry edge cases.

@doisyg doisyg marked this pull request as draft December 2, 2025 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants