Skip to content

CM/CD: Add polygon source#3885

Closed
tonynajjar wants to merge 38 commits intoros-navigation:mainfrom
logivations:add-polygon-source-main
Closed

CM/CD: Add polygon source#3885
tonynajjar wants to merge 38 commits intoros-navigation:mainfrom
logivations:add-polygon-source-main

Conversation

@tonynajjar
Copy link
Contributor

@tonynajjar tonynajjar commented Oct 16, 2023


Basic Info

Info Please fill out this column
Ticket(s) this addresses N/A
Primary OS tested on Ubuntu
Robotic platform tested on Custom gazebo simulation

Description of contribution in a few bullet points

We would like to add a source to the collision monitor and that is an array of arbitrary PolygonStamped fed from a topic. Our specific use case is that we would like to prevent collisions with some tracked objects.

Note: Adding these tracked objects to the global/local costmap was tried, however this was not enough as the controller makes a "best effort" to avoid the collisions; we need more decisive logic to stop the robot and therefore need the collision monitor

  • Add Polygon as a source
  • Create new msg type PolygonArray to feed in polygons to the Polygon source

Description of documentation updates required from your changes


Future work that may be required in bullet points

For Maintainers:

  • Check that any new parameters added are updated in navigation.ros.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 tonynajjar marked this pull request as draft October 16, 2023 13:47
@mergify
Copy link
Contributor

mergify bot commented Oct 16, 2023

@tonynajjar, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

Tony Najjar added 2 commits October 16, 2023 16:43
@tonynajjar
Copy link
Contributor Author

@SteveMacenski @AlexeyMerzlyakov that's a first draft, a lot of small improvements can still be made but wanted your opinion on the idea before spending more time refining it. Thanks

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.

Test coverage?

I'd think for polygon-source types, we would want to make the minimum number of points 1 for this special case

@tonynajjar tonynajjar marked this pull request as ready for review October 17, 2023 10:01
@tonynajjar
Copy link
Contributor Author

I thin most points have been addressed, please have another look.

@SteveMacenski
Copy link
Member

SteveMacenski commented Oct 30, 2023

@AlexeyMerzlyakov please re-review

@tonynajjar this file has the most missing coverage of the Collision Monitor files https://app.codecov.io/gh/ros-planning/navigation2/blob/add-polygon-source-main/nav2_collision_monitor%2Fsrc%2Fpolygon_source.cpp#L179 please fill in the gaps. I'm not worried about the node.lock() exceptions, but there is a complete function missing and a for loop unexecuted. This particular package more than all others needs to have really solid test coverage due to its nature. Add a few more things to really exercise and check it all, this one is pretty key

At this point, the rest of the code here is approved except the actual polygon_source hpp/cpp's implementation of the polygon deconfliction

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.

Generally looks good, but I don't think the are polygons similar approach is valid general approach to this. This assumes alot about what specifically these polygons are, how often they're published, and that they're spatially near each other due to that regular frequency

@SteveMacenski
Copy link
Member

@tonynajjar when you're back, rebase and fix the API changes for bool getData and checking the timeout via #3880 merging

Copy link
Collaborator

@AlexeyMerzlyakov AlexeyMerzlyakov left a comment

Choose a reason for hiding this comment

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

The main challenge here - is how to solve moving polygon task, where polygon changing its coordinates during the time.
Whether should we use unique_identifier_msgs/msg/UUID.msg (in newly added nav2_msgs message or by resolved ros2/common_interfaces#230 task); or get back to previous implementation when all polygons are being published once per a time, or might be something else (which I did not note).

@tonynajjar
Copy link
Contributor Author

Addressed most of the comments, I'll wait for the result of ros2/common_interfaces#230 to continue this PR and the tests. It's also my preferred solution to the problem

@mergify
Copy link
Contributor

mergify bot commented Nov 6, 2023

@tonynajjar, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

@SteveMacenski
Copy link
Member

@tonynajjar Polygons with IDs have been added! ros2/common_interfaces#232 (comment)

That can be used now to implement this PR to get over the issue :-)

@tonynajjar
Copy link
Contributor Author

tonynajjar commented Jan 24, 2024

Awesome! In the meantime I left my old company that needed this and am quite busy with my new one. @jplapp maybe you want to take over? If not I'll try to eventually find the time but can't promise when.

@SteveMacenski
Copy link
Member

It should be straight forward now if using those messages. All of the arePolygonsSimilar functionality can be deleted and just check the IDs. Add a few unit tests and a nice video and I think its done.

@jplapp
Copy link
Contributor

jplapp commented Feb 23, 2024

Thanks for review! Sure, I'll find someone to complete this, but cannot promise a timeline

@oldtopos
Copy link

oldtopos commented Mar 6, 2024

@jplapp I have cycles to pick this up; it seems there is one use of arePolygonsSimilar to replace, so this should be straightforward. LMK if that works for you

@oldtopos
Copy link

oldtopos commented Mar 7, 2024

I am attempting to copy the branch associated with the PR to my own fork of nav2 in github, as I presume (and have not tested) I do not have commit access to the repo currently associated with the merge. Will see how the build goes tomorrow, fingers crossed

@mergify
Copy link
Contributor

mergify bot commented Apr 23, 2024

This pull request is in conflict. Could you fix it @tonynajjar?

@SteveMacenski
Copy link
Member

@oldtopos did you happen to get a chance to play with this? It was largely done and it would be a shame to let this die on the vine if you'd get use from it

@mergify
Copy link
Contributor

mergify bot commented Apr 25, 2024

@tonynajjar, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

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.

6 participants