Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed optimization bug under degeneracy #96

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KitKat7
Copy link
Contributor

@KitKat7 KitKat7 commented Nov 2, 2018

According to the paper, "J. Zhang, M. Kaess and S. Singh, On degeneracy of optimization-based state estimation problems, 2016 IEEE International Conference on Robotics and Automation (ICRA), Stockholm, 2016", when degeneracy happens, the update should be remapped by "matV.inverse() * matV2".

However, in the original paper Vu and Vf are transposed matrix of eigenvalues, which should correspond to matV2 and matV in the current implementation a4c364a.

Thus, to update the states towards well-conditioned directions only, "matV.transpose().inverse() * matV2.transpose()" should be used, or equivalently "matV2 * matV.inverse()".

@ojura
Copy link

ojura commented Nov 20, 2018

I tried out your PR and it helped with wild pose divergences indoors (VLP-16). They still happen (especially in confined spaces), but less often.

@KitKat7
Copy link
Contributor Author

KitKat7 commented Nov 27, 2018

Glad it helped :).
IMHO, this approach will prefer to make the weights of the degeneracy parts (eigen vectors) close to the ones from predictions.
In some confined spaces, the predictions (constant velocity assumption or erroneous laser odometry poses) may be incorrect. Using only the constraints from the sparse point clouds can cause the alignment slip.

@ojura
Copy link

ojura commented Nov 27, 2018

Indeed, thank you :) Yup, this implementation trusts the IMU too little. It only uses the gyro/accelerometer to unwarp the point cloud, even though the IMU provides a stable pitch/roll reference with respect to gravity which does not drift.

@ojura
Copy link

ojura commented Nov 29, 2018

@KitKat7 can you PTAL at this line? Do you think that the transform should be scaled according to scan time (s = (1.f / _scanPeriod) * (pointOri.intensity - int(pointOri.intensity)) like in BasicLaserOdometry::transformToStart), instead of s = 1?

@KitKat7
Copy link
Contributor Author

KitKat7 commented Nov 30, 2018

Actually, the s=1 here does not make any effects. I think some scale can be considered, but at least not according to the scan time, since this s is for weighting for the optimization instead of of point cloud undistortion.

@ojura
Copy link

ojura commented Nov 30, 2018

Hmmm, are you sure it's just scaling? The angle inside the sines/cosines a couple of lines below is also multiplied by s.

If the costs d (inside coeff.intensity) are calculated by transforming the point by s fraction of transform according to the point scan time, shouldn't the point scan time also appear in the Jacobian of the costs?

@KitKat7
Copy link
Contributor Author

KitKat7 commented Dec 10, 2018

I think in the original paper, the point scan time is not optimized here. The scan time of the point is corrected before the optimization in transformToStart .

@tungdanganh
Copy link

Hi @KitKat7,
I think you are right because the eigenvectors are in column order in this implementation. However, we have to change also in the for loop. We could simply transpose the eigenvectors in begining and keep the same like previous version:
matV = esolver.eigenvectors().real();
matV = matV.transpose();

@ojura
Copy link

ojura commented Jan 24, 2019

@tungdanganh You need to use "transposeInPlace()". https://eigen.tuxfamily.org/dox/group__TutorialMatrixArithmetic.html

@narutojxl
Copy link

narutojxl commented May 12, 2020

Hi, @KitKat7,
I have some stupid questions about this section, If you have some time to have a look, i will be much appreciated a lot !
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants