feat: Speckle filter

[reinzor]

Filter for removing noise / speckles based on ranges. Similar to https://github.com/ros-perception/laser_filters/pull/93 but provides a cheaper (more light weight) method for filtering based on neighboring ranges.

[nlimpert]

Looks nice! And I agree that your solution is much more efficient than mine. I'm fine in replacing my solution with yours as I don't really see a reason for having the same functionality implemented twice (you even implemented a test which I didn't). What do you think?

I guess the only things missing would be the example launchfile and its configuration.

Btw; I'd have liked to hear your opinion in #93. ;)

[jonbinney]

+1 for combining the functionality into one filter! It would mean one less filter to update :-)

[reinzor]

Maybe that makes sense indeed since both filters are designed to filter out speckle measurements from lidar sensors. I do think however that euclidean filtering sometimes makes more sense than range based filtering and the other way around. @nlimpert , thanks for #93 , nice to see some more activity in this repo and people implementing dynamic reconfigure for filters ;).

For the merge of these two filters, I would suggest:

• Call it SpeckleFilter
• Add some type of type Enum parameter so the user can choose between RadiusOutlier and Distance.
• Merge parameter sets into one, something like:

type: 0, 1 # RadiusOutlier / Distance
max_distance: 0.4 # For RadiusOutlier: max distance between points, For Distance max distance between consecutive points
window: 4 # For RadiusOutlier: Minimum number of neighbors, For Distance: Number of consecutive ranges that will be tested for max_distance
num_consecutive_measurements: 10  # Do we want this?

What do you think? I am not sure whether combining these two makes things more clear in terms of parameters.

[nlimpert]

For the merge of these two filters, I would suggest:

• Call it SpeckleFilter

• Add some type of type Enum parameter so the user can choose between RadiusOutlier and Distance.

Sounds good to me!

• Merge parameter sets into one, something like:

type: 0, 1 # RadiusOutlier / Distance
max_distance: 0.4 # For RadiusOutlier: max distance between points, For Distance max distance between consecutive points
window: 4 # For RadiusOutlier: Minimum number of neighbors, For Distance: Number of consecutive ranges that will be tested for max_distance
num_consecutive_measurements: 10  # Do we want this?

What do you think? I am not sure whether combining these two makes things more clear in terms of parameters.

Good point in merging parameters and using an enum for the selection. I agree on the max_distance parameter but thought that the window parameter would have its functionality just like the proposed num_consecutive_measurements. Imo this would have the benefit that either for Distance or RadiusOutlier the parameter's purpose would stay the same - being the number of consecutive ranges to be checked. Regarding the neighbors I think at least for the RadiusOutlier there's no real need in having an additional parameter (I've just tried setting min_neighbors_ = config.window for the RadiusOutlierFilter and it seems to work out nicely in practice).

So imo there's no need for an additional num_consecutive_measurements.

[reinzor]

Thanks, looks good! Would you have the time to merge the two? If not, I can try to take a look at it at the end of this week.

[nlimpert]

Yes I can take a look but not before Thursday. I'll branch off your work so we can discuss the changes and you can possibly cherry-pick later.

[reinzor]

Thanks for the update @nlimpert , now this PR should address the merge of the two speckle filter implementations.

[jonbinney]

@reinzor is this ready for a review? Could you rebase to fix the conflict in the cmakelists?

[reinzor]

@jonbinney , it should be ready for a review now.

[reinzor]

Thanks for the review, and thanks @nlimpert :)