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
Add surface_matching module. #70
Add surface_matching module. #70
Conversation
Apart from not displaying the table, it breaks the doc build of opencv/opencv_contrib#70.
The doc build will be fixed when opencv/opencv#3133 is merged. |
@vpisarev , could you please take a look at this? |
@vpisarev All green. Can you please review the code? I can squash the commits before you merge the PR. We are working on some further improvemens but this could still go through now as it adds basic functionalities and docs. |
@@ -0,0 +1,163 @@ | |||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please, move icp.hpp and other headers to include/opencv2/surface_matching subdirectory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@vpisarev please don't review it yet, I have to make some order in the branch of the PR, the last merge/rebase this morning made an awful mess. This is not the final version at all. I will ping you again when it's ready and neat. Sorry for the inconvenience. |
* and whether it should be loaded or not | ||
* \return Returns the matrix on successfull load | ||
*/ | ||
CV_EXPORTS cv::Mat loadPLYSimple(const char* fileName, int withNormals); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to use cv:: when you are already inside cv namespace. Extra non-alphabetic characters make the code less readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Squashed commits, from now on only fixes for your comments will come. @vpisarev please proceed. |
// | ||
// Author: Tolga Birdal <tbirdal AT gmail.com> | ||
|
||
#ifndef _OPENCV_POSE3D_HPP_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we use some common prefix for the guard macros like
_ _ OPENCV_SURFMATCH_POSE3D_HPP _ _?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
I still see that 2 headers, icp.hpp and ppf_match_3d.hpp are not inside surfacematch subdirectory. Otherwise, the PR seems to be pretty much ready for integration |
I will work a bit more on removing not needed members and arguments of the API classes and make sure that Ptr changes are harmless. |
ok, to me it looks pretty much ready for integration; @bmagyar, if you are done with the changes, I will merge it in. |
The code works as it is now. If the last doc build passes, you can merge it if you want. I will open a new PR with further refinements. I almost got rid of all the pointers but I'm too pedantic to leave them there, I also have some more ideas for other parts... |
looks like doc builder failed; could you correct it? |
Docs should be good now. On 2 September 2014 19:04, Vadim Pisarevsky notifications@github.com
Bence Magyar Robotics Engineer Pal Robotics, S.L. Facebook http://www.facebook.com/palrobotics1 - Twitter P Antes de imprimir este e-mail piense bien si es necesario hacerlo: El AVISO DE CONFIDENCIALIDAD: Este mensaje y sus documentos adjuntos, pueden |
All greeen, sample runs fine here. |
👍 |
thanks! 👍 |
Please define the keytype as follows: #if (defined x86_64 || defined _M_X64) This will be fixed. All the bests,
|
Thank you! I fixed it by replacing "murmurHash" with "hashMurmurx86", as written here #170 |
Hello, I am new at C++. Can you tell me in which file I should add those lines? Thank you |
This is the surface_matching module developed by @tolgabirdal for the GSoC 2014.
Video can be found here