implemented angle wrap

[kymadogg]

Added handling angle wrapping as suggested in issue #260. Also added test cases in the same style as other test cases.

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[kymadogg]

I think this probably could also be merged into rolling but I chose ros2 for now since I mainly use Jazzy.

[jonbinney]

I think we have to be pretty careful here since, if I understand correctly, this could change the behavior of existing robots who expect the old, non-wrapped version.

For example if we had the params set to lower_angle=5.0, upper_angle=8.0, then before this PR a laser beam with angle=7.0 would have been filtered out, but now it wouldn't, right?

[kymadogg]

yeah I could see that potentially being a problem. There could maybe be a parameter that toggles wrapping vs non-wrapping behavior and the default is false to prevent older robot's laser filtering from breaking?

I am also totally open to ideas if you have any potential solutions you think would be a better fit.

On another note, I have been working on fixing the test cases since I believe I messed something up with setting parameters and had some configuration error on my local machine. I do not use GTest too often if you couldn't tell already.

[jonbinney]

Yes, I think that having a parameter for "wrap angles" that defaults to false is the way to go. Tests look like they are passing now, thanks for that. gtest is never fun :-P

One other request - could you add a comment just above the class definition that describes what the filter does when wrapping is on, and what the filter does when wrapping is off, with a couple of numeric examples? Then I'll do a quick code review for any last details.

[kymadogg]

Sounds good, I'll add the wrap_angle parameter and maybe throw in a few more tests. Also, in addition to adding docs/explanation of the math to the comments, are you ever planning to add a repo level readme or repo wiki pages?

I would be happy to make another PR with a draft readme and/or some wiki pages that have a blurb for each filter, lists its parameters, and contains install / setup instructions.

[jonbinney]

Adding documentation would be great; I have to admit I haven't looked into best practices for documentation for ROS2 packages too much. A top level README.md sounds like a good approach. I'm torn on how much detail should be included - I do like having class level documentation in the header file - it's close to the implementation and we're less likely to forget to update it when changes are made. Not sure how to separate what goes in the README from what goes in the header file. A fair amount of info for the README.md could be pulled from the ROS1 wiki page: https://wiki.ros.org/laser_filters

I'll look at your updated changes in the next few days.

[kymadogg]

I was thinking of making another PR for that since it going to take a while to make sure everything is documented correctly. I also should probably look into some other documentation examples for ROS packages that share a similar structure. I think it's also a little off topic from this current PR.

I'm also moving back for my final year of college soon so it might be a while.

[kymadogg]

Also this last update was to remove a file that shouldn't have been added to one commit (test_detail.xml).

[jonbinney]

Sounds good - no worries if you don't get around to looking at documentation improvements - I should also look into it at some point.

[kymadogg]

Sounds good, let me know if anything filter related needs to change so this PR can get merged/closed. Also thanks for reviewing this!