search

ros-perception / laser_filters

Assorted filters designed to operate on 2D planar laser scanners, which use the sensor_msgs/LaserScan type.
BSD 3-Clause "New" or "Revised" License
161 stars 199 forks source link

Add static polygon filter #145

Closed erwinbonsmatopic closed 2 years ago

erwinbonsmatopic commented 2 years ago

This adds a variant of the existing polygon filter.

The new filter assumes that the transform from the robot's base to the laser filter is static, i.e. the position and orientation of the laser filter is fixed. This is typically the case. This assumption allows significantly lower CPU usage. First, it does not need to continuously listen to the active transform topic anymore. This ensures that laser filter chains for which there is no laser filter data (e.g. because they are temporarily disabled or are optional and not installed) consume no CPU. On our set-up, each dormant chain was still consuming 1% of CPU, which adds up if you have multiple chains. Second, the transform calculations can be removed from the update loop, as they only need to be performed once, during configuration.

I am not sure if the behavior of the old filter is required. If not, the new filter could replace the old one. However, in order not to introduce any breaking changes, the new static filter is added alongside the old implementation. A common base class is added for code reuse.

jonbinney commented 2 years ago

One question for context before I do a detailed review on this PR - why a new filter in stead of adding a new parameter "dynamic_frame" or something similar, and have it default to true? That would preserve the old behavior while adding the new feature, without needing to add another filter. (no need to change any code yet, i'm just curious)

erwinbonsmatopic commented 2 years ago

One question for context before I do a detailed review on this PR - why a new filter in stead of adding a new parameter "dynamic_frame" or something similar, and have it default to true? That would preserve the old behavior while adding the new feature, without needing to add another filter. (no need to change any code yet, i'm just curious)

This is mainly guided by the implementation. The state that both variants require is quite different. See the header file in this PR.

I can understand that you may want to hide this implementation detail from filter users, and make it appear a single filter with a boolean parameter. Even in this case, I would suggest keeping the implementations separate. To achieve this, the filter logic would move to two "filter logic" components derived from a shared base class (with an API to configure and update). Depending on the new parameter, the PolygonFilter would instantiate and use the appropriate implementation.

This would make the implementation slightly more complex. Let me know if you would prefer this approach. If so, we can refactor the code accordingly.

jonbinney commented 2 years ago

Sounds reasonable. Thanks @erwinbonsmatopic !