izhengfan / se2lam

(ICRA 2019) Visual-Odometric On-SE(2) Localization and Mapping
https://github.com/izhengfan/se2lam
MIT License
404 stars 108 forks source link

代码错误反馈: #15

Closed menglitanhua closed 4 years ago

menglitanhua commented 4 years ago
  1. 建图和定位模式下大多数findFundamentalMat函数使用错误。
  2. Localizer::GetLocalKFs()和Localizer::GetLocalMPs()中所加锁重复定义。
  3. 定位模式中,重定位成功后,直接使用回环帧位姿,结合当前帧与回环帧,当前帧与局部地图帧中地图特征匹配进行两次局部BA优化获得精确定位值,该设计小场景看不出差别,大场景中可能影响定位精度。另外,最好增加当前帧和回环帧共视地图点数目大于15的限制,满足该限制才进行BA。不然大场景中因为重定位假阳,程序可能崩溃。
izhengfan commented 4 years ago

前两错误方不方便指出举例哪一行?

第三个我需要研究下。

menglitanhua commented 4 years ago
  1. Localizer.cpp 文件 601行数 findFundamentalMat(vPtCurr, vPtLoop, FM_RANSAC, 3.0, 0.99, vInlier)中,vInlier的位置应该在第三个参数。GlobalMapper.cpp 1229行。 2.Localizer.cpp 433和439行,去掉mutex关键字,你好像已经改了。
menglitanhua commented 4 years ago

MapPublish.cpp中 378 行 locker lock(mpLocalize->mMutexLocalMap),这一句话也需要删除,锁已经在之后的调用函数中获取,这里不需要重复添加了。

izhengfan commented 4 years ago

findFundamentalMat 是有 overload 的,3.2.0版本的头文件:

CV_EXPORTS_W Mat findFundamentalMat( InputArray points1, InputArray points2,
                                     int method = FM_RANSAC,
                                     double param1 = 3., double param2 = 0.99,
                                     OutputArray mask = noArray() );

/** @overload */
CV_EXPORTS Mat findFundamentalMat( InputArray points1, InputArray points2,
                                   OutputArray mask, int method = FM_RANSAC,
                                   double param1 = 3., double param2 = 0.99 );
izhengfan commented 4 years ago

MapPublish.cpp中 378 行 locker lock(mpLocalize->mMutexLocalMap) 后面调用函数获得的是另外的锁,不是 mMutexLocalMap

menglitanhua commented 4 years ago

ok,没问题了。findFundamentalMat函数确实有两种写法,我习惯第二种,没有求证好轻易误认为错误改掉了。Localizer::GetLocalKFs()和Localizer::GetLocalMPs()中分别加的锁我没太明白你的意图,就被我统一改成了mMutexLocalMap,这样才需要去掉MapPublish.cpp 中的378行代码,否则你在同文件的230行getLocalKF时候是不是也需要加上locker lock(mpLocalize->mMutexLocalMap)这句话?

izhengfan commented 4 years ago

mMutexLocalMap 只是为了方便,涉及所有LocalMap的读写时锁它。反正加锁这种事逻辑对就行,怎么习惯怎么来。

menglitanhua commented 4 years ago

你好,不好意思又要打扰你了,感谢你的及时解答和代码无私奉献给我带来的工作便利。不过,MapPoint.cpp 128-137行,我理解后感觉还是有点疑意,代码如下

    // Get the oldest KF in the last 6 KFs
    PtrKeyFrame pKF0 = mObservations.begin()->first;
    for(auto it = mObservations.begin(), iend = mObservations.end(); it != iend; it++) {
        PtrKeyFrame pKF_ = it->first;
        if(pKF->mIdKF - pKF_->mIdKF > 6)
            continue;
        if(pKF_->mIdKF < pKF0->mIdKF) {
            pKF0 = pKF_;
        }
    }

看你的注释是要获取mObservations中最后添加的6帧中最老的帧(即该6帧中id 最小的帧),比如已添加关键帧Id为0,1,2,3,4,5,6,7,待添加关键帧为8时,pKF0是应该选取id为2的关键帧,map的实现一般是由小到大排序的,你的这种写法应该只能永远返回id为0的关键帧。当然如果你做了mObservations的map结构元素由大到小排序应该就没有问题了。我改成了如下,你看是否合理?

  // Get the oldest KF in the last 6 KFs
    PtrKeyFrame pKF0 = nullptr;
    for(auto it = mObservations.begin(), iend = mObservations.end(); it != iend; it++)
 {
        PtrKeyFrame pKF_ = it->first;
        if(pKF->mIdKF - pKF_->mIdKF > 6)
            continue;
       if(pKF0==nullptr || pKF_->mIdKF < pKF0->mIdKF)
        {
            pKF0 = pKF_;
        }
    }
izhengfan commented 4 years ago

感谢纠错,改好了