Move projectState after getPointsInside

BriceRenaudeau commented 4 months ago

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	#4355
Primary OS tested on	Ubuntu
Robotic platform tested on	Our simulation
Does this PR contain AI generated software?	No

Description of contribution in a few bullet points

Move projectState after getPointsInside to compute time=0.0 collision
Change comment to fit returned time. time = collision not before collision

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

tonynajjar commented 4 months ago

Seems like there are a bunch of tests that fail now:

    [  FAILED  ] 3 tests, listed below:
    [  FAILED  ] Tester.testProcessApproachRotation
    [  FAILED  ] Tester.testCrossOver
    [  FAILED  ] Tester.testSourceTimeoutOverride

SteveMacenski commented 4 months ago

Ditto on tests + run this locally and please verify it removes the problem you were looking to address + doesn't break the general behavior of the system.

I trust tests, but something this low-level I want tested on robots when possible (and I hope you want that too :wink: )

BriceRenaudeau commented 4 months ago

I trust tests, but something this low-level I want tested on robots when possible (and I hope you want that too 😉 )

The test on robot went well.

The unit test failures come from the change in time. The modified method returns the collision time, not the time before the collision. I will make it keep the previous behavior.

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Files	Coverage Δ
nav2_collision_monitor/src/polygon.cpp	`97.16% <100.00%> (+0.02%)`	:arrow_up:

... and 13 files with indirect coverage changes

ros-navigation / navigation2