Skip to content

Updating polygon parameter format#4020

Merged
SteveMacenski merged 10 commits intoros-navigation:mainfrom
ajsampathk:polygon_parameter_uniformisation
Dec 23, 2023
Merged

Updating polygon parameter format#4020
SteveMacenski merged 10 commits intoros-navigation:mainfrom
ajsampathk:polygon_parameter_uniformisation

Conversation

@ajsampathk
Copy link
Contributor

@ajsampathk ajsampathk commented Dec 20, 2023 •
edited
Loading

Basic Info

Info Please fill out this column
Ticket(s) this addresses (#3959)
Primary OS tested on (Ubuntu 22.04)
Robotic platform tested on (Gazebo Ignition)
Does this PR contain AI generated software? (No)

Description of contribution in a few bullet points

  • Changed the parameter description format polygons in nav2_collision_monitor to conform with the description of the footprint polygons in nav2_costmap_2d
  • Moved array_parser from nav2_costmap_2d to nav2_util
  • Added getPolygonFromString funtion in nav2_collision_monitor::Polygon namesapce to parse the Polygon's points parameter from string using the array parser
  • Moved the test for array_parser from nav2_costmap_2d to nav_util and renamed to test_array_parser for consistency.

Description of documentation updates required from your changes

Future work that may be required in bullet points

  • Might be worth looking at moving both the new getPolygonFromString function in nav2_collision_monitor and makeFootprintFromString function in nav2_costmap_2d to nav2_util/array_parser

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

@ajsampathk ajsampathk changed the title moving libraries and using parser in collision monitor Updating polygon parameter format Dec 20, 2023
SteveMacenski
SteveMacenski requested changes Dec 20, 2023
View reviewed changes
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 to me, thanks!!

SteveMacenski
SteveMacenski requested changes Dec 21, 2023
View reviewed changes
Copy link
Member

@SteveMacenski SteveMacenski left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Otherwise LGTM. If you make the doc updates I requested last time, I can merge the pair after CI passes

Thanks!

@ajsampathk
Copy link
Contributor Author

ajsampathk commented Dec 21, 2023

Hi @SteveMacenski , Thanks for the suggestions, I have applied them and updated the unit tests to follow the new format (Pending CI agrees).

However, there is one issue I am unsure of whether/how I should solve. Effectively, the array_parser that was moved from nav2_costmap_2d to nav2_util does not have a recognised licence, so the copyright linter fails in nav2_util.

This was "solved" in the costmap package by simply skipping the copyright check https://github.com/ros-planning/navigation2/blob/3992eb1e8663d312aeac685901b3283055aeeaeb/nav2_costmap_2d/CMakeLists.txt#L178

I'm not sure if nav2_util has a strict copyright requirement, but I will wait for your suggestion on this.

@SteveMacenski
Copy link
Member

SteveMacenski commented Dec 21, 2023

We actually do that in alot of projects - go ahead and add that here too

@SteveMacenski
Copy link
Member

SteveMacenski commented Dec 21, 2023

Why a draft PR still?

@ajsampathk
Copy link
Contributor Author

ajsampathk commented Dec 21, 2023

Just waiting till I properly test it. Should be open with the doc PR soon.

@ajsampathk ajsampathk mentioned this pull request Dec 22, 2023
Merged
@ajsampathk ajsampathk marked this pull request as ready for review December 22, 2023 11:57
@ajsampathk
Copy link
Contributor Author

ajsampathk commented Dec 22, 2023

Hi @SteveMacenski, Thanks for all the suggestions, the changes are now ready. along with the documentation update. Please let me know if I have missed anything.

But, I am very new to the coverage CI and am not sure how to get past it as it rightfully complains about files that we have moved.
Would appreciate any tips you might have about this, apart from that all tests are passing and I have also run a sanity check in simulation.

SteveMacenski
SteveMacenski approved these changes Dec 22, 2023
View reviewed changes
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.

The docs PR needs a little work, but otherwise this is good to ship once that's done!

@SteveMacenski
Copy link
Member

SteveMacenski commented Dec 22, 2023 •
edited
Loading

@ajsampathk there are 3 instances of https://github.com/ros-planning/navigation2/blob/main/.circleci/config.yml#L36 v17 in this file. Please update them all to v18. CI is failing due to the ABI change of moving the file and I want to make sure its just a caching problem that'll be resolved with a clear build before merging. The CI failure is unusual.

@ajsampathk
Copy link
Contributor Author

ajsampathk commented Dec 22, 2023

Updated it. 🤞🏽

@codecov
Copy link

codecov bot commented Dec 22, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d04de4e) 90.10% compared to head (babc0f6) 90.39%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4020      +/-   ##
==========================================
+ Coverage   90.10%   90.39%   +0.29%     
==========================================
  Files         415      415              
  Lines       18586    18593       +7     
==========================================
+ Hits        16747    16808      +61     
+ Misses       1839     1785      -54

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ajsampathk ajsampathk force-pushed the polygon_parameter_uniformisation branch from c9b883f to babc0f6 Compare December 23, 2023 00:11
@SteveMacenski SteveMacenski merged commit e2c8555 into ros-navigation:main Dec 23, 2023
@doisyg
Copy link
Contributor

doisyg commented Dec 23, 2023

Good job @ajsampathk ! (and fyi @kaichie)

@ajsampathk ajsampathk deleted the polygon_parameter_uniformisation branch December 23, 2023 14:39
jwallace42 pushed a commit to jwallace42/navigation2 that referenced this pull request Jan 3, 2024
@ajsampathk @jwallace42
Updating polygon parameter format (ros-navigation#4020)
dbb631e 
* moving libraries and using parser in collision monitor

* apply changes from review

* linting missed update

* unit test update

* collision_monitor utests + uncrustify lint

* skip copyright linting

* Combine error messages

* update yaml

* circleci update v18

* add invalid points test for cov

Signed-off-by: gg <josho.wallace@gmail.com>
enricosutera pushed a commit to enricosutera/navigation2 that referenced this pull request May 19, 2024
@ajsampathk @enricosutera
Updating polygon parameter format (ros-navigation#4020)
b5fe599 
* moving libraries and using parser in collision monitor

* apply changes from review

* linting missed update

* unit test update

* collision_monitor utests + uncrustify lint

* skip copyright linting

* Combine error messages

* update yaml

* circleci update v18

* add invalid points test for cov

Signed-off-by: enricosutera <enricosutera@outlook.com>
Marc-Morcos pushed a commit to Marc-Morcos/navigation2 that referenced this pull request Jul 4, 2024
@ajsampathk @Marc-Morcos
Updating polygon parameter format (ros-navigation#4020)
be276aa 
* moving libraries and using parser in collision monitor

* apply changes from review

* linting missed update

* unit test update

* collision_monitor utests + uncrustify lint

* skip copyright linting

* Combine error messages

* update yaml

* circleci update v18

* add invalid points test for cov
mini-1235 pushed a commit to mini-1235/navigation2 that referenced this pull request Feb 5, 2025
@ajsampathk
Docs for ros-navigation#4020 (ros-navigation#502)
544749e 
* update docs for polygon points

* migration guide update

* Hyperlink miss

* code block miss

* minor linting

* Title * update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SteveMacenski SteveMacenski SteveMacenski approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ajsampathk @SteveMacenski @doisyg