64-bit Unity support about osvr-unity

[geary]

Is anyone actively working on this? If not, I'm on it since I need it for my project.

[VRguy]

Michael - go ahead. I am not aware of anyone actively working on it.

You can use our Waffle board https://waffle.io/osvr/osvr-core and assign yourself to this task if you wish.

Thank you!

[rpavlik]

FYI, I just did some work yesterday on the Managed-OSVR portion of the issue, so we do now have a 64-bit-functional version of the .NET bindings. See https://github.com/OSVR/Managed-OSVR and #12 I think we'll just need to do whatever cleanups are deemed necessary on that new repo then port this code to use the new assembly, including build system.

[geary]

Ryan, I should have known you'd be a step ahead of me on this! :-)

With Managed-OSVR off in another project now, it seems that the main thing left is to fix build-unity-packages.cmd to copy the DLL files from that project and put them all in the right places. I went ahead and updated it and am testing the new .unitypackage now. I'll test it in in all nine environments:

Unity 4.6 32-bit editor and 32/64 bit builds
Unity 5 32-bit editor and 32/64 bit builds
Unity 5 64-bit editor and 32/64 bit builds

BTW, build-unity-packages.cmd assumes that this repo and the new Managed-OSVR repo are siblings of each other so it knows where to copy the DLLs from. Does that seem like a reasonable assumption?

[rpavlik]

Well, I just got a little carried away when I split that repo out. 😀 I had all the references noted to handle the size_t issue, and well, I couldn't test it easily unless I gave it some smarts to find the 64-bit native libraries since it apparently wanted to run as 64-bit after cleaning up the build scripts.

Thanks for your help! Note that I think that now, we can probably put the assembly just right in the Plugins directory, since it's "Any CPU", then just put the native libraries in their Unity-standard platform-specific directories. I think I got the right ones on the search paths in Managed-OSVR - see this line for 64-bit and the one a few lines down for 32-bit https://github.com/OSVR/Managed-OSVR/blob/master/ClientKit/ClientKit.cs#L111

[geary]

Yup, that's exactly what I did: the managed DLLs are in Plugins and native DLLs in Plugins/x86 and Plugins/x86_64.

[geary]

Back after a busy weekend... I'm guessing the idea with moving Managed-OSVR into a separate repo is to make it generally useful for other CLR apps beyond Unity, is that right?

In that case (or even if it's just for Unity), I think the x86 vs. x64 path logic needs a bit more work.

In the Unity Editor, native plugin DLLs do go in Plugins/x86 or Plugins/x86_64. But when you make a 32-bit or 64-bit build, Unity copies either the x86 or x86_64 directory into Plugins itself, so everything is in Plugins - there's no x86 or x86_64 directory.

For other CLR apps, I don't think we can assume any particular 32-vs-64-bit directory layout. They may have separate 32-64 bit directories with any arbitrary names, or they may have everything in one directory, or who knows what.

So if Managed-OSVR is general-purpose, it seems that the app using it should pass in the directory where the native DLLs are located instead of Managed-OSVR guessing at it. Also, if it uses SetDllDirectory(), it should use GetDllDirectory() first to get the old value, then immediately load the native DLLs with LoadLibrary("osvrClientKit.dll"), then use SetDllDirectory() again to restore the old value.

The Unity code, OTOH, is modifying the PATH environment variable and adding both Plugins and either Plugins/x86 or Plugins/x86_64. Since only a single one of these directories is required, SetDllDirectory() sounds like a better solution, but actually my usual preference is to do neither of these and instead explicitly load all the needed native DLLs with LoadLibrary(), in reverse order of their dependencies. So the current DLLs would be loaded in this order:

msvcr120.dll
msvcp120.dll
osvrUtil.dll
osvrCommon.dll
osvrTransform.dll
osvrClient.dll
osvrClientKit.dll

Once the DLLs are loaded, the references to them from managed code will pick up the preloaded DLLs instead of trying to load them again. Of course this has a couple of disadvantages: it requires this list of DLLs to be hard coded into the C# code and updated if the list changes, and it's important to do a FreeLibrary() when the app is done (probably not important in a standalone app but matters in the Unity Editor so it doesn't keep the DLLs loaded after the game exits). But it has the advantage of being non-invasive for any other DLL loading the app may do. (Always tradeoffs!)

I experimented with this approach in Unity and it works in all four modes: 32-bit editor, 64-bit editor, 32-bit runtime, 64-bit runtime. I can put together a PR if this approach seems sound.

In any case, much of this complexity comes from having all these separate DLLs. In an ideal world, there would be a single osvrClientKit.dll native library that statically links all of its dependencies including the C/C++ runtime DLLs. It makes me a tiny bit nervous having the runtime DLLs required in any of the Plugins directories - what if another Unity plugin also dynamically links to these and has a slightly different version of them (e.g. a patch release or something)?

The nice thing about a statically linked osvrClientKit.dll would be that it would work automatically with Unity with no PATH or SetDllDirectory() or explicit loading of a list of DLLs, and for other CLR apps it would work with at most an explicit LoadLibrary().

I started to look at OSVR-Core a bit to see what would be required to build osvrClientKit.dll with everything statically linked, but as you know it's a bit of work getting all the other dependencies installed and set up. I did get as far as getting CMake to generate all the project files for OSVR-Core, but figured I'd get your thoughts before doing anything more along those lines. Thanks!

[rpavlik]

On 4/6/2015 2:49 PM, Michael Geary wrote:

Back after a busy weekend... I'm guessing the idea with moving
Managed-OSVR into a separate repo is to make it generally useful for
other CLR apps beyond Unity, is that right?

Yes, essentially, since it was designed to be independent of Unity and
just shared a repository for early development convenience. It'll go up
on NuGet at some point, there is already a MonoGame integration in
progress, and presumably others might want it. The primary consumer is
the Unity plugin, since that has the most users by far at the moment
(most of CLR users and also of overall OSVR client users, I believe),
but it is not the only consumer.

In that case (or even if it's just for Unity), I think the x86 vs. x64
path logic needs a bit more work.

In the Unity Editor, native plugin DLLs do go in Plugins/x86 or
Plugins/x86_64. But when you make a 32-bit or 64-bit build, Unity
copies either the x86 or x86_64 directory into Plugins itself, so
everything is in Plugins - there's no x86 or x86_64 directory.

I think we already handle that case with the path logic in both
managed-osvr and the unity-specific code - if it doesn't find a
platform-specific directory, it falls back on the assembly directory or
the plugins or assets directory, respectively. The effective behavior
implied by that code is that if you want both 64 and 32 bit support,
your libraries (which have the same names on the two platforms) are
separated by being in different subdirectories. If you don't have such
subdirectories, we assume you've already taken care to use/make
available the correct platform version of the native libraries.

For other CLR apps, I don't think we can assume any particular
32-vs-64-bit directory layout. They may have separate 32-64 bit
directories with any arbitrary names, or they may have everything in
one directory, or who knows what.

I agree, and also think that it would presumably be their responsibility
(or that of their runtime) then to make sure we can get to our native DLL(s)

So if Managed-OSVR is general-purpose, it seems that the app using it
should pass in the directory where the native DLLs are located instead
of Managed-OSVR guessing at it.

Sounds reasonable, though perhaps something that the .NET runtime should
be handling for us - I just implemented what I did so that I could click
"Run" in the IDE for Managed-OSVR and have the examples work even when I
was monkeying with the build files. :) (Changed from x86 to Any CPU to
be more truthful and silence warnings, which ended up meaning my system
asked for 64-bit native libraries)

Also, if it uses |SetDllDirectory()|, it should use
|GetDllDirectory()| first to get the old value, then immediately load
the native DLLs with |LoadLibrary("osvrClientKit.dll")|, then use
|SetDllDirectory()| again to restore the old value.

I wasn't sure of the side-effects there or if we had to restore - it
wasn't quite clear to me when I read the docs.

Of course, since we want this to be cross-platform, unless Mono provides
a simulation of GetDllDirectory/SetDllDirectory/LoadLibrary on
non-Windows systems, any additional dependence here is undesirable.

The Unity code, OTOH, is modifying the PATH environment variable and
adding both Plugins and either Plugins/x86 or Plugins/x86_64.

Yes - this code came first and has been in use longer. (It actually now
handles the Assets directory correctly in a built version, which is
another case) At the time it seemed simpler and more in line with how
I'd solved DLL search path issues in the past: batch files or similar
setting a local process PATH.

Since only a single one of these directories is required,
|SetDllDirectory()| sounds like a better solution, but actually my
usual preference is to do neither of these and instead explicitly load
all the needed native DLLs with |LoadLibrary()|, in reverse order of
their dependencies.

Ah, so that is interesting. Is that a common preference in .NET
development or is that just a personal preference? It does sound less
likely to "fail silently", but does introduce a code dependency on
filenames.

So the current DLLs would be loaded in this order:

|msvcr120.dll
msvcp120.dll
osvrUtil.dll
osvrCommon.dll
osvrTransform.dll
osvrClient.dll
osvrClientKit.dll
|

Once the DLLs are loaded, the references to them from managed code
will pick up the preloaded DLLs instead of trying to load them again.

This is interesting - I suspected it, but did not know for sure. My
initial plan for the x64 code was to try loading with SetDllDirectory
and LoadLibrary for a number of possibilities until we succeeded.

Of course this has a couple of disadvantages: it requires this list of
DLLs to be hard coded into the C# code and updated if the list
changes, and it's important to do a |FreeLibrary()| when the app is
done (probably not important in a standalone app but matters in the
Unity Editor so it doesn't keep the DLLs loaded after the game exits).
But it has the advantage of being non-invasive for any other DLL
loading the app may do. (Always tradeoffs!)

I experimented with this approach in Unity and it works in all four
modes: 32-bit editor, 64-bit editor, 32-bit runtime, 64-bit runtime. I
can put together a PR if this approach seems sound.

I think long-term, this or something like it is probably a robust
approach. However, short-term, is there a solution that would enable us
to get a 64-bit-capable Unity package out while the other approaches are
considered? (Or, if this is the quickest path to shipping, then let's go
ahead and get that pull request, since we can always change it later if
needed)

I have had issues with lifetime management in Unity Editor, so I'd
rather not open any unnecessary cans of worms there, but that's just my
personal bad experiences talking.

In any case, much of this complexity comes from having all these
separate DLLs. In an ideal world, there would be a single
|osvrClientKit.dll| native library that statically links all of its
dependencies including the C/C++ runtime DLLs. It makes me a tiny bit
nervous having the runtime DLLs required in any of the Plugins
directories - what if another Unity plugin also dynamically links to
these and has a slightly different version of them (e.g. a patch
release or something)?

I can certainly imagine how we could add an option to make a unified
clientkit dll in the future, though I am a bit worried about
proliferation of library variations, and statically linking to the C
runtime DLL might be less feasible since that seems to require all
dependencies and consumers to do the same. Ideally we'd not need the
runtime dlls in there, but I think that's actually the recommended way
(that or providing the redist installer) - it's what CMake defaults to
in InstallRequiredSystemLibraries. In any case, they're not needed if
you have the VC2013 runtime installed on your system.

The nice thing about a statically linked osvrClientKit.dll would be
that it would work automatically with Unity with no PATH or
|SetDllDirectory()| or explicit loading of a list of DLLs, and for
other CLR apps it would work with at most an explicit |LoadLibrary()|.

I started to look at OSVR-Core a bit to see what would be required to
build osvrClientKit.dll with everything statically linked, but as you
know it's a bit of work getting all the other dependencies installed
and set up. I did get as far as getting CMake to generate all the
project files for OSVR-Core, but figured I'd get your thoughts before
doing anything more along those lines. Thanks!

I think it's worth considering for the medium to long term: can
certainly file an issue on OSVR-Core as a request, would be some
build-system work.

For the short term, getting something shipped that works on Unity 64,
though it might not be as portable or theoretically ideal as desired, is
probably good enough and would give breathing room to consider a
longer-term solution.

Thanks for your work on this!

[rpavlik]

@geary Any progress you can push, even if just a stop-gap solution?

[geary]

Hi guys, I'm really sorry for the radio silence! I got pulled onto an urgent project and meant to let you know I'd be delayed on things. I will have some time today and this weekend, so I will look at the code I was working on and see what is suitable to push.

[rpavlik]

I'm now working on this.

[JeroMiya]

Do we need LoadLibrary calls for transient dependencies, or is that handled for us when osvrClientKit.dll is loaded? Is that what OSVR/OSVR-Core#83 is addressing?

If it helps to make progress on this, we can just require Managed-OSVR users pick their target arch and copy the appropriate files into an arch-specific output directory (maybe driven by build configuration?), like unity does. Most client-side .Net development is moving towards AOT with CoreCLR (.Net Native and LLILC) and Unity's IL2CPP, and these all pre-compile the .Net code to a specific target arch anyway. So, I think figuring out a way to dynamically load the right native DLLs at runtime isn't a big priority. A simple two-output solution for x86/x64 support is reasonable at this point.

For MonoGame I need to create an app launcher anyway (to select the display). It's pretty simple to just make the launcher app either 32-bit or AnyCPU, but have it launch the 64-bit or 32-bit version of the app and call it good.

[rpavlik]

Basically, yeah, that's what that other issue works around in a different way: because the code is modular and more than one library, even if osvrClientKit.dll can be found, that doesn't mean that the dependencies of osvrClientKit can be - even if they're in the same directory. Ah, the joy of Windows dynamic library search paths.

Funny (or perhaps intentional) you should comment on this now - I literally just got done doing some work on this issue recently. I got the CI building Managed-OSVR, and also got a job somewhat crudely grabbing binaries from that project and using them in OSVR-Unity instead of building the old managed-osvr code in this repo. With the changes in OSVR/Managed-OSVR#17 (code review appreciated) I have Unity 4.6.1 32-bit editor, 5.0 64-bit editor, and both versions both 32- and 64-bit builds working properly - was just going to clean up this repo a bit in a branch and pull request it, then teach the CI someplace public to stick binaries and effectively "flip the switch" to ship 64-bit support for unity Monday (or maybe sooner).

[JeroMiya]

Ah, I see. I'm surprised this doesn't work without explicit library loading. From what I've read that should be handled by the native windows loader (and the mono equivalent on other platforms):

http://stackoverflow.com/questions/16018794/when-loading-a-c-dll-in-c-does-the-clr-load-the-dependencies-of-the-c-file

Out of curiosity, what is it about the native libraries that makes it difficult for the windows loader to auto-load them when osvrClientKit.dll is loaded?

Here are some notes from a quick sight review:

• Since you are moving the static constructor into a static PreloadNativeLibraries method that is explicitly called, it needs to be thread safe.
• (minor) CheckDir method name could be more descriptive given that it is also responsible for loading the libraries it checks for.
• (minor) produceLibPath should be named ProduceLibPath even though it is a private method.

[rpavlik]

Well, it just has to do with the default search path for DLLs in windows- by default, it includes the .exe directory, possibly a windows directory or two (don't remember since I never put things there), and the contents of the PATH environment variable. I think what makes it difficult is that there is a transitive dependency on additional DLLs - osvrClientKit.dll isn't monolithic - so while Unity may have tweaked the p/invoke code to add their own platform-specific paths (as I am presuming they did based on the behavior I've observed - they can find p/invoked dlls in Plugins/x86, etc), that doesn't help Windows find the dependencies of the library Unity is helping to find - Windows doesn't search the directory it finds the .dll in for dependencies (unless that's also the directory with the exe). And, whatever directory it finds a dll with the right name in first, it uses those DLLs even if they're the wrong platform (it will just blow up then with an "image format" exception)

• Good point on thread safety, though a: I'd expect that to only be called once by the consumer of the library, b: if we can safely put it back in the static constructor I'm fine with it as long as "leaking" those library handles is no big deal, and c: what about it isn't thread safe? It's fine if more than one call to LoadLibrary with the same name takes place, I think - the worst that will happen is an extra reference on that library handle we're already leaking. I mean, I guess we could wrap the whole thing in a mutex, but I'm not sure that there is a need.
• Yeah, doesn't really just check the directory any more, does it...
• Good catch, thanks - these are C++-accustomed fingers, and I don't pascal-case my C++.

[JeroMiya]

Perhaps it's an over-abundance of caution, but the two thread safety issues I see are:

1. (less likely) The static method being called from two threads simultaneously. This isn't very likely given the convention we've set up, and the consequences may not be that bad - an extra library handle perhaps.
2. (not sure how likely) Threads that depend on the static method being called may run from a different thread. This might be more likely if the framework they are using takes over the application bootstrapping process, etc...

I like the static method approach you've gone with, because it makes it optional. If the developer wants, they can just have a completely separate output directory for 32-bit and 64-bit exe+native dlls as suggested here (#19 (comment)), in which case they don't even need to call the method.

I reviewed your updates and everything seems good. I think it would be OK to go ahead with this PR and investigate the need for thread-safety/idempotency of the static method as a separate lower-priority PR.

[rpavlik]

Great, thanks for your review.