gisbi-kim / SC-A-LOAM

Robust LiDAR SLAM with a versatile plug-and-play loop closing and pose-graph optimization.
438 stars 95 forks source link

Does the line "loopFindNearKeyframesCloud(cureKeyframeCloud, _curr_kf_idx, 0, _loop_kf_idx);" works as it should be? #7

Closed QiMingZhenFan closed 2 years ago

QiMingZhenFan commented 2 years ago

Hi, thanks for your excellent contribution!

When I learned your code, I wondered if line 433 in laserPosegraphOptimization.cpp(mentioned below) works well as it's supposed to be. In my opinion, we should extract the point cloud in the global frame corresponding to "_curr_kf_idx" in this step. However, the function "loopFindNearKeyframesCloud" doesn't use the second parameter. Is there a mistake or I have a wrong understanding?

Looking forward to your reply. Thanks a lot!

void loopFindNearKeyframesCloud( pcl::PointCloud<PointType>::Ptr& nearKeyframes, const int& key, const int& submap_size, const int& root_idx)
{
    // extract and stacking near keyframes (in global coord)
    nearKeyframes->clear();
    for (int i = -submap_size; i <= submap_size; ++i) {
        int keyNear = root_idx + i;
        if (keyNear < 0 || keyNear >= int(keyframeLaserClouds.size()) )
            continue;

        mKF.lock(); 
        *nearKeyframes += * local2global(keyframeLaserClouds[keyNear], keyframePosesUpdated[keyNear]);
        mKF.unlock(); 
    }

    if (nearKeyframes->empty())
        return;

    // downsample near keyframes
    pcl::PointCloud<PointType>::Ptr cloud_temp(new pcl::PointCloud<PointType>());
    downSizeFilterICP.setInputCloud(nearKeyframes);
    downSizeFilterICP.filter(*cloud_temp);
    *nearKeyframes = *cloud_temp;
} // loopFindNearKeyframesCloud

https://github.com/gisbi-kim/SC-A-LOAM/blob/f088000dfc383937609df416523a38fab06d36d0/src/laserPosegraphOptimization.cpp#L433

gisbi-kim commented 2 years ago

For line 433, I had expected it works like virtual icp between two point clouds More details, I previously explained here https://github.com/irapkaist/SC-LeGO-LOAM/issues/16 I hope this would be an answer to you, and feel free to ask more questions if you have.

QiMingZhenFan commented 2 years ago

Thanks for your reply! @gisbi-kim I followed the link you mentioned above and understood the fact that adding a loop constraint is equal to the case of consecutive frame-to-frame ICP. But in my opinion, what i mean is quite different from what is in your link.

loopFindNearKeyframesCloud(cureKeyframeCloud, _curr_kf_idx, 0, _loop_kf_idx); // use same root of loop kf idx loopFindNearKeyframesCloud(targetKeyframeCloud, _loop_kf_idx, historyKeyframeSearchNum, _loop_kf_idx);

In the code above, the cureKeyframeCloud is a point cloud scan corresponding to the keyframe index _loop_kf_idx. And the targetKeyframeCloud is a submap which consists of scans from many keyframe poses surrounding the _loop_kf_idx keyframe pose. So you use ICP to calculate the transform between the target keyframe (an old keyframe where loop closure was found between the current keyframe and it) and the submap surrounding the target keyframe. Should there be a transform between the current keyframe and the submap surrounding the target keyframe?

gisbi-kim commented 2 years ago
QiMingZhenFan commented 2 years ago

@gisbi-kim Sorry for my poor English, and thanks for your immediate reply!

Yes! This plot is the same as what I thought. I try to explain myself more clearly. It seems that line 433 doesn't extract the scan relative to the _curr_kf_idx keyframe. Because the function loopFindNearKeyframesCloud doesn't use the second parameter. So the point cloud which is extracted by line 433 is actually the scan relative to _loop_kf_idx.

gisbi-kim commented 2 years ago

@QiMingZhenFan I got it!! you are right and this is an error. I'll fix it asap. thanks for the reporting! I'll replace the root_idx in the line 407 to "key"

ps.