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

Split shadow filter headers #139

Closed erwinbonsmatopic closed 2 years ago

erwinbonsmatopic commented 2 years ago

The code for the shadow filter had its entire implementation in its header file.

There not seem to be a good reason for this. It is inconsistent with the implementation of the other filters. It also slows down incremental builds. It can also result it bloated binaries (as code may be built multiple times).

So this PR moves the implementation to cpp files. Also made minor changes to header comments to improve consistency. No changes have been made the filter's implementation. (This will be done in follow-up PRs)

jonbinney commented 2 years ago

@erwinbonsmatopic looks like tests failed?

erwinbonsmatopic commented 2 years ago

The problem is that in the CI build the tests do not link to the built library in build/devel/lib but instead to the installed library in /opt/ros/noetic (which is included in the ros:noetic-perception docker image).

A workaround is to adapt ci.sh to invoke cmake twice (directly after the first invocation). When doing this, the second invocation modifies build/catkin_generated/setup_cached.sh and adds the following line: export LD_LIBRARY_PATH="/home/erwin/views/laser_filters/build/devel/lib:$LD_LIBRARY_PATH"

With this, the tests correctly link to the latest built code and pass. Admittedly this is quite an ugly workaround. Ideally the first cmake invocation already adds this required path but I am not sure yet how to achieve that.

Note the wrong-linking problem was already present before this changeset. It for example also occurs for the speckle filter tests. This might even explain why the shadow filter header files included the implementation. That may have been an even uglier workaround for this issue.

erwinbonsmatopic commented 2 years ago

I added the CI workaround to this PR. Although not the perfect solution, this may be sufficient to enable the merge for several reasons. First, the problem was not introduced by this PR, it only reveals the existing problem. Second, this fixes the issue that the tests were not actually testing the built laser_filters library, but the installed library instead so that at least the tests can detect regressions. Third, it enables creation and review of PRs for subsequent improvements to the filter.

jonbinney commented 2 years ago

Since directly using cmake was causing this problem, I've updated CI to use catkin_make: https://github.com/ros-perception/laser_filters/pull/140

Could you rebase this PR on noetic-devel and remove your changes to ci.sh?

erwinbonsmatopic commented 2 years ago

Thanks for properly fixing ci.sh script. I rebased from noetic-devel (via a merge) and thereby undid my changes to ci.sh.

erwinbonsmatopic commented 2 years ago

@jonbinney Thanks for reviewing. I handled your comments except for one (see individual responses for details).

jonbinney commented 2 years ago

Thanks @erwinbonsmatopic, looks good!