Updating polygon parameter format #4020

ajsampathk · 2023-12-20T17:37:43Z

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

Would need to update the Polygon Parameters points description to with an example similar to Costmap 2D parameter footprint

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

SteveMacenski

Generally looks good to me, thanks!!

nav2_collision_monitor/src/polygon.cpp

SteveMacenski

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

Thanks!

nav2_collision_monitor/src/polygon.cpp

ajsampathk · 2023-12-21T17:42:10Z

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 · 2023-12-21T19:56:47Z

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

SteveMacenski · 2023-12-21T21:15:10Z

Why a draft PR still?

ajsampathk · 2023-12-21T21:17:44Z

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

ajsampathk · 2023-12-22T12:08:03Z

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

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

SteveMacenski · 2023-12-22T20:54:42Z

@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 · 2023-12-22T20:59:58Z

Updated it. 🤞🏽

codecov · 2023-12-22T22:36:53Z

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.

doisyg · 2023-12-23T14:21:02Z

Good job @ajsampathk ! (and fyi @kaichie)

* 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>

* 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>

* 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

ajsampathk changed the title ~~moving libraries and using parser in collision monitor~~ Updating polygon parameter format Dec 20, 2023

SteveMacenski requested changes Dec 20, 2023

View reviewed changes

nav2_collision_monitor/src/polygon.cpp Show resolved Hide resolved

nav2_collision_monitor/src/polygon.cpp Outdated Show resolved Hide resolved

SteveMacenski requested changes Dec 21, 2023

View reviewed changes

nav2_collision_monitor/src/polygon.cpp Outdated Show resolved Hide resolved

ajsampathk mentioned this pull request Dec 22, 2023

Docs for #4020 ros-navigation/docs.nav2.org#502

Merged

ajsampathk marked this pull request as ready for review December 22, 2023 11:57

SteveMacenski approved these changes Dec 22, 2023

View reviewed changes

ajsampathk added 10 commits December 23, 2023 00:11

moving libraries and using parser in collision monitor

7f3d93c

apply changes from review

b019ef9

linting missed update

8120503

unit test update

3ed36db

collision_monitor utests + uncrustify lint

b49f5f4

skip copyright linting

30b5947

Combine error messages

9f0d699

update yaml

9ba06a4

circleci update v18

c894d40

add invalid points test for cov

babc0f6

ajsampathk force-pushed the polygon_parameter_uniformisation branch from c9b883f to babc0f6 Compare December 23, 2023 00:11

SteveMacenski merged commit e2c8555 into ros-navigation:main Dec 23, 2023
7 checks passed

ajsampathk deleted the polygon_parameter_uniformisation branch December 23, 2023 14:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating polygon parameter format #4020

Updating polygon parameter format #4020

ajsampathk commented Dec 20, 2023 •

edited

Loading

SteveMacenski left a comment

SteveMacenski left a comment •

edited

Loading

ajsampathk commented Dec 21, 2023

SteveMacenski commented Dec 21, 2023

SteveMacenski commented Dec 21, 2023

ajsampathk commented Dec 21, 2023

ajsampathk commented Dec 22, 2023

SteveMacenski left a comment

SteveMacenski commented Dec 22, 2023 •

edited

Loading

ajsampathk commented Dec 22, 2023

codecov bot commented Dec 22, 2023 •

edited

Loading

doisyg commented Dec 23, 2023

Updating polygon parameter format #4020

Updating polygon parameter format #4020

Conversation

ajsampathk commented Dec 20, 2023 • edited Loading

Basic Info

Description of contribution in a few bullet points

Description of documentation updates required from your changes

Future work that may be required in bullet points

For Maintainers:

SteveMacenski left a comment

Choose a reason for hiding this comment

SteveMacenski left a comment • edited Loading

Choose a reason for hiding this comment

ajsampathk commented Dec 21, 2023

SteveMacenski commented Dec 21, 2023

SteveMacenski commented Dec 21, 2023

ajsampathk commented Dec 21, 2023

ajsampathk commented Dec 22, 2023

SteveMacenski left a comment

Choose a reason for hiding this comment

SteveMacenski commented Dec 22, 2023 • edited Loading

ajsampathk commented Dec 22, 2023

codecov bot commented Dec 22, 2023 • edited Loading

Codecov Report

doisyg commented Dec 23, 2023

ajsampathk commented Dec 20, 2023 •

edited

Loading

SteveMacenski left a comment •

edited

Loading

SteveMacenski commented Dec 22, 2023 •

edited

Loading

codecov bot commented Dec 22, 2023 •

edited

Loading