new flag 'keep_box_points' to decide if points in box should be kept …

[NikolasE]

…or discarded

[jonbinney]

Looks like this was already done on the noetic-devel branch. Could you create a new PR that cherry-picks commit 3a7da52bd072ec78f5115804f74358a2c5b31417 from the noetic-devel branch?

[NikolasE]

you are right, that is almost exactly how I did it. But I think my version is better because the "invert"-flag still does not tell the user if the points in the box are discarded or not. There is also some debug-Output ("Box filter started") which imho shouldn't be there during normal operation. (so at least ROS_DEBUG or better removed). And the checks in line 185 to 190 can be written much shorter.

So could we merge my ROS2 version? I could also add a PR for ROS1 (with essentially my code but for ROS1, I however currently don't have a system to test it)

[jonbinney]

I can see what you mean about "keep_box_points" being descriptive, but I think it's more important to be consistent across various filters, and a few filters in the noetic-devel branch already use "invert". I also prefer cherry-picking existing commits so that we can go back and figure out what still needs to be cherry-picked later.

I do agree with you about removing the debugging output. I actually prefer the longer form of the if statements on lines 185 to 190 though. It takes more lines, but I find it more obvious to read. With your version, I found myself having to think a bit to make sure I understand what it is doing.

How about:

• cherry pick the ros1 commit
• add another commit removing the extra debugging output

[NikolasE]

cherry-pick: we'd need an additional commit to go from ros1 to ros2 so I'd rather stay with the new code invert: I get the point and changed the parameter, but it's still inconsistent. The angular filter e.g. is also parameterized by a range, but keeps only the points in the range. So it's not clear if the user has to configure the kept or discarded range. readbility I think now (if (remove_boxpoints == inBox(point)) it's well readable

[jonbinney]

@NikolasE it's not about getting the code perfect in this case, or minimizing the number of commits; it is about maintainability. We still get commits to both the noetic and the ros2 branches, and if they do different things, or even split up their commits differently, it makes it much harder to figure out what still needs to be backported.

[jonbinney]

To be clear - I understand your points and they do have merit, but as the maintainer I'm making the call that they're outweighed by the difficulty of diverging the ros1 and ros2 branches further.

[NikolasE]

I tried to follow your proposal, but cherry-picking won't work directly as the implementation of the box-filter was moved from src/box_filter.cpp to include/laser_filters/box_filter.h

Should I again move the code in ROS2 to a new cpp-file to make the old commit pickable?

[jonbinney]

Good point.... let me take a look.

[jonbinney]

I should have caught that earlier. I don't see any reason the implementation should be in the header files; there are no templates that where the type isn't known until compilation. Looks like it happened during the transition from ros1 to ros2.

Splitting the files up again between headers and source files will make it much easier to port commits between ros1 and ros2. Let me try separating the box filter now.

[jonbinney]

ok, my dream of seamlessly cherry-picking commits across between ros1 and ros2 doesn't seem feasible. There's too much ros1/ros2 specific code scattered throughout the filter implementations. Given that, I think merging this PR is our best option (sorry for the long discussion to get to this point!). @NikolasE github is telling me "the repository that submitted this pull request has been deleted" - can you put it back up?