Add passthrough filter

[jordi-pages]

An optional passthrough filter along the Z axis of the camera optical frame is added in order to remove noise that appears close to the optical center.

Apart from that, the camera_link is named "base_rgbd_camera_optical_frame" rather than "base_rgbd_camera_link" as the Z axis actually points forward.

[gavanderhoorn]

Is this something we want to include in the driver itself, or should it be a separate node(let)?

I appreciate the difficulties with high-bandwidth communication between nodes, but in my opinion a driver should just be concerned with getting the data out of the device / OS driver as efficient as possible, nothing more (single responsibility principle?).

[jordi-pages]

Thanks for the comment. I get your point and I agree that perhaps it is not good to overload a driver like this. Does anyone else want to discuss it? Otherwise I will cancel the pull request.

[gavanderhoorn]

Don't get me wrong: nothing wrong with the functionality per se, just thought we might be careful and try to avoid feature creep.

I've seen some other drivers successfully use the nodelet approach for precisely this kind of thing (adding the ability to setup a filtering pipeline between the camera driver and image/depthmap consumer). Perhaps that approach could be used here as well. With a suitable nodelet wrapper, standalone execution would be completely backwards compatible with the current package.

[ipa-flg]

Even though I completely agree with @gavanderhoorn , I am inclined to merge the PR anyway. As we have these kind of simple filters included already in the driver, and as they can be deactivated easily, adding another one will be ok.

Nonetheless, it'll be more coherent to move these features to a seperate node (or nodelet). I'll open an Issue for that.

[corot]

@jordi-pages, this PRs has become quite big and containing different changes. Could you please split it into individual changes, each one in his own branch so you can keep developing while someone can test and merge it?

Thanks!

Out of curiosity, can you change camera parameter on runtime? Or do you need to restart the DepthSense nodes?

[corot]

Hi @jordi-pages, I have reviewed the PR and all changes seem reasonable. If nobody disagrees, I'll merge, and then merge also to indigo. However, I think is misleading add the camera parameters on the dynamic reconfigure, as the new parameters are not really applied, right? You would need to restart the DepthSense nodes.

[jordi-pages]

You are right @corot. @efernandez, I think you added these parameters in the dynamic reconfigure. Can you comment on that?

[corot]

Btw, are you still using hydro branch? If so, no plans to move to indigo in the near term? I would encourage to do so, as indigo is notably faster, and like this we can concentrate all efforts on it, starting with this PR. Then will come separate filters in a nodelet and release.

[corot]

Hi @jordi-pages, @efernandez, I have tested the PR and it works, but there are 2 problems:

• Two errors on dyn reconf (fixed here)
• I think the non valid depths should be nan (as in kinect and indigo), or at least zero (as in hydro). You change them to max distance, what generates a weird "negative" plane projected at 6 meters from the camera.

If you are already using indigo, I would suggest to remake the PR for indigo_dev branch. If not, just fix the two issues and we merge it on hydro, and I will port filters and dyn rec to indigo.

Thanks!

[efernandez]

@corot, thanks for the fix on the cfg file.

I admit that the points published at max range look bad. I think at the end, that "feature" isn't really useful, so it could be removed and changes into NaN as you're suggesting for indigo.

[gavanderhoorn]

@corot & @efernandez: see REP-117 for the established way of handling out-of-range and / or invalid depth values for sensors that perform depth measurements.

[efernandez]

@gavanderhoorn Thanks. I already knew about REP-117. However, for this kind of sensors, AFAIK it's not possible to know if the measurement is very close, very far or simply there's no measurement. So I think NaN is the only value that actually make sense to put on the PC. What do you think? I'd be very happy to have +Inf and -Inf as well, but I don't think that's possible.

[gavanderhoorn]

@efernandez wrote:

So I think NaN is the only value that actually make sense [..]

Isn't that exactly what REP-117 stipulates? Quote:

Detections that are too close to the sensor to quantify shall be represented by -Inf. Erroneous detections shall be represented by quiet (non-signaling) NaNs. Finally, out of range detections will be represented by +Inf.

I understood that you already wanted to use NaNs for this (you wrote: "I think the non valid depths should be nan"). I just wanted to provide you with a rationale for why this would be a good idea.

[corot]

@efernandez is right: the sensor only tells you "wrong reading" when it detects nothing, so you cannot discriminate between too short, too long and wrong readings. So I change all to NaN in indigo.

[corot]

Hi all, I merged the PR into my fork. All seems to work fine except the frustum culling filter, that kills the whole pc (I suppose because in indigo we have a different tf tree, mimicking that of kinect)

[ipa-flg]

hydro_dev not supported anymore