search

peci1 / robot_body_filter

Filters the robot's body out of point clouds and laser scans.
BSD 3-Clause "New" or "Revised" License
84 stars 22 forks source link

refactor(RobotBodyFilter): Removed frames/sensor parameter #2

Closed reinzor closed 5 years ago

reinzor commented 5 years ago

The sensor frame will now be derived from the incoming sensor messages. This way, messages from different sensors can be processed and the filter can be configured without any knowledge of the sensor.

Addresses #1

reinzor commented 5 years ago

I don't get your use case, weren't we neglecting these scans anyway?

https://github.com/peci1/robot_body_filter/blob/master/src/RobotBodyFilter.cpp#L436

peci1 commented 5 years ago

That's the LaserScan template instantiation. In this one, sensor frame has to be the real sensor frame - otherwise the LaserScan message doesn't make sense. These messages can't be transformed like pointclouds. On the other hand, in the PointCloud2 template instantiation, the sensor frame can differ from what's in the message header. And this is the use-case I want to retain.

reinzor commented 5 years ago

So you are referring to this line: https://github.com/peci1/robot_body_filter/blob/master/src/RobotBodyFilter.cpp#L313

So how you suggest I continue up-on this?

Default the frames/sensor parameter to "" and only use it here https://github.com/peci1/robot_body_filter/blob/master/src/RobotBodyFilter.cpp#L313 with the specified parameter if the parameter is not an empty string? Otherwise, take just the header.frame_id ?

peci1 commented 5 years ago

I'd pass sensorFrame as a parameter to computeMask. In LaserScan version, it will be taken unconditionally from the incoming message. In PointCloud2 version, it could be as you say - if it's empty, then take the message frame, otherwise take whatever is set in the parameter.

You could also issue a warning if the LaserScan version is used and frames/sensor is nonempty.

I'd also add a comment to the docs of frames/fixed that for whole-scan-at-once messages, it would be better to set it to the sensor frame if you know all incoming messages will be from the same sensor. It would then save a few conversions. And we will no longer be able to safely detect this situation from the code.

reinzor commented 5 years ago

Thanks for the feedback, I will try to get at this tomorrow

peci1 commented 5 years ago

Thanks. I've just released version 1.1.5. It should bubble up to the official repos with the next melodic sync (approx. a month).