-
Notifications
You must be signed in to change notification settings - Fork 38
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
64-bit Unity support #19
Comments
Is anyone actively working on this? If not, I'm on it since I need it for my project. |
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! |
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. |
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 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? |
Well, I just got a little carried away when I split that repo out. 😀 I had all the references noted to handle the Thanks for your help! Note that I think that now, we can probably put the assembly just right in the |
Yup, that's exactly what I did: the managed DLLs are in Plugins and native DLLs in Plugins/x86 and Plugins/x86_64. |
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 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,
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 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 The nice thing about a statically linked osvrClientKit.dll would be that it would work automatically with Unity with no PATH or 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! |
On 4/6/2015 2:49 PM, Michael Geary wrote:
Of course, since we want this to be cross-platform, unless Mono provides
I have had issues with lifetime management in Unity Editor, so I'd
For the short term, getting something shipped that works on Unity 64, Thanks for your work on this! |
@geary Any progress you can push, even if just a stop-gap solution? |
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. |
I'm now working on this. |
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. |
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). |
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): 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:
|
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
|
Perhaps it's an over-abundance of caution, but the two thread safety issues I see are:
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. |
Great, thanks for your review. |
This bug is the catch-all for any work required for 64-bit Unity builds that isn't a separate bug.
Includes:
The text was updated successfully, but these errors were encountered: