ros-perception/laser_filters

specifying LaserScanAngularBoundsFilter can result in *the wrong points* being removed

v4hn opened this issue · 7 comments

v4hn commented

The filters package requires plugins to be specified with their package name as a prefix:
https://github.com/ros/filters/blob/hydro-devel/include/filters/filter_chain.h#L202

If this is not the case, such as e.g. in the PR2 navigation config, @tfoote made filters look through all available packages and pick the last one that contains (not has the exact) name (see link above).

Now what could possibly go wrong after @kmhallen came along in 2014 and added a LaserScanAngularRemovalFilter filter that has exactly the opposite behavior of LaserScanAngularBoundsFilter?
Nothing.
Until they renamed the filter to LaserScanAngularBoundsFilterInPlace one day later.

Since then, this kind of (heavily outdated but otherwise fully functional) style of restricting the LaserScan

- name: angular_bounds
  type: LaserScanAngularBoundsFilter
  params:
    lower_angle: -1.22173048
    upper_angle: 1.22173048

will make laser_filters autocomplete the type to laser_filters/LaserScanAngularBoundsFilterInPlace instead and the plugin will invert its behavior.

As a result the PR2 at the moment fails to see hovering obstacles in front.

Wow.

jonbinney commented

@v4hn thanks for describing in detail how this happens! This should have been happening for quite a while, right? Did this just start happening on PR2s?

Since renaming filters now could break other things, and there are lots of robots besides the PR2, I'm not crazy about that. Likewise making the plugin finding logic stricter to require package names would likely break lots and lots of existing configurations. I would say we should add a warning, but there already is one, right? ros/filters@f7a402c#diff-d2e0e6681a387bd0b976321a1170c6a8R437

So are there any changes you'd like to see to laser_filters as a result of this problem? It seems to me that the solution is for the PR2 code to get fixed, and for people to heed the warning that is already there.

jonbinney commented

One possible fix: we could error out in the case that multiple plugins contain the filter name from the yaml. I suppose that would only break robots that already have a config that could be breaking things.

jonbinney commented

Also, what version of ROS are PR2s running these days? Is it a version still built by the buildfarm?

v4hn commented

Morning @jonbinney :)

There is a ... partially incomplete ... indigo release of the PR2 that got released by Clearpath.
This is one of the things that are still wrong there. As you can see above, I just filed a request with the PR2 repo too. So yes, obviously this should be fixed there.

However, due to the bizarre nature of this error, I wanted to point it out to this repo too.
I believe it would be a good idea to make the filters package more strict in the matching in ROS lunar+,
but I don't see any good way to resolve this problem for indigo to kinetic either.

jonbinney commented

Since the plugin finding code is in the filters package, I've created an issue there as well: ros/filters#14

jonbinney commented

Filter finding in the filters package has now been updated to require the package name and exact filter name. When the PR2s are updated to Lunar, there will at least be a better error message 😉

v4hn commented