-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Full-area footprint check #5416
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Full-area footprint check #5416
Conversation
Signed-off-by: Tony Najjar <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
@tonynajjar, your PR has failed to build. Please check CI outputs and resolve issues. |
Signed-off-by: Tony Najjar <[email protected]>
@tonynajjar, your PR has failed to build. Please check CI outputs and resolve issues. |
for (const auto & point : footprint) { | ||
unsigned int mx, my; | ||
if (!worldToMap(point.x, point.y, mx, my)) { | ||
return static_cast<double>(NO_INFORMATION); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note to self, should be consistent with the one above (LETHAL_OBSTACLE)
Signed-off-by: Tony Najjar <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
@tonynajjar, your PR has failed to build. Please check CI outputs and resolve issues. |
@SteveMacenski I replaced the opencv with a custom implementation - I compared them using a performance test (included) and they were similar. |
Signed-off-by: Tony Najjar <[email protected]>
@tonynajjar, your PR has failed to build. Please check CI outputs and resolve issues. |
I will take a look at this tomorrow probably - but can you highlight the metrics you see different? How much 'worse' is this? |
Signed-off-by: Tony Najjar <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
This is the output of the performance test for this PR, it should be self-explanatory:
I also compared the perimeter-only check before and after the changes and no degradation of performance (modified the test here for perimeter-only) Before
After
|
So this look reasonable from the initial review perspective. Is there a good way to parameterize this for those that want it + only do the checks when its important to do so (i.e. max cost is INFLATED, else we're not near an obstacle anyway to require it). That should give you speed ups back to normal collision checking when possible. I think this is good to keep in our pocket as a working example and everything else is optimizations to see if we can do better. For example, doing the swept area from interpolated perimeter footprints instead -- which in my head should be better in terms of compute because we should be checking less cells and accounts for the outer-most edge's sweeping between search jumps in case it 'knicks'. That is what https://github.com/ros-navigation/navigation2/tree/h4pvbk-codex/add-optimized-collision-checking-method starts (I know you know; for others reading / posterity). I don't love how heavy this is and doesn't solve an element of your problem with jumps while rotating / turning where a long robot can have is corners overlap that isn't caught if its not in a parent or child polygon. But, I'm also not someone that blocks 'better/good' in the name of 'perfect' |
Got it 👍 Counter then is to do what we do in MPPI/Smac and find the cost in which anything is possibly circumscribed and use that rather than
I think this uses the IsPathValid service, if so, I think the full collision checking should be a field of that as well possibly. |
No it uses |
Ah, then
|
Basic Info
Description of contribution in a few bullet points
Description of documentation updates required from your changes
Description of how this change was tested
Future work that may be required in bullet points
For Maintainers:
backport-*
.