libmv C API clean up

Non-functional clean up of some inconsistencies and mistakes in the libmv C API. Identifiers now have a common style, and libmv_structNameVerb has been preferred (when applicable) so the function names have some heirarchy. Feature detection has been moved to the top since it typically occurs before tracking. A number of spelling mistakes have also been corrected.

[sergey.vfx, 2013-07-28 21:53:25]

Overall:

- Naming changes seems to be fine.
- Reshuffling chunks of code is not a good idea, since it breaks
troubleshooting.
- If you really want to clean stuff up, you'll also probably want to change
style to blender's one instead of google's one.

Maybe there's something else, didn't have enough time to look deeper into the
change.

https://codereview.appspot.com/11494044/diff/4001/extern/libmv/libmv-capi.cc
File extern/libmv/libmv-capi.cc (right):

https://codereview.appspot.com/11494044/diff/4001/extern/libmv/libmv-capi.cc#...
extern/libmv/libmv-capi.cc:268: /* ************ Feature detector ************ */
Is there a real reason to reorder such a huge chunk of code? It's gonna to break
line history which is not good for troubleshooting at all.

Same goes to some re-orders happening later.

https://codereview.appspot.com/11494044/diff/4001/extern/libmv/libmv-capi.h
File extern/libmv/libmv-capi.h (right):

https://codereview.appspot.com/11494044/diff/4001/extern/libmv/libmv-capi.h#n...
extern/libmv/libmv-capi.h:34: typedef struct libmv_Features libmv_Features;
I don't think you could typedef anonymous structure like this. At least  it's
not gonna to work in some compilers we need to support.

https://codereview.appspot.com/11494044/diff/4001/extern/libmv/libmv-capi.h#n...
extern/libmv/libmv-capi.h:45: libmv_Features *libmv_detectFeaturesFAST(const
unsigned char *data, int width, int height, int stride,
Same as before. I'd say it shall be good reason to reshuffle the whole blocks of
code.

[sftrabbit, 2013-07-28 23:09:22]

On 2013/07/28 21:53:25, sergey.vfx wrote:
> Overall:
> 
> - Naming changes seems to be fine.
> - Reshuffling chunks of code is not a good idea, since it breaks
> troubleshooting.
> - If you really want to clean stuff up, you'll also probably want to change
> style to blender's one instead of google's one.
> 
> Maybe there's something else, didn't have enough time to look deeper into the
> change.
> 
> https://codereview.appspot.com/11494044/diff/4001/extern/libmv/libmv-capi.cc
> File extern/libmv/libmv-capi.cc (right):
> 
>
https://codereview.appspot.com/11494044/diff/4001/extern/libmv/libmv-capi.cc#...
> extern/libmv/libmv-capi.cc:268: /* ************ Feature detector ************
*/
> Is there a real reason to reorder such a huge chunk of code? It's gonna to
break
> line history which is not good for troubleshooting at all.
> 
> Same goes to some re-orders happening later.
> 
> https://codereview.appspot.com/11494044/diff/4001/extern/libmv/libmv-capi.h
> File extern/libmv/libmv-capi.h (right):
> 
>
https://codereview.appspot.com/11494044/diff/4001/extern/libmv/libmv-capi.h#n...
> extern/libmv/libmv-capi.h:34: typedef struct libmv_Features libmv_Features;
> I don't think you could typedef anonymous structure like this. At least  it's
> not gonna to work in some compilers we need to support.
> 
>
https://codereview.appspot.com/11494044/diff/4001/extern/libmv/libmv-capi.h#n...
> extern/libmv/libmv-capi.h:45: libmv_Features *libmv_detectFeaturesFAST(const
> unsigned char *data, int width, int height, int stride,
> Same as before. I'd say it shall be good reason to reshuffle the whole blocks
of
> code.

I hadn't thought about the whole breaking history thing - good point!

What do you mean about the style? I went for the C API style that we decided on:
lib_TypeName and lib_functionName. This is really neither Blender nor Google
style. :P

[sergey.vfx, 2013-07-29 01:58:33]

By the style i've meant:

  if (blender_style) {

versus:

  if(libmv_style) {


Not a big deal, but if you'll feel like wanting to do some cleanup, you can do
it. And here's some stuff you might find useful for this:

- Blender's style guide: http://wiki.blender.org/index.php/Dev:Doc/CodeStyle
- Blender's style check script:
https://svn.blender.org/svnroot/bf-blender/trunk/blender/source/tools/check_s...

[sftrabbit, 2013-07-29 10:46:42]

On 2013/07/29 01:58:33, sergey.vfx wrote:
> By the style i've meant:
> 
>   if (blender_style) {
> 
> versus:
> 
>   if(libmv_style) {
> 
> 
> Not a big deal, but if you'll feel like wanting to do some cleanup, you can do
> it. And here's some stuff you might find useful for this:
> 
> - Blender's style guide: http://wiki.blender.org/index.php/Dev:Doc/CodeStyle
> - Blender's style check script:
>
https://svn.blender.org/svnroot/bf-blender/trunk/blender/source/tools/check_s...

Cool, I didn't know about the script. I've changed everything to blender style.
Mostly involved aligning arguments and putting spaces around operators.

Do you think it's ready to commit to trunk?

[sergey.vfx, 2013-07-30 17:57:12]

Overall seems fine for me. Once the only note is addressed it's fine for commit.

https://codereview.appspot.com/11494044/diff/20001/extern/libmv/libmv-capi.cc
File extern/libmv/libmv-capi.cc (right):

https://codereview.appspot.com/11494044/diff/20001/extern/libmv/libmv-capi.cc...
extern/libmv/libmv-capi.cc:65: struct libmv_Reconstruction {
Afraid we're running into misunderstanding here.

The idea was to keep non-typedef-ed structures for those guys who're using C-API
from C code. They don't know how how structure looks like and couldn't
de-reference it.

Here structure is local and explicitly defined, And i would keep it typedef-ed.

[sftrabbit, 2013-07-30 19:43:39]

On 2013/07/30 17:57:12, sergey.vfx wrote:
> Overall seems fine for me. Once the only note is addressed it's fine for
commit.
> 
> https://codereview.appspot.com/11494044/diff/20001/extern/libmv/libmv-capi.cc
> File extern/libmv/libmv-capi.cc (right):
> 
>
https://codereview.appspot.com/11494044/diff/20001/extern/libmv/libmv-capi.cc...
> extern/libmv/libmv-capi.cc:65: struct libmv_Reconstruction {
> Afraid we're running into misunderstanding here.
> 
> The idea was to keep non-typedef-ed structures for those guys who're using
C-API
> from C code. They don't know how how structure looks like and couldn't
> de-reference it.
> 
> Here structure is local and explicitly defined, And i would keep it
typedef-ed.

libmv_Reconstruction is defined in the .cc file though, so its structure is not
known anywhere else. As far as any of the rest of Blender is concerned,
libmv_Reconstruction and libmv_Tracks (for example) are both just forward
declarations and pointers to them can't be dereferenced. I think it would be
confusing for C API users if one were typedefed and the other wasn't because
they can't tell the difference. It's an implementation detail that one of them
happens to alias a C++ class and the other is local to a particular translation
unit. They're both just handles to internal types. I think they should either
all be typedefed or all not, not a mix of both.

[sergey.vfx, 2013-07-31 03:16:17]

Valid point. Let's go with current patch then. If it'll become annoying in the
future we could change it any time.

So, let's commit this patch and go having fun with real stuff :)