Stricter filter finding strategy?

[jonbinney]

As described by @v4n here, there are some pitfalls to the current filter finding strategy. Currently if there is no package name specified for a filter in the yaml, then the first filter with the specified string anywhere in its name is chosen:

filters/include/filters/filter_chain.h

Line 202 in 7664bef

if (std::string(config[i]["type"]).find("/") == std::string::npos)

That does output a warning, but given that it can create some really subtle errors when the wrong filter is chosen, it might be better if it actually errored out. For better or worse, many robots have lots of warnings output by various packages, and one more can be easily missed. I can think of a few possible solutions (these would probably only be applied in lunar and later ROS distros):

1. When no package is specified, require an exact string match (not just a substring) between the string specified in yaml and the name of the plugn (without the package)
2. Require specification of the full plugin name, including the package. Error out if the package name isn't there. This is the strictest option. I prefer it since it removes ambiguity from the start, but it could require a lot of people to update their robot configs.
3. Error out if more than one plugin matches the string from the yaml. In theory this would only break robot configurations that are already silently broken.

@tfoote @v4hn what do you guys think?

[tfoote]

The section you linked to which does the promiscuous search was added for backwards compatibility 7 years ago. f7a402c I'd suggest officially deprecating it for lunar and removing it in the future.

[jonbinney]

It's actually had ROS_WARN("Deprecation Warning: No '/' detected in FilterType, Please update to 1.2 plugin syntax. "); for that case since it was added. Since it is already deprecated, do you mind if I open a PR removing it for lunar?

[jonbinney]

(one downside being this will require branching this for lunar, unless we want to check the ros distro in the code)

[tfoote]

Good point. We can definitely move ahead on removing it. It's a very low traffic repo so forking it for this is fine.

I've created a lunar-devel branch for you to target.

[v4hn]

Thank you for taking care of this. Personally I would prefer the "exact match without the package prefix" solution though, because it requires no user to update anything. As we are talking about ROS-l though, all proposed solutions are valid.

[tfoote]

Fixed in #15