MRPT / mrpt

:zap: The Mobile Robot Programming Toolkit (MRPT)
https://docs.mrpt.org/reference/latest/
BSD 3-Clause "New" or "Revised" License
1.89k stars 627 forks source link

Covariance mapping to SE(3) for GTSAM #1241

Closed miguelcastillon closed 2 years ago

miguelcastillon commented 2 years ago

Changed apps/libraries

PR Description

Comments

Hi! I'm very new to contributing to large repositories. I have read the docs on how to do it but I fear I may have done some steps not fully right. If there's anything else needed from my side, please let me know. Cheers, Miguel


I acknowledge to have:

(Notify: @MRPT/owners )

jlblancoc commented 2 years ago

I'll review in depth soon, thanks! Please add also an entry to the changelog.md with the change, or let me know if you can't find it (I'm on the phone now....)

codecov[bot] commented 2 years ago

Codecov Report

Merging #1241 (044ee2b) into develop (0d162d5) will decrease coverage by 0.00%. The diff coverage is n/a.

:exclamation: Current head 044ee2b differs from pull request most recent head 45f536d. Consider uploading reports for the commit 45f536d to get more accurate results

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1241      +/-   ##
===========================================
- Coverage    39.24%   39.24%   -0.01%     
===========================================
  Files         1292     1292              
  Lines       109832   109831       -1     
===========================================
- Hits         43108    43106       -2     
- Misses       66724    66725       +1     
Impacted Files Coverage Δ
libs/slam/src/slam/se3_ransac_unittest.cpp 98.48% <0.00%> (-1.52%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0d162d5...45f536d. Read the comment docs.

jlblancoc commented 2 years ago

Ey! I missed something important in my earlier first look: there are two gtsam_wrappers.h files in mrpt, one under mrpt/math and another under mrpt/poses (two different libs, and "poses" depends on "math", so to say "math" provides "more basic/fundamental" stuff).

So, you cannot use CPose3D or any other class from #include <mrpt/poses/xxx.h> in this gtsam_wrapper file under mrpt/math. Instead, keep here everything that has to do with mrpt::math:: stuff (like the Quaternion), and move the rest to this one, please. If needed, add #include <mrpt/math/gtsam_wrappers.h> to mrpt/poses/gtsam_wrappers.h so you can use new functions you added here in that header.

Let me know whether I explained it well enough or not! :smile:

PS: The final goal is have the library as modular as possible, so someone could only install sudo apt install libmrpt-poses-dev or libmrpt-math-dev and have exactly what he needs only.

miguelcastillon commented 2 years ago

Ah my bad! I hope it's correct now :)

Btw, I saw all the tests passed but the clang format. However, I don't understand why. I tried to look for a codying style file but the link in the contributing doc is broken.

miguelcastillon commented 2 years ago

Thank you for your time José Luis. I just updated the files according to your recommendations, let me know how it looks to you. About clang, the test was successful in the last commit already, maybe there had been a small error in the first commit.

jlblancoc commented 2 years ago

Awesome, thanks for your contribution and your patience! ;-)

PS: Yes, the .asEigen() is safe to use within headers expected to be used along with GTSAM, since the latter 100% depends on the existence of Eigen in its #includes .