Repository: valgur/velodyne_decoder (language: python)

Issue:
Message decode issue with VLP16
We are using the velodyne_decoder to decode the velodyne_packets topic obtained from the VLP16 Lidar. Previously, I was using version 2.3.0 or 2.0.0, and for each scan, I would usually get an array of N-by-6. Today I installed the latest 3.0.0 version, and although I still get an N-by-8 array, I found that each row contains the same values, which seems to indicate an error after the update. For the same bag file, version 2.3.0 gives me the correct results. However, it is obvious that version 3.0.0 is easier to use, and I hope this error can be fixed.


Discussion / hints:
Interesting. I have used version 3.0.0 without issues so far.
I would like to investigate this. Would it be possible for you to share a small slice of data? A couple of scans should be sufficient.
Sure, I am willing to share some data with you. Unfortunately, the bag files I have right now are too big to use. I am going to collect some small data and share with you today afternoon. The image shown below is the output points generated with 3.0.0
![image](https://github.com/valgur/velodyne_decoder/assets/59807534/ceef6078-b877-4b8f-9d99-b347c5820486)




Hi Martin, the attached bag file contains the /velodyne_packets topic, I think it can be used for testing. I forgot to mention earlier when I used the 3.0.0, I was set model=vd.Model.VLP16, it showed that the model was incorrect, then I changed to vd.Model.PuckHiRes to fix the error. I am not sure whether this setting results in identical data points in one scan. Please let me know if you need more data, thank you.

[test_bag.zip](https://github.com/user-attachments/files/16023900/test_bag.zip)

Thanks! The bag you shared contains already decoded point clouds as `sensor_msgs/PointCloud2`, though. Would it be possible to share the actual data you are feeding into `velodyne_decoder`?
Also, can you please share a snippet of the code you are using for decoding? I assume you are using `vd.read_bag(<bag>)`?
I would recommend leaving the `model` config parameter unset in v3. Filling it has basically no benefit unless you are using HDL-64, where that info is not available from the data.
Hi Martin, I am sorry I didn't realize the previous bag file doesn't contain the raw Velodyne data packets (velodyne_msgs/VelodyneScan). I have attached another bag file that contains velodyne_packets topic and the scripts we used to parse the bag file. The bag file may contain other topics from different sensors. Please let me know if you have any questions or concerns. 
[Bag file and parse function.zip](https://github.com/user-attachments/files/16060369/Bag.file.and.parse.function.zip)

I tested the bag with:

```py
import velodyne_decoder as vd
from rosbag import Bag

bag = Bag("mapping_van_2024-06-28-12-30-32_0.bag")
scans = list(vd.read_bag(bag, topics=["/velodyne_packets"]))

print("Number of points in each scan:")
print([len(scan.points) for scan in scans])
t0 = scans[0].stamp.device
print("Relative device timestamp of each scan:")
print([round(scan.stamp.device - t0, 3) for scan in scans])
```
Which prints:
```
Number of points in each scan:
[15024, 15036, 15031, 15021, 15042, 15031, 15039, 15029, 15029, 15031, 15024, 15026, 15020, 15028, 15045, 15033, 15030, 15021, 15035, 15038, 15037, 15023, 15022, 15038, 15031, 15019, 15043, 15019, 15028, 15024, 15035, 15029, 15020, 15028, 15043, 15054, 15066, 15075, 15125, 15146, 15187, 15189, 15215, 15254, 15280, 15284, 15283, 15286, 15272, 15276, 15272, 15279, 15283, 15280, 15275, 15277, 15267, 15285, 15275, 15276, 15271, 15269, 15272, 15287, 15286, 15272, 15278, 15266, 15273, 15278, 15284, 15279, 15282, 15278, 15272, 15281, 15279, 15275, 15270, 15279, 15275, 15286, 15276, 15273, 15268, 15268]
Relative device timestamp of each scan:
[0.0, 0.101, 0.202, 0.303, 0.403, 0.504, 0.605, 0.706, 0.807, 0.908, 1.009, 1.109, 1.21, 1.311, 1.412, 1.513, 1.614, 1.715, 1.815, 1.916, 2.017, 2.118, 2.219, 2.32, 2.421, 2.521, 2.622, 2.723, 2.824, 2.925, 3.026, 3.127, 3.228, 3.328, 3.429, 3.53, 3.631, 3.732, 3.833, 3.934, 4.034, 4.135, 4.236, 4.337, 4.438, 4.539, 4.64, 4.74, 4.841, 4.942, 5.043, 5.144, 5.245, 5.346, 5.446, 5.547, 5.648, 5.749, 5.85, 5.951, 6.052, 6.152, 6.253, 6.354, 6.455, 6.556, 6.657, 6.758, 6.858, 6.959, 7.06, 7.161, 7.262, 7.363, 7.464, 7.564, 7.665, 7.766, 7.867, 7.968, 8.069, 8.17, 8.271, 8.371, 8.472, 8.573]
```

So the decoded clouds are distinct and I could not reproduce the issue.

My guess would be that there's some kind of logic error in your code. I did not notice anything glaringly wrong in the shared code, except for setting and using `config = vd.Config(model = "VLP-16", rpm = 600)`. It's both unnecessary with the latest version and the string value being passed to `model` would raise an error with the latest version as well. Are you sure you are using the latest v3.0.0 from pip?
Closing as likely not a library bug, but feel free to follow up with any comments or questions if you have any.
I had the same issue when using this library to read pcap files from a VLP-32C.
The workarround for me was to set `as_pcl_structs=True` which would avoid the duplicate messages.
Thank you for your reply, I set 'as_pcl_structs=True` and it works. 

> I had the same issue when using this library to read pcap files from a VLP-32C. The workarround for me was to set `as_pcl_structs=True` which would avoid the duplicate messages.

Hi, Marting, I am not sure whether you compared the differences between the two versions. The results I got from version 3.0.0 are different from version 2.3.0. The following two images respectively show the results of the same scan obtained from the two versions, it can be noticed that although the parsed points are closed, there still exists offsets between them. We are using a VLP-16 Hi-Res LiDAR, and using 'config = vd.Config(model = "VLP-16", rpm = 600)' in version 2.3.0, while leaving the config setting empty in the latest version. Is this setting the cause of the different results? Thank you!

![image](https://github.com/user-attachments/assets/5648f1c1-a908-4ca4-bdfb-1287ad6e0e09)

![image](https://github.com/user-attachments/assets/baf76ef3-453e-4e82-bb49-2ca40cb4c29a)







Fix patch:
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index a620693..fac81f4 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -71,6 +71,10 @@ jobs:
     steps:
       - uses: actions/checkout@v4
 
+      - uses: actions/setup-python@v5
+        with:
+          python-version: '3.12'
+
       - name: Install Clang
         if: matrix.platform[2] == 'clang'
         uses: KyleMayes/install-llvm-action@v1
@@ -112,6 +116,10 @@ jobs:
     steps:
       - uses: actions/checkout@v4
 
+      - uses: actions/setup-python@v5
+        with:
+          python-version: '3.12'
+
       - name: Install Clang
         if: matrix.platform[2] == 'clang'
         uses: KyleMayes/install-llvm-action@v1


Test patch:
diff --git a/test/test_pcap.py b/test/test_pcap.py
index 33b02c2..af65fa4 100644
--- a/test/test_pcap.py
+++ b/test/test_pcap.py
@@ -15,6 +15,8 @@ def test_pcap_as_contiguous_array(sample_pcap_path):
     # Check that the column index is monotonically increasing
     column_idx = pcd[:, vd.PointField.column].astype(int)
     assert np.diff(column_idx).min() == 0
+    # Sanity check that the point clouds are actually distinct
+    assert not all(len(x) == len(pcd) for _, x in pcds)
 
 
 def test_pcap_as_struct_array(sample_pcap_path, pcl_struct_dtype):
@@ -27,6 +29,7 @@ def test_pcap_as_struct_array(sample_pcap_path, pcl_struct_dtype):
     # Check that the column index is monotonically increasing
     column_idx = pcd["column"].astype(int)
     assert np.diff(column_idx).min() == 0
+    assert not all(len(x) == len(pcd) for _, x in pcds)
 
 
 def test_pcap_time_range(sample_pcap_path):
@@ -35,3 +38,23 @@ def test_pcap_time_range(sample_pcap_path):
     stamp, pcd = pcds[0]
     assert stamp.host >= t0
     assert stamp.device >= t0
+
+
+def _struct_pcd_to_contiguous(pcd_struct):
+    shape = (len(pcd_struct), len(pcd_struct.dtype.fields))
+    converted_pcd = np.empty(shape, dtype=np.float32)
+    for i, field in enumerate(pcd_struct.dtype.fields):
+        converted_pcd[:, i] = pcd_struct[field]
+    return converted_pcd
+
+
+def test_pcap_structs_match_contiguous(sample_pcap_path):
+    """Check that the contents of as_pcl_structs=True/False match."""
+    pcds_struct = list(vd.read_pcap(sample_pcap_path, as_pcl_structs=True))
+    pcds_contiguous = list(vd.read_pcap(sample_pcap_path, as_pcl_structs=False))
+    assert len(pcds_struct) == len(pcds_contiguous)
+    for (stamp_struct, pcd_struct), (stamp_contiguous, pcd_contiguous) in zip(pcds_struct, pcds_contiguous):
+        assert stamp_struct.host == stamp_contiguous.host
+        assert stamp_struct.device == stamp_contiguous.device
+        cast_struct_pcd = _struct_pcd_to_contiguous(pcd_struct)
+        assert np.allclose(cast_struct_pcd, pcd_contiguous, rtol=1e-6)
diff --git a/test/test_ros.py b/test/test_ros.py
index 73acfd0..5925269 100644
--- a/test/test_ros.py
+++ b/test/test_ros.py
@@ -1,6 +1,6 @@
 # Copyright (c) 2021-2023, Martin Valgur
 # SPDX-License-Identifier: BSD-3-Clause
-
+import numpy as np
 import velodyne_decoder as vd
 
 
@@ -13,6 +13,8 @@ def test_bag_as_contiguous_array(sample_bag_path):
     assert pcd.shape == (27300, 8)
     assert pcd.dtype.name == "float32"
     assert frame_id == "velodyne"
+    # Sanity check that the point clouds are actually distinct
+    assert not all(len(x) == len(pcd) for _, x, _, _ in pcds)
 
 
 def test_bag_as_struct_array(sample_bag_path, pcl_struct_dtype):
@@ -24,6 +26,7 @@ def test_bag_as_struct_array(sample_bag_path, pcl_struct_dtype):
     assert pcd.shape == (27300,)
     assert pcd.dtype == pcl_struct_dtype
     assert frame_id == "velodyne"
+    assert not all(len(x) == len(pcd) for _, x, _, _ in pcds)
 
 
 def test_bag_automatic_topic(sample_bag_path):
@@ -40,3 +43,23 @@ def test_bag_time_range(sample_bag_path):
     t0 = 1636622717
     pcds = list(vd.read_bag(sample_bag_path, time_range=(t0, None)))
     assert pcds[0].stamp.host >= t0
+
+
+def _struct_pcd_to_contiguous(pcd_struct):
+    shape = (len(pcd_struct), len(pcd_struct.dtype.fields))
+    converted_pcd = np.empty(shape, dtype=np.float32)
+    for i, field in enumerate(pcd_struct.dtype.fields):
+        converted_pcd[:, i] = pcd_struct[field]
+    return converted_pcd
+
+
+def test_pcap_structs_match_contiguous(sample_bag_path):
+    """Check that the contents of as_pcl_structs=True/False match."""
+    pcds_struct = list(vd.read_bag(sample_bag_path, topics="/velodyne_packets", as_pcl_structs=True))
+    pcds_contiguous = list(vd.read_bag(sample_bag_path, topics="/velodyne_packets", as_pcl_structs=False))
+    assert len(pcds_struct) == len(pcds_contiguous)
+    for (stamp_struct, pcd_struct, _, _), (stamp_contiguous, pcd_contiguous, _, _) in zip(pcds_struct, pcds_contiguous):
+        assert stamp_struct.host == stamp_contiguous.host
+        assert stamp_struct.device == stamp_contiguous.device
+        cast_struct_pcd = _struct_pcd_to_contiguous(pcd_struct)
+        assert np.allclose(cast_struct_pcd, pcd_contiguous, rtol=1e-6)