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

Fully templated #162

Open
JensMunkHansen opened this issue Sep 23, 2023 · 10 comments
Open

Fully templated #162

JensMunkHansen opened this issue Sep 23, 2023 · 10 comments

Comments

@JensMunkHansen
Copy link

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
Copy link
Owner

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

@JensMunkHansen
Copy link
Author

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

@JensMunkHansen
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Author

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

@JensMunkHansen
Copy link
Author

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
Copy link
Author

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.

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

No branches or pull requests

2 participants