Added Line Iterator

[afifswaidan]

---

Basic Info

Info	Added Line Iterator
Ticket(s) this addresses	Issue number #3151
Primary OS tested on	Ubuntu 20.04.5 LTS
Robotic platform tested on	ROS2 Foxy

---

Description of contribution in a few bullet points

• Added Line Iterator Python API

---

Future work that may be required in bullet points

• I have few questions before I start implementing the python version of the footprint_collision_checker:
• The Cpp script uses footprint.hpp and several other modules like "nav2_costmap_2d/exceptions.hpp", should I implement all of these modules first in python (So I use them in the python version of footprint_collision_checker) or are they already implemented somewhere else?

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

[mergify[bot]]

@afifswaidan, please properly fill in PR template in the future. @stevemacenski, use this instead.

• [ ] 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]

How did you test this? A set of unit tests here would be good.

[afifswaidan]

Hello,

As the main purpose of this class is to do some basic mathematical computations, I tested it manually by giving some valid and invalid inputs and I print the outputs for validation.

If required, let me know what exact tests you have in mind so I can implement and commit them.

[SteveMacenski]

add some unit tests in /tests that can be run in our CI suite that check that this works properly. Give it a few different line segments at different angles and check that it visits every cell increment correctly

[afifswaidan]

Okay sure, I'll implement and push the changes today or tomorrow. Thanks!

[afifswaidan]

Hello @SteveMacenski I implemented a new way for the line iterator that contains more features and more precision.

Can you please have a look at it before I start implementing the unit tests for it? Maybe you want me to add/remove anything. Thanks!

[SteveMacenski]

Can you explain the method / benefits over the old?

CI shows this has linting errors

[afifswaidan]

As for the linting errors, I am developing on a new machine and apparently, no linter was installed, I'll move back to my old machine tomorrow.

As for the advantages:

• The user can input the resolution of the calculations (mainly the iterations)
• The user can get the equation of the line
• The user can get the characteristics of the line (slope m and y-intercept b) to solve for any x or y points. (I will add two methods called solve_for_x and solve_for_y where the user inputs x or y and the function solves it using the equation of the line.
• This method takes into consideration the cases where the equation includes only x (like, x=5)

[SteveMacenski]

I think the density of resolution is a good point - but that would need to be parameterized and by default just every 1-cell jumps. I think though someone could want to try 2x or 3x cell lengths to get cells that are barely touching but might technically be in collision. This comes as a high cost for longer range queries, but for footprints I think that's a useful feature.

If you run ament_pep257 and ament_flake8 you should be able to see the outputs locally to iterate

[afifswaidan]

I think the density of resolution is a good point - but that would need to be parameterized and by default just every 1-cell jumps. I think though someone could want to try 2x or 3x cell lengths to get cells that are barely touching but might technically be in collision. This comes as a high cost for longer range queries, but for footprints I think that's a useful feature.

If you run ament_pep257 and ament_flake8 you should be able to see the outputs locally to iterate

Okay I will make the resolution as a parameter that the user can define, and by default it'll be 1.

As for the linting I will do it as well, and then I will commit the changes for review, once everything is good I will implement a few test cases in the /test directory and commit it for the final review and merge.

Thank you so much @SteveMacenski

[mergify[bot]]

@afifswaidan, 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).

[afifswaidan]

Hello, I implemented the resolution as a parameter and handled the linting based on pep257 and flake8. Kindly check and give me your feedback. (I found a typo mistake in line 69 in the comments, I"ll fix it in the next commit after getting your feedback)

Thanks in advance @SteveMacenski

[SteveMacenski]

Make sure the tests execute during CI! You may need to add something to the setup.py/cfg files

[afifswaidan]

Make sure the tests execute during CI! You may need to add something to the setup.py/cfg files

Thanks for the info, I will try to do that asap this week. I'm sorry if the process is going slow but it's a busy period with my current full-time job.

I'll handle the CI tests and wait for your feedback. Thanks!

[SteveMacenski]

Totally understand! Thanks for talking me through the changes on my questions.

[afifswaidan]

Hello @SteveMacenski , I added all the unit tests and modified line_iterator.py to handle more exceptions that can be caused by the user. But I am facing an issue while importing the line_iterator.py module to the test script in /test directory. I am not facing this issue when the files are in the same directory. Can you please check it and give me your opinion?

Thanks!

[SteveMacenski]

https://github.com/ros2/geometry2/tree/rolling/tf2_ros_py Try using this Python3 package as a guide.

[afifswaidan]

https://github.com/ros2/geometry2/tree/rolling/tf2_ros_py Try using this Python3 package as a guide.

Hello, Thanks for the reference. But I noticed in the tests script they are using the "unittest" package. While in the setup.py it contains "tests_require=['pytest']" This is why I got confused and I implemented the tests using pytest instead. (Also the tf2_ros_py repo contains pytest.ini file but they didn't use the pytest package in their test scripts) Also in package.xml (<test_depend>python3-pytest</test_depend>)

I will implement the using "unittest" as you suggested based on tf2_ros_repo. But I just wanted to ask you about this because it got me a bit confused. Thanks @SteveMacenski

[codecov[bot]]

Codecov Report

Base: 88.97% // Head: 88.58% // Decreases project coverage by -0.38% :warning:

Coverage data is based on head (a571cde) compared to base (4bd3f73). Patch has no changes to coverable lines.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3197      +/-   ##
==========================================
- Coverage   88.97%   88.58%   -0.39%     
==========================================
  Files         351      351              
  Lines       15658    15658              
==========================================
- Hits        13932    13871      -61     
- Misses       1726     1787      +61     

Impacted Files	Coverage Δ
...av2_dwb_controller/dwb_critics/src/oscillation.cpp	72.00% <0.00%> (-24.00%)	:arrow_down:
nav2_costmap_2d/include/nav2_costmap_2d/layer.hpp	75.00% <0.00%> (-12.50%)	:arrow_down:
...2_core/include/nav2_core/controller_exceptions.hpp	70.00% <0.00%> (-10.00%)	:arrow_down:
..._planner/include/nav2_smac_planner/node_hybrid.hpp	92.85% <0.00%> (-7.15%)	:arrow_down:
nav2_controller/src/controller_server.cpp	82.85% <0.00%> (-6.35%)	:arrow_down:
...tmap_2d/plugins/costmap_filters/costmap_filter.cpp	88.40% <0.00%> (-5.80%)	:arrow_down:
nav2_amcl/src/sensors/laser/beam_model.cpp	97.29% <0.00%> (-2.71%)	:arrow_down:
...tree/include/nav2_behavior_tree/bt_action_node.hpp	85.96% <0.00%> (-2.64%)	:arrow_down:
...stmap_2d/plugins/costmap_filters/binary_filter.cpp	94.80% <0.00%> (-2.60%)	:arrow_down:
...clude/nav2_behavior_tree/bt_action_server_impl.hpp	85.55% <0.00%> (-2.23%)	:arrow_down:
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[afifswaidan]

@SteveMacenski Linting and Unit Testing are done, let me know if you need anything else for this commit. Thanks! 🙂

[afifswaidan]

@SteveMacenski anything else required from my side?

[SteveMacenski]

Sorry, was gone at ROSCon/IROS, just getting back to this now

[afifswaidan]

Otherise LGTM, just remove some methods that don't really appear like that need to be in the file. Dead code is one common way how bugs are made (if its not tested and not used, then if you want to use in the future, you're going to assume it works). Plus some of it seems like printing convenience functions that can be made if/when it becomes an issue. Nice for early development, but I don't think need to be here anymore

I hope you enjoyed the conference and welcome back! All these unused methods that you mentioned are basically extra methods that the user can later on use if they need more information about the line that they fed to the class (the line equation, the current point they are at after calling advance(), etc...) I can either add unit tests for these methods or remove them, whatever you think is better. I just added them to provide extra features for the user later on to access more information about the given line. Let me know what you think :)

[SteveMacenski]

I updated the comments for each about what to remove / keep and add tests. The things that return debugging strings lets remove. The line length can be a helpful gut check, lets add unit tests for that. get_curr_point is already possible via getX and getY.

[afifswaidan]

@SteveMacenski I was checking why the circleci release_test failed, I found it was a failure in another package (nav2_controller). Is anything required from my side? Thanks!

[SteveMacenski]

Probably not, I just retriggered CI to see if it was just a fluke

[SteveMacenski]

Please push a bogus commit (remove a white space in a doc or change a word), CI's built for detecting C++ changes not Python changes so CI isn't happy with caching with getting it to re-run

[afifswaidan]

Please push a bogus commit (remove a white space in a doc or change a word), CI's built for detecting C++ changes not Python changes so CI isn't happy with caching with getting it to re-run

Okay, I'll change a word in a comment and commit again. Edit: I just changed the order of the imports section in the test script

[SteveMacenski]

FYI @ruffsl the colcon cache keyword did not detect the changes to the python files for rebuilding. I don't think you need to do anything about it on our account, just wanted to let you know.

[SteveMacenski]

Huh, just an odd fluke. If it happens again I'll look into it.

Thanks for this contribution!