Fixes to ros1 port

[aashish-tud]

summary of changes:

• ros2->1 port changes
  • service callback arguments
  • Using rospy for populating timestamp
• Image type generalization
  • Specific to zivid camera or any camera that generated depth images with 32FC1 encoding (suggestion is to use convert_metric nodelt, and 2d images with rgba8 encoding (it is handled by changing encoding using opencv)

This is tested on a physical camera and robot, with expected results.

[gavanderhoorn]

Thanks for the PR.

Since the changes are mostly fixups of things I missed in the initial port, I opted to apply them locally and squash them into ff82ced.

The only change I didn't merge was this bit: https://github.com/Aashish-TUD/industrial_reconstruction/blob/80d3b81042698c9f8e4dbd738c86a230df891d87/industrial_reconstruction/nodes/industrial_reconstruction_node.py#L290-L292 (ie: the RGBA to RGB conversion).

That's not because I don't believe it's necessary, but because I'm wondering whether there'd be a nicer / less invasive way to do it (like a nodelet or something). The proposed change seems a bit too localised to me right now (ie: works for the case you've tested).

We could of course decide to make industrial_reconstruction defensive, and make it pre-process all its inputs so as to enforce its pre-conditions (ie: depth in mm, unsigned short, RGB, not RGBA, image, etc).

[aashish-tud]

Since the changes are mostly fixups of things I missed in the initial port, I opted to apply them locally and squash them into ff82ced.

sounds good.

We could of course decide to make industrial_reconstruction defensive, and make it pre-process all its inputs so as to enforce its pre-conditions (ie: depth in mm, unsigned short, RGB, not RGBA, image, etc).

We could make a noedlet in image_proc maybe? It can do rgba ->rgb conversion.

I read the comments here

I think a bottleneck we will run into is the image format the open3d algorithm expects in the end, which is rgb for the color image. So I think relying on nodelets to pre process the images makes sense, otherwise we will have a cascade of if conditions checking for the encoding types.

At the very least we should add this to the readme for industrial reconstruction so untill the time we have a good solution people are aware of this bottleneck.

[gavanderhoorn]

We could make a noedlet in image_proc maybe? It can do rgba ->rgb conversion.

something like that was indeed what I also had in mind.

OpenCV is pretty 'intelligent' when it comes to data conversions though. If they are not needed, most algorithms will exit early.

So we could just hard-code a conversion to whatever the input requirements for Open3D are, and depend on OpenCV to do the right thing (ie: nothing when not needed).

As to the depth image encoding: reading the code (of Open3D) a bit, it seems they accept more than just integer depth images, but then internally convert to a floating point format anyway. I'm not sure whether it'd be possible to just pass a 32FC1 directly -- it might work wrt the data type, but I believe Open3D wants/needs mm, even if using a float image.

Multiple conversions are probably not going to increase the quality of the data -- or at the very least just increase processing overhead.

Something to discuss upstream, in addition to the point you make about the current requirements which should be documented.

[gavanderhoorn]

Btw, Open3D does accept RGBA, but not for constructing an RGBD image I believe. There are plenty of places in the code which accept 4-channel images.

[gavanderhoorn]

@aashish-tud: just remembered this was still open.

Would you want to refactor this, or perhaps log the outstanding task(s) in an issue.

[gavanderhoorn]

Friendly ping @aashish-tud.

[aashish-tud]

This PR can be closed.
The clean way of handling this issue is outside of industrial reconstruction.
The solution, for now, is to handle the messages before they make it to industrial reconstruction, through a nodelet.
This can be seen for inspiration.