vectr-ucla / direct_lidar_odometry

[IEEE RA-L & ICRA'22] A lightweight and computationally-efficient frontend LiDAR odometry solution with consistent and accurate localization.
MIT License
882 stars 186 forks source link

getSubmapKeyframes() #7

Closed narutojxl closed 2 years ago

narutojxl commented 2 years ago

Hello authors, Thanks for sharing this excellent work to let us study firstly :) In getSubmapKeyframes() we call computeConvexHull() to calculate the keyframes indice whose position(xyz) consist of convex hull, we then push the indices into keyframe_convex. Now let we suppose keyframe_convex's content is keyframe_convex = [3, 6, 8, 10, 13]. We then push the distance between curr scan position to each of keyframe_convex position one by one from begining into convex_ds. Let we suppose convex_ds is convex_ds =[1.0, 0.2, 1.4, 0.3, 3.0]. Then in pushSubmapIndices() we calculate submapkcv smallest elements, which represent submapkcv nearest convex keyframes indices from current scan position. Let we suppose submapkcv = 2. In pushSubmapIndices distance 0.2 and 0.3 will be sorted, so we push indice = 1 and indice = 3 into submap_kf_idx_curr, but i think we should push the corresponding indices in keyframe_convex namely 6 and 10 into submap_kf_idx_curr, which are distance 0.2 and 0.3 correspond keyframe indices, right?
If i mistake somewhere code please correct me. If I'm right, the same problem also in pushSubmapIndices() of computeConcaveHull(). BTW, this->pose in L1252 is last scan position in global frame, why not use the current scan's guess value in global frame, namely this->T_s2s, we get from this->propagateS2S(T_S2S);. Maybe the robot movement is small and lidar frequency is 10hz, we can ignore this effect. Thanks for your time and help very much in advance!

kennyjchen commented 2 years ago

Hi @narutojxl -- thanks for your interest in our work!

Great catch -- the indices should be extracted from the original list of convex/concave keyframes (rather than from the distance vector). Your second point is also correct in that we should be using the most current positional estimate during the search (although in practice this difference is most likely negligible).

Thanks for the keen eye! We've fixed both of these bugs in v1.3.0 and have credited you accordingly. Let us know if there are any other issues.

narutojxl commented 2 years ago

Thanks author, close now.