search

gadomski / cpd

C++ implementation of the Coherent Point Drift point set registration algorithm.
http://www.gadom.ski/cpd
GNU General Public License v2.0
385 stars 122 forks source link

Fully templated #162

Open JensMunkHansen opened 9 months ago

JensMunkHansen commented 9 months ago

Hi Gadomski

I have used your implementation in a hobby project of mine (not yet public) where I have wrapped the registration for use with VTK. I went through everything and added additional templates to support both single and double precision.

I have edited everything using VIM, so some reformatting has been done and perhaps not suited for a pull request. I could fork your repository and push it upstream for you to see.

gadomski commented 9 months ago

Please feel free to open a PR, even if it's imperfect -- we can always clean up formatting later. 🍻

JensMunkHansen commented 9 months ago

Will do. I have not used the Fgt library, but I have updated these files as well.

JensMunkHansen commented 9 months ago

How do create an upstream branch in your repository for making a PR? I cannot find any buttons for creating branches. I have local branch and tried to set the upstream, but it just returned permission denied. Could it be a 2FA issue?

JensMunkHansen commented 9 months ago

jmh@DebianX1Extreme:~/github/ICP/cpd$ git push --set-upstream origin fullytemplated ERROR: Permission to gadomski/cpd.git denied to JensMunkHansen. fatal: Could not read from remote repository.

JensMunkHansen commented 9 months ago

It worked using gh. I used clang-format on all the files with the formatting files in your root. There may be a better way, but I decided to simply add Matrix and Vector as template parameters everywhere and use Matrix::Scalar instead of double and float.

gadomski commented 9 months ago

Glad you figured it out. For your awareness, the gh executable created a fork and a branch here: https://github.com/JensMunkHansen/gadomski-cpd/tree/fullytemplated.

There may be a better way, but I decided to simply add Matrix and Vector as template parameters everywhere and use Matrix::Scalar instead of double and float.

Ok, I'll try to see if I can take a look in the next week or two. For your awareness, this is a purely hobby project at this point (I'm not using it for any research anymore) so my responses and reviews may be slow 🙏🏼.

JensMunkHansen commented 9 months ago

So are my projects. I have a 150 kLOC hobby project build upon VTK for SLAM. Also have a faster version for cpd (rigid only), but I find your solution pretty elegant and by making it a bit more generic, it could be easily integrated in my hobby project. I took a little extra time making everything generic. The fgt part I have not compiled - there may a little thing to fix there. Also, many of return types could be replaced with auto and using decltype inside the functions, but that is a matter of taste. I decided to explicit instantiate the templates needed for handling double and float. It is a little easier to resolve template errors this way. Happy coding

JensMunkHansen commented 9 months ago

If you know how to avoid explicitly instantiating the needed templates, I would like to know. There may be a better way.

JensMunkHansen commented 9 months ago

I can see that the FGT library needs to be changed in a similar way for this to work with both float and double.

JensMunkHansen commented 9 months ago

I had no idea what Fgt was, I found it now. I created a templated version of this also. I works with the old version of libflann that you are using. For me it is a little annoying that the LICENSE is GPL. I can make a PR later this week for this library also, but I guess that I have to re-introduce building gtest again. I am not a fan of pulling in to many third-party library if they can simple be installed instead. Let me know, if you want me to make a PR for that also.