Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spatial median filter #201

Merged
merged 10 commits into from
Aug 23, 2024
Merged

Conversation

YBachmann
Copy link
Contributor

This PR adds a 1D spatial median filter.
The size of the filter can be set via the parameter window_size.

In this screenshot you can see the raw laserscan in blue and the result of the spatial median filter in red (window_size=31).
The filter smoothes the walls and also removes isolated/scattered noise (see e.g. directly right of the robot).
spatial_median_filter

@jonbinney
Copy link
Contributor

This looks quite useful! I'll take a closer look tomorrow.

Copy link
Contributor

@jonbinney jonbinney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some questions. Also could you add an example yaml and python launch file in the examples folder?

include/laser_filters/median_spatial_filter.h Outdated Show resolved Hide resolved
include/laser_filters/median_spatial_filter.h Show resolved Hide resolved
include/laser_filters/median_spatial_filter.h Outdated Show resolved Hide resolved
include/laser_filters/median_spatial_filter.h Outdated Show resolved Hide resolved
include/laser_filters/median_spatial_filter.h Show resolved Hide resolved
include/laser_filters/median_spatial_filter.h Outdated Show resolved Hide resolved
laser_filters_plugins.xml Outdated Show resolved Hide resolved
@jonbinney
Copy link
Contributor

It looks like you've made some changes to address my comments. When you have a chance, could you reply to each review comment to note that you've addressed it, and let me know when this PR is ready for another look?

@YBachmann
Copy link
Contributor Author

YBachmann commented Aug 14, 2024

It looks like you've made some changes to address my comments. When you have a chance, could you reply to each review comment to note that you've addressed it, and let me know when this PR is ready for another look?

Hey @jonbinney ,
I replied to each comment and marked it as resolved, please re-open a comment if you think it still needs something changed.
Edit: I Just noticed, that an example yaml and Python launchfile are still missing. I will add those on monday.

@jonbinney
Copy link
Contributor

Thanks! One more small request: could you add a comment in the code about why the window size needs to be incremented if it is even? Your explanation was good, just copy it into the code there: "If the window_size is even, the current range, which will be modified cannot be centered in the window." Once you do that and add the example files, this should be ready for me to merge.

…dded comment and warning when ensuring window_size_ is odd
@YBachmann
Copy link
Contributor Author

Hello @jonbinney ,
sorry for the late response, I had some urgent things to do this week.
I added an example xml and python launchfile and a parameter yaml file that's used by the launchfiles. I also added a comment regarding the odd window_size_ in the code.
Should this be the last update, I would like to thank you for taking the time to reviewing this PR!

@jonbinney
Copy link
Contributor

Merging - thanks for contributing this @YBachmann !

@jonbinney jonbinney merged commit de3520a into ros-perception:ros2 Aug 23, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants