[RPP] Cap collision check distance (regression from #5598)

[EricoMeger]

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	Resolves #5595, Follow-up to #5598
Primary OS tested on	Ubuntu 24.04
Robotic platform tested on	Not fully tested due to local environment issues
Does this PR contain AI generated software?	No
Was this PR description generated by AI software?	Out of respect for maintainers, AI for human-to-human communications are banned

---

Description of contribution in a few bullet points

• simulation_distance is now capped by max_lookahead
• warns the user if min_distance_to_obstacle > max_lookahead_dist

Description of documentation updates required from your changes

None

Description of how this change was tested

I was unable to complete a full navigation test due to some local environment setup issues. However, I did validate that the code compiles successfully and that the new warning message is correctly printed to the console when using a configuration where min_distance_to_obstacle > max_lookahead_dist.
I'm opening this PR now to speed up the review process. I will try to perform a full navigation test on my physical robot tomorrow and report back.

---

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

[EricoMeger]

@SteveMacenski @doisyg @mini-1235
Could you please take a look when you have a moment?

As mentioned in the PR description, I wasn't able to perform a full navigation test yet, but the logic should now be correct. I will be doing a full test tomorrow and will report back.

@mini-1235 , would you be able to pull this branch and check if it resolves your issue?
Thanks for your patience, and my apologies for the oversight.

[doisyg]

Trying to catch up on all, but this and the recent RPP changes breaks the behavior we are using at Dexory. I guess mainly because of:

• Change isCollisionImminent logic to always cover at least up to min_distance_to_obstacle, even when carrot_dist shrinks.

We are using a huge min_distance_to_obstacle (e.g. 2.5 m) but we still want to be able to reach goals closer to obstacle than min_distance_to_obstacle, hence we need the simulation distance to be capped by the carrot_dist (that never goes past the goal).
And yes, for it to work well, we use velocity_scaled_lookahead: false .

But I guess our use-case was not really explicit. So I am fine with the current changes (conceptually). We will just have to find a clean (optional with parameter) way to not enforce min_distance_to_obstacle at the goal.

[mini-1235]

Yes, it solves my issue in #5598 (comment)

[EricoMeger]

@doisyg, thanks for the review.

The "creeping" behavior my initial PR was trying to fix was only observed when velocity_scaled_lookahead was true, so that's another detail I overlooked. I will update the PR to make the new logic conditional based on whether use_velocity_scaled_lookahead_dist is being used.

About this part:

We are using a huge min_distance_to_obstacle (e.g. 2.5 m) but we still want to be able to reach goals closer to obstacle than min_distance_to_obstacle, hence we need the simulation distance to be capped by the carrot_dist (that never goes past the goal).

I'm still trying to fully understand your scenario. In my tests, I have goals at the end of corridors where the robot needs to get within ~40cm of a wall, and it works fine even with min_distance_to_obstacle set to 0.65m.

Would you mind explaining your setup a bit more? I want to make sure this PR doesn't introduce any other regressions.

I'll wait for your feedback before pushing anything. Thanks.

[doisyg]

@EricoMeger thanks for your effort and sorry I am a bit brief, I lack the bandwidth atm.

I will update the PR to make the new logic conditional based on whether use_velocity_scaled_lookahead_dist is being used.

I am fine with this logic going into a future PR, and finalizing the current one without any extra logic asap so we don't leave @mini-1235 with a broken behavior.

[EricoMeger]

Got it. Thanks for the clarification, @doisyg.

@SteveMacenski, a process question for you:

Given that my initial PR introduced a regression, and this new fix feels a bit rushed, would it be cleaner to revert #5598 first? Reverting would allow me to develop a more complete PR, now that I have a better understanding of the use cases. My only concern is creating extra work for @mini-1235, since he would need to resync.

I don't have much experience with large projects like this, so I'm not sure what the best practice is. I'm happy to proceed either way. Just looking for guidance.

[mini-1235]

My only concern is creating extra work for @mini-1235, since he would need to resync.

I am OK with both, this is not blocking my work in #5446, just something I noticed when testing

[SteveMacenski]

I think the indentations are wrong here, it should just be 2 spaces more than the simulation_distance_limit = line.

I don't know what's going on with CI / linting tools and why its not picking up on these issues right now :(

[EricoMeger]

That was from ament_uncrustify --reformat, I'll fix it manually on the new PR then

[SteveMacenski]

OK we can revert that PR (#5620) to be addressed fully with these comments in mind. Thanks @EricoMeger !

Should this be closed then?

[EricoMeger]

Agreed. Thanks for the guidance.

[SteveMacenski]

Thanks for your help in all of this!