How to contribute
Version 3 (Daniil Osokin, 2014-07-14 06:01 pm)
1 | 1 | h1. How to contribute |
|
---|---|---|---|
2 | 1 | ||
3 | 1 | *Under construction* |
|
4 | 1 | ||
5 | 1 | You can: |
|
6 | 1 | # Say "thanks":http://sourceforge.net/projects/opencvlibrary/reviews/. |
|
7 | 1 | # Help others on "forum":http://tech.groups.yahoo.com/group/OpenCV. |
|
8 | 1 | # Provide feedback: report bugs, request features. |
|
9 | 1 | # Submit a code: create a ticket with the _Patch_ tracker (issue type). |
|
10 | 2 | Daniil Osokin | # -Donate- You should wait for the OpenCV Foundation. |
11 | 3 | Daniil Osokin | |
12 | 3 | Daniil Osokin | *Below are recommendations for some contributors. They are raw, but can be used as a starting point for anybody who wants to see his code added to the library.* |
13 | 3 | Daniil Osokin | |
14 | 3 | Daniil Osokin | Hello! |
15 | 3 | Daniil Osokin | |
16 | 3 | Daniil Osokin | We would gladly accept such a contribution, but because of little resources available on our side, I would ask you to do all the preparations. Here is the checklist, including both generic items and the specific things related to your code that I found after brief inspection: |
17 | 3 | Daniil Osokin | |
18 | 3 | Daniil Osokin | 0. A prerequisite: is the algorithm patented? If yes, then we may not be that interested in putting it in. |
19 | 3 | Daniil Osokin | |
20 | 3 | Daniil Osokin | 1. License - the current license is fine, it's compatible with OpenCV. |
21 | 3 | Daniil Osokin | |
22 | 3 | Daniil Osokin | 2. Interface. Here some work is needed to make this code a seamless part of OpenCV. |
23 | 3 | Daniil Osokin | - @M_PI -> CV_PI@ |
24 | 3 | Daniil Osokin | - remove all the static constants from the header. it's not portable and generally a bad practice. Use defines (which should have prefix CV_FREAK_ or something) and enumerations. |
25 | 3 | Daniil Osokin | Do not use k prefix in constant names. Put enumerations inside FreakDescriptorExtractor class. |
26 | 3 | Daniil Osokin | - "typedef unsigned char uint8" should be removed. it's duplication of "uchar" OpenCV type. |
27 | 3 | Daniil Osokin | - structures PatternPoint, DescriptionPair, OrientationPair, PairStat, sortMean should be removed from the header |
28 | 3 | Daniil Osokin | - cv:: and std:: should not be used in the header, since all the stuff is already inside cv namespace and we already have "using std::vector", "using std::string" directives. |
29 | 3 | Daniil Osokin | - "CV_INLINE" in "uchar meanIntensity(...)" declaration should be dropped. |
30 | 3 | Daniil Osokin | - "m_" in member names should be dropped, we do not this convention in OpenCV. |
31 | 3 | Daniil Osokin | |
32 | 3 | Daniil Osokin | - m_patternLookup, m_descriptionPairs, m_orientationPairs can be turned into Mat's or be put (probably, with some other members) into hidden structure: |
33 | 3 | Daniil Osokin | |
34 | 3 | Daniil Osokin | <pre> |
35 | 3 | Daniil Osokin | class CV_EXPORTS FreakDescriptorExtractor : public DescriptorExtractor |
36 | 3 | Daniil Osokin | { |
37 | 3 | Daniil Osokin | ... |
38 | 3 | Daniil Osokin | protected: |
39 | 3 | Daniil Osokin | struct FreakDescriptorExtractorImpl* impl; |
40 | 3 | Daniil Osokin | }; |
41 | 3 | Daniil Osokin | </pre> |
42 | 3 | Daniil Osokin | |
43 | 3 | Daniil Osokin | - reading/writing point pairs from a file looks not very good. First of all, in mobile world reading data from external files may be inefficient and inconvenient. It's also inconvenient for redistribution of final applications that may use this functionality. Why not just drop all the I/O and just pass optional const vector<int>& to the constructor? If the final application is written in Python, Java, C#, Objective-C etc., it's super-easy to read those array from application resources, xml file, a database etc. |
44 | 3 | Daniil Osokin | |
45 | 3 | Daniil Osokin | - drawPattern is purely debugging method, I suggest to remove it from the external interface (make it a method of FreakDescriptorExtractorImpl?) and comment it off in the implementation, so that it can be restored if needed. |
46 | 3 | Daniil Osokin | |
47 | 3 | Daniil Osokin | 3. implementation. |
48 | 3 | Daniil Osokin | |
49 | 3 | Daniil Osokin | - The code should be portable, it should compile fine on ARM too, for example. Therefore, you should not use SSE2 intrinsics outside of conditional compilation. Instead of "#ifdef USE_SSE" you should use "#if CV_SSE2". |
50 | 3 | Daniil Osokin | |
51 | 3 | Daniil Osokin | - Avoiding duplication of existing OpenCV functionality. There is already more or less efficient normHamming() in OpenCV. Please, use it instead of hammingseg.h |
52 | 3 | Daniil Osokin | |
53 | 3 | Daniil Osokin | - Writing to cerr should be replaced with CV_Error() calls. |
54 | 3 | Daniil Osokin | |
55 | 3 | Daniil Osokin | 4. documentation. |
56 | 3 | Daniil Osokin | |
57 | 3 | Daniil Osokin | documentation in RST format should be provided for the functionality. Check http://docs.opencv.org/modules/features2d/doc/feature_detection_and_description.html#orb as example. |
58 | 3 | Daniil Osokin | |
59 | 3 | Daniil Osokin | 5. sample code - you have it already. |
60 | 3 | Daniil Osokin | |
61 | 3 | Daniil Osokin | it needs to be renamed (e.g. to freak_demo.cpp) |
62 | 3 | Daniil Osokin | help() function should be added, check opencv/samples/cpp. |
63 | 3 | Daniil Osokin | the commented out section should probably be uncommented and executed when user passes some optional parameter. |
64 | 3 | Daniil Osokin | no USE_SSE is allowed in samples, they should be written in completely platform-abstract way whenever possible |
65 | 3 | Daniil Osokin | kResPath should be made command line parameter (maybe optional) |
66 | 3 | Daniil Osokin | |
67 | 3 | Daniil Osokin | 6. regression tests - the code should include some regression tests. |
68 | 3 | Daniil Osokin | |
69 | 3 | Daniil Osokin | please, take a look at opencv/modules/features2d/test/test_features2d.cpp, Features2d_DescriptorExtractor_BRIEF or _ORB in particular (since they also use hamming distance). You will need to add a similar test for Freak. |
70 | 3 | Daniil Osokin | |
71 | 3 | Daniil Osokin | 7. add the Algorithm registration. Take a look at features2d_init.cpp. |
72 | 3 | Daniil Osokin | |
73 | 3 | Daniil Osokin | 8. probe the integration. After you've done 1-7, you may do a test integration of the descriptor. |
74 | 3 | Daniil Osokin | |
75 | 3 | Daniil Osokin | Take the latest snapshot of the SVN trunk |
76 | 3 | Daniil Osokin | Add the class FreakDescriptorExtractor to features2d.hpp |
77 | 3 | Daniil Osokin | Add the algorithm registration to features2d_init.cpp |
78 | 3 | Daniil Osokin | Put freak.cpp file to features2d/src/, put #include "precomp.hpp" in the beginning |
79 | 3 | Daniil Osokin | Insert documentation and |
80 | 3 | Daniil Osokin | Add the test to test_features2d.cpp. |
81 | 3 | Daniil Osokin | Build OpenCV, run tests. If you have access to both Linux and Windows - great! Sometimes GCC-approved code does not built with MSVC and vice versa. |
82 | 3 | Daniil Osokin | Create patch file and either send it to me by e-mail or submit it as patch at code.opencv.org. |
83 | 3 | Daniil Osokin | |
84 | 3 | Daniil Osokin | -- |
85 | 3 | Daniil Osokin | It may look like a lot of work to do. And it is. Earlier we did such refactoring and cleanups many times by ourselves. Some contributions were just put as-is, without tests, documentation, samples etc. and now we have no idea what to do with it - either remove completely such immature functionality or put it to legacy and let it die slowly. Not to let this happen again, we decided to raise the bar and accept only well-prepared code into the library. We also have to ask contributors to do this work. |
86 | 3 | Daniil Osokin | |
87 | 3 | Daniil Osokin | Do not hesitate to ask any questions (now and/or during the process, if you decide to go ahead with integration) |
88 | 3 | Daniil Osokin | |
89 | 3 | Daniil Osokin | Regards, |
90 | 3 | Daniil Osokin | Vadim |