Support for MSVC 2013 -- Take 2

[rovarma]

I needed VS2013 support, but noticed that the pull request for it (#182) is in a bit of a limbo state. As @cboulay suggested in the comments, I took his original changes and finished them; the credit for the CMake work goes to him.

I've further extended his CMake changes and fixed some code which, as far as I can tell, never should have compiled/worked in the first place (?).

With this, the VS2013 solution is correctly generated and all projects now build without warnings or errors (I have not tested the bindings because I don't know how).

Note: I've tried to keep all my cmake changes in IF(MSVC), so it shouldn't affect other platforms/compilers, but I don't have a linux or osx system to test on, so I can't be sure.

[thp]

Thanks for the work; that's a big change. I could start by cherry-picking some changes and then review the rest? There are nearly 40.000 lines removed and only 1.300 added; is this intended?

[cboulay]

@thp , the reason for the almost 40k lines removed is that the libusb source has been removed and replaced with a submodule. A large chunk of the added 1.3k lines is adding pthreads-w32 to external.

If you deem it necessary, it might be possible for @rovarma to first do the libusb replacement (and corresponding changes to the build system) as a separate pull request (A), then do the pthreads addition by itself as another (B, based on A), and finally this PR (C, based on B). Then you can more easily browse the detailed changes to psmoveapi after you've merged B (> A).

One problem with this is that 'B' will be pointless without the changes in C so you'll have to merge B on faith that C is forthcoming.

[rovarma]

@cboulay already explained what the reason is for the removal of 40k lines, but to summarize what's changed at a high level:

• LibUSB has been removed from the repository and has been added as a submodule (this is where most of the 40k lines removed comes from)
• The CMake files have been split up into multiple files per directory. This is where some of both the removal and addition of new lines comes from. Actually, now that I'm browsing the other pull requests, I think this is probably the same change as described here: Separated CMakeLists into self-contained files in subdirectories. #180
• MSVC13 compiles with warning level 4. This led to a lot of warnings, which I've fixed. Most of these warnings were of type "C4244: conversion from 'type1' to 'type2', possible loss of data". The fixes for these just entailed adding lots of explicit casts all over the place.
• SDL2 has been added as a submodule
• pthreads-w32 has been added to the repository. I've been looking at the reason for that and I don't think it's actually neccessary; it's a bit of a heavy-weight dependency for something that's not used much in the code. It should be possible to remove it and just use Win32 threading primitives directly (probably with some platform wrapper around it) in the few places that need it. If you prefer to keep this PR as small as possible, I can probably remove pthreads altogether and add it to this PR (or do that in a separate PR)
• getopt.c and unistd.c have been added to (I believe) support some linux-specific functionality on Win32. Like pthreads, I believe we can probably do without them as well.

Altogether, if you disregard the removal of libusb and addition of pthreads, this is actually a surprisingly small change. I can imagine though that it's a bit hard for you to review in its current form; I'm open to redoing this in a way that makes it easier for you.

I could do the thing @cboulay suggested (issueing seperate pull requests), but I'd need someone to walk me through how to do that; I'm a complete Git(Hub) noob.

[cboulay]

You're absolutely right that pthreads-w32 is not needed. I actually don't have it in my fork's master anymore. But getting rid of pthreads-w32 means lots of little changes to the code that makes it a bit harder to evaluate. Same with getopt.c and unistd.c. That's why my simpler_msvc branch has pthreads-w32 still. Getting rid of pthreads was to be the next step after the first attempt was merged.

I haven't tested it exactly, but your git flow would look something like the following (please backup the directory before attempting this):

1. git checkout master
2. git checkout -b pristine_upstream
3. git remote add upstream https://github.com/thp/psmoveapi.git
4. git fetch upstream
5. git reset --hard upstream/master # Now your pristine_upstream branch is in the same state as thp/master
6. git checkout -b libusb_submodule
7. Do the libusb submodule changes.
8. git checkout -b win32_posix
9. Do the changes so you can use posix-like functions in win32. Whether this is adding libraries or creating platform-dependent wrappers is up to you.
10. Depending on the order of your commits in your vs2013 branch, this next step may be really easy using rebase, or it may be really hard.
    • You can try using rebase
      • git checkout vs2013
      • git checkout -b vs2013_b
      • git rebase win32_posix
      • Hopefully you will have very few conflicts to deal with.
    • If using rebase gives you many conflicts, it's probably easier to just do the changes manually.
      • git checkout win32_posix
      • git branch -D vs2013_b
      • git checkout -b vs2013_b
      • Do the remaining changes.
11. git checkout vs2013
12. git reset --hard vs2013_b
13. git push -f # This overwrites the vs2013 branch remotely, and also will update this pull request.
14. git branch -d vs2013_b # Delete vs2013_b locally
15. git push --all -u # Pushes the remaining new branches to the remote.
16. Go to github and create pull requests from each of the new branches.

When thp views the pull requests, the libusb_submodule PR will have only the changes in that branch, the win32_posix will have the libusb_submodule changes AND the changes from its branch, and the last PR will have all the changes. Viewing the vs2013 PR will be like viewing the present PR, unless he first merges the previous PRs or he uses github URL trickery to compare the vs2013 PR against (e.g.) the win32_posix PR.

[nitsch]

If you deem it necessary, it might be possible for @rovarma to first
do the libusb replacement (and corresponding changes to the build
system) as a separate pull request (A), then do the pthreads addition
by itself as another (B, based on A), and finally this PR (C, based
on B). Then you can more easily browse the detailed changes to
psmoveapi after you've merged B (> A).

BTW, since this exact same problem with submodules comes up from time to
time, we might consider solving this by either using git subtrees
instead of submodules or by handling the dependencies in the build
configuration step instead (using CMake's external projects for
instance). I actually planned to use the latter approach for a locally
built OpenCV, but never found the time to actually do it.

[rovarma]

@cboulay Thanks for the detailed instructions. It seems like quite a bit of work (having to redo the changes in different branches, basically). I'll leave it up to @thp if he wants me to do that or not.

@nitsch Actually, one of the changes @cboulay has made as part of the CMake refactor is that OpenCV can now be a locally built version. I believe it first searches for a version in /external/ and if it can't find it there, use the system version.

[nitsch]

@nitsch Actually, one of the changes @cboulay has made as part of the
CMake refactor is that OpenCV can now be a locally built version. I
believe it first searches for a version in /external/ and if it
can't find it there, use the system version.

The option for a locally built OpenCV was part of the build system
already. This feature is not new and also not what I meant. I was only
talking about a way to implement it differently using Git and CMake.

[rovarma]

@nitch Okay -- I misunderstood what you meant then.
@cboulay Regarding OpenCV -- In the msvc readme you mention that on Windows, PSMoveAPI only works with OpenCV 2.4. What were the problems you were seeing with using OpenCV 3.0? I haven't tried using 3.0 myself (yet), but it would be nice if we could solve those issues and use the same OpenCV version for all platforms.

[cboulay]

@rovarma , Honestly, I can't remember. And that was at a time when OpenCV3 was still in beta. It might work now. You'll probably have to make some changes, especially in the cmake files, but it may work now.

[rovarma]

@cboulay Okay. I tested OpenCV 3.0 and while it builds & links fine (no changes needed to the CMake files), it crashes when running test_tracker in cvGetSize during psmove_tracker_blinking_calibration.

Looking at the disassembly, it appears that incorrect code is generated for cvGetSize (it's reading the argument to cvGetSize from the wrong register, causing it to basically work with a garbage pointer). This is probably due to some disagreement between PSMoveAPI and OpenCV 3.0 about what the memory layout of CvSize is, causing caller and callee not to match up during code gen.

I haven't tracked it down exactly yet, but it should be relatively straightforward to find and fix. It's not a high priority for me right now, but I might have a look at it over the weekend.

@thp Any news on how you want to proceed with this PR?

[thp]

Reviewing this one right now