New feature: LiDAR-based panoptic segmentation - EfficientLPS

[aselimc]

Hi,

This PR adds EfficientLPS network. The original repo can be found here.

ROS Nodes are also added together with this PR. Furthermore, we have introduced PointCloud2 publisher, which is the latest PointCloud message type in ROS.

This PR does not need to be merged before the next release.

[Edit] Performance evaluation has to be completed but I don't have a recent Jetson right now. @vniclas

[aselimc]

@ad-daniel , all the changes you have requested should be addressed now. Can you please re-review it ?

[aselimc]

Thank you for the new tool! I have added some comments regarding the ROS nodes and the docs. ~Is there a plan for ROS2?~ Edit: Sorry missed the other PR that adds the ROS2 nodes. Note that the ROS1 node and doc comments from this review, might also apply for ROS2.

@tsampazk I just have checked the other PR, and it seems that branch is super old at the moment. Is it okay for me to reset that branch into the current develop and re-write the ROS2 node again?

[tsampazk]

Thank you for the new tool! I have added some comments regarding the ROS nodes and the docs. ~Is there a plan for ROS2?~ Edit: Sorry missed the other PR that adds the ROS2 nodes. Note that the ROS1 node and doc comments from this review, might also apply for ROS2.

@tsampazk I just have checked the other PR, and it seems that branch is super old at the moment. Is it okay for me to reset that branch into the current develop and re-write the ROS2 node again?

Sure, go ahead and do it the way you think it's more convenient, it is still draft anyway. :)

[aselimc]

Sorry I had to send an empty commit due to the fact that previous commit wasn't included in the PR somehow, so I had to re-trigger.

[tsampazk]

Thank you for the fixes!

To be honest i haven't been able to test the node myself because it requires the dataset which is quite large. From previous experience there are lots of things that can go wrong when running the nodes that cannot be anticipated beforehand. Is there a possibility of adding a single point cloud and its label on our FTP server for testing purposes similar to other tools, both for the nodes and the demos?

[aselimc]

Thank you for the fixes!

To be honest i haven't been able to test the node myself because it requires the dataset which is quite large. From previous experience there are lots of things that can go wrong when running the nodes that cannot be anticipated beforehand. Is there a possibility of adding a single point cloud and its label on our FTP server for testing purposes similar to other tools, both for the nodes and the demos?

I will try to have a look on this. When I was testing, I was also using a single point cloud obviously but I need to have closer look to upload this on the FTP.

[tsampazk]

I will try to have a look on this. When I was testing, I was also using a single point cloud obviously but I need to have closer look to upload this on the FTP.

Yes of course, if there are licensing or other issues we can work with what we have. :smile:

[aselimc]

I will try to have a look on this. When I was testing, I was also using a single point cloud obviously but I need to have closer look to upload this on the FTP.

Yes of course, if there are licensing or other issues we can work with what we have. smile

Now you should be able to test it using the test data. I have added an option to the Point Cloud publisher node where if you call the node with rosrun opendr_perception point_cloud_2_publisher_node.py --test_data , then it should automatically download the test data from the FTP server and publish it.

Edit: I had some issues with this error Illegal instruction (core dumped) when I did the catkin_make but my last few builds were working fine. If you see such an error please let me know.

[tsampazk]

Thank you very much @aselimc!

Regarding the download methods, for consistency reasons and for being easier to use (e.g. through demos too), i would suggest moving the download stuff inside the learner's download method, by adding a second mode similar to other nodes, sorry for not making more clear earlier.

I tested the node and unless i'm mistaken, it seems that the output topic is not really an RGB visualization topic as the name suggests. Similar to the other panoptic segmentation node, there are two output topics expected, one where the heatmap is published and another one where the heatmap is blended with an RGB image for visualization purposes.

[aselimc]

Thank you very much @aselimc!

Regarding the download methods, for consistency reasons and for b eing easier to use (e.g. through demos too), i would suggest moving the download stuff inside the learner's download method, by adding a second mode similar to other nodes, sorry for not making more clear earlier.

I tested the node and unless i'm mistaken, it seems that the output topic is not really an RGB visualization topic as the name suggests. Similar to the other panoptic segmentation node, there are two output topics expected, one where the heatmap is published and another one where the heatmap is blended with an RGB image for visualization purposes.

@tsampazk For the first suggestion, we already have the download method of download but it was a bit confusing for me to call the learner in two nodes due to the error that I mentioned earlier. But now I figured out that my environment was the root cause for the error so I will fix this.

But about the second point, I believe the purpose of panoptic segmentation is to simply merge the semantic and instance segmentations and publish according to this. Thus, the RGB visualization topic publishes an RGB labelled pointcloud where users can visualize it with tools like RVIZ. I can separate the panoptic output of pointcloud into semantic and instance segmentation maps but I think this does not bring much to the table. @vniclas What do you suggest? Also, I am not entirely sure about heatmap+input combination for this task simply because inputs are not RGB images.

[tsampazk]

@tsampazk For the first suggestion, we already have the download method of download but it was a bit confusing for me to call the learner in two nodes due to the error that I mentioned earlier. But now I figured out that my environment was the root cause for the error so I will fix this.

It is true that it doesn't make much sense to initialize and call the learner in the point cloud publisher, so i would suggest to keep that as-is and add a test_data mode to the learner's download method that basically just downloads the data. This will make it possible to quickly run the demos of the tool. This will lead to some minor code duplication but i think it's acceptable.

But about the second point, I believe the purpose of panoptic segmentation is to simply merge the semantic and instance segmentations and publish according to this. Thus, the RGB visualization topic publishes an RGB labelled pointcloud where users can visualize it with tools like RVIZ. I can separate the panoptic output of pointcloud into semantic and instance segmentation maps but I think this does not bring much to the table. @vniclas What do you suggest? Also, I am not entirely sure about heatmap+input combination for this task simply because inputs are not RGB images.

You're right, when looking at the output i missed it entirely that this node indeed doesn't work with RGB stuff :confused:.

The output topic doesn't show up on rqt-image-viewer which i have been using for node testing to do visual verification. I test the nodes this way because from use-case partner feedback in other nodes, they usually first expect something tangible in the output, i.e. visual output, and secondly a detection, e.g. a heatmap or a bounding box which can be echoed. If it is indeed not meaningful or possible to have an easy visualization of the output then i suggest renaming the topic to something that doesn't indeed imply (easy) RGB visualization to avoid confusion.

I'm sorry about the wall of text and insisting on the matter, we had several user issues in the past about the output topics.

[aselimc]

@tsampazk For the first suggestion, we already have the download method of download but it was a bit confusing for me to call the learner in two nodes due to the error that I mentioned earlier. But now I figured out that my environment was the root cause for the error so I will fix this.

It is true that it doesn't make much sense to initialize and call the learner in the point cloud publisher, so i would suggest to keep that as-is and add a test_data mode to the learner's download method that basically just downloads the data. This will make it possible to quickly run the demos of the tool. This will lead to some minor code duplication but i think it's acceptable.

But about the second point, I believe the purpose of panoptic segmentation is to simply merge the semantic and instance segmentations and publish according to this. Thus, the RGB visualization topic publishes an RGB labelled pointcloud where users can visualize it with tools like RVIZ. I can separate the panoptic output of pointcloud into semantic and instance segmentation maps but I think this does not bring much to the table. @vniclas What do you suggest? Also, I am not entirely sure about heatmap+input combination for this task simply because inputs are not RGB images.

You're right, when looking at the output i missed it entirely that this node indeed doesn't work with RGB stuff confused.

The output topic doesn't show up on rqt-image-viewer which i have been using for node testing to do visual verification. I test the nodes this way because from use-case partner feedback in other nodes, they usually first expect something tangible in the output, i.e. visual output, and secondly a detection, e.g. a heatmap or a bounding box which can be echoed. If it is indeed not meaningful or possible to have an easy visualization of the output then i suggest renaming the topic to something that doesn't indeed imply (easy) RGB visualization to avoid confusion.

I'm sorry about the wall of text and insisting on the matter, we had several user issues in the past about the output topics.

Sorry for the late reply. Right now the download method is a static method where it is called from the learner and I have further adjusted the naming of the output topic as you have suggested. Please let me know if there are any extra changes required.

[vniclas]

Thanks for the extensive review @tsampazk :pray:

@passalis @ad-daniel , could you please also have a look when you have time so that we can merge this PR