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

More Documentation needed #1

Closed
uahic opened this issue Aug 8, 2022 · 5 comments
Closed

More Documentation needed #1

uahic opened this issue Aug 8, 2022 · 5 comments
Assignees
Labels
documentation Improvements or additions to documentation enhancement New feature or request

Comments

@uahic
Copy link

uahic commented Aug 8, 2022

I kind of like the idea of a unified filter chain package but the documentation is really really sparse. For example I have never worked with nodlets the pcl_ros perception filters etc. before and can't infer if I could for example use the passthrough filter of pcl_ros directly with your chain or not (probably not?).

@peci1 peci1 self-assigned this Aug 8, 2022
@peci1 peci1 added documentation Improvements or additions to documentation enhancement New feature or request labels Aug 8, 2022
@peci1
Copy link
Member

peci1 commented Aug 8, 2022

Hi, thanks for letting me know you're struggling.

Based on your comments, I have a few ideas regarding improvements of the docs. Maybe the most important would be putting a link to https://wiki.ros.org/filters near the top of the readme. This page explains the concept of ROS filters.

Regarding pcl_ros "filters" - no, you can't use them with this package. They are filters from the PCL point of view, but from the ROS point of view, they are nodelets, and thus cannot be plugged inside a ROS filter chain. I'll also try to explain that in the readme.

Last idea - I'll take the example files typed verbatim at the end of the readme and turn them into real files inside a launch folder, just where many ROS users are used to look for examples.

Do you have any further ideas on how the docs could be improved?

Generally speaking, the concept of ROS filters is largely underused in the ROS ecosystem, although they offer the highest performance of all alternatives. I don't exactly know why. But that's the reason why you won't find many ready-made filters out there. So the expected use-case for this package is that you write a few of your own filters, and sensor_filters will save you writing the boilerplate of a node(let) that's setting up the chain of filters and is running it. One of the few non-trivial filters available out there is for example https://github.com/peci1/robot_body_filter .

@uahic
Copy link
Author

uahic commented Aug 15, 2022

@peci1 Hi! thanks for the quick response, I've managed to implement my own LaserScan and PointCloud2 filters (partially using PCL filters inside). To know that filters::FilterBase is the core helps alot, yep I've had a look into robot_body_filter which gave me all the clues :)

Another thing you should point out is in terms of performance. Currently I'm a bit under project time pressure so didnt crawled your source code (or that of filter base) yet, but I wonder how big the complexity is to pass data from one filter to another; Best would be if its possible to pass a message through the filter chain with zero copy (assuming that each filter does nothing else than identity mapping, of course its up to the user if they operate in-place on the message or copy it into other formats)

Currently, I'm not sure about this and assume that between each filter there is a copy indeed but for the moment I'm willing to sacrafice a little performance for the sake of having my filters chainable and reusable instead of writing a standalone node that chains them

@peci1
Copy link
Member

peci1 commented Aug 15, 2022

Glad to hear the additional info helped you. I'll incorporate it here shortly.

Support for zero-copy filters has been proposed here: ros/filters#38 . I'd be glad if you could chime in and vouch for the PR. It only has to be updated now because recent changes to the filters packages made it incompatible.

The standard filters are not zero-copy, but they should still be the most performant way of processing a chain of data modifiers. Passing data from filter to filter is just about copying the message structure. In nodelets, you have to use shared pointers to get the most performant way of sharing data, so there you pay the extra price of dynamic memory allocations (unless you do tricks with pool-based allocation). Not speaking about the other ways of sharing data, which include (de)serialization or even travel via the TCP stack.

@uahic
Copy link
Author

uahic commented Aug 15, 2022

Awesome. I will have a look on the PR in late September during my vacations, currently I have to meet 3 deadlines very soon

Seems like the copying situation is much better than expected, worst case would have been that each filter is instantiated as a separate ROS node that passes around messages over the network stack. Most filter implementations I have seen so far seem to convert the sensor_msgs:: to PCL anyways and dont work in-place and prefer simplicity over performance. Yeah ok fiddling around with data field structures and computing offsets for iterators isnt the nicest thing to do, so I get it :D

@peci1 peci1 closed this as completed in a6308f3 Aug 15, 2022
@peci1
Copy link
Member

peci1 commented Aug 15, 2022

I've finished the extension of the docs. I hope they will be more useful from now on! Thanks for triggering this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants