-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
Support for Unreal 5 #9
Conversation
RuntimeDependencies.Add("$(TargetOutputDir)/BoardController.dll", Path.Combine(ModuleDirectory, "Compiled", "x64_dynamic", "lib", "BoardController.dll")); | ||
RuntimeDependencies.Add("$(TargetOutputDir)/DataHandler.dll", Path.Combine(ModuleDirectory, "Compiled", "x64_dynamic", "lib", "DataHandler.dll")); | ||
RuntimeDependencies.Add("$(TargetOutputDir)/GanglionLib.dll", Path.Combine(ModuleDirectory, "Compiled", "x64_dynamic", "lib", "GanglionLib.dll")); | ||
RuntimeDependencies.Add("$(TargetOutputDir)/gforce64.dll", Path.Combine(ModuleDirectory, "Compiled", "x64_dynamic", "lib", "gforce64.dll")); | ||
RuntimeDependencies.Add("$(TargetOutputDir)/gForceSDKWrapper.dll", Path.Combine(ModuleDirectory, "Compiled", "x64_dynamic", "lib", "gForceSDKWrapper.dll")); | ||
//RuntimeDependencies.Add("$(TargetOutputDir)/gforce64.dll", Path.Combine(ModuleDirectory, "Compiled", "x64_dynamic", "lib", "gforce64.dll")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those dlls seem to be missing (they are built by the python script but not copied over).
Are they still relevant should I look into copying them over or should I remove them from here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are not missed, they are copied if you run python script with --oymotion
option and it seems like I forgot to add this option to readme.
I also considered an option to take dlls from this job which stored all possible libs as github artifacts https://github.com/brainflow-dev/brainflow/actions/workflows/deploy_cpp_libs.yml but ended up using submodules because it was easier for me but maybe not for users
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw in fact there are some missing libraries inside this unreal engine build script, for example simpleble, muse_bglib, etc. Need to add them to support all possible devices. You can set all options in build.py to true to see the full list of libs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also maybe it makes sense to write a method which will go through files in lib folder after compilation and add all of them ro runtimedependencies. It will look better than hardcoding names, especially including the fact that there are different build options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also considered an option to take dlls from this job which stored all possible libs as github artifacts https://github.com/brainflow-dev/brainflow/actions/workflows/deploy_cpp_libs.yml but ended up using submodules because it was easier for me but maybe not for users
My suggestion, especially if you want to publish this to the maketplace sometime soon, is to push the precompiled binaries into this repo. I know it's not ideal to have binary files in git, but it really makes the publishing process to marketplace easier + you are sure if someone clones they are using a compatible version (UE SDK - C++ SDK). The job you have there seems perfect we just need to make sure it supports all the platforms you care about. I also wrote something similar here: https://github.com/outoftheboxplugins/configcat-unreal/actions/runs/5833331101
It's still very important to give the users some instructions about compiling locally because they might need to change the C++ SDK.
also maybe it makes sense to write a method which will go through files in lib folder after compilation and add all of them ro runtimedependencies. It will look better than hardcoding names, especially including the fact that there are different build options
Yes, it makes total sense. I actually did something similar a few weeks ago for this plugin: https://github.com/outoftheboxplugins/configcat-unreal/blob/main/Source/ThirdParty/ConfigCatCppSDK/ConfigCatCppSdk.Build.cs
I wanted to keep the changes of the PR to a minimum, but I am happy to implement it here for this PR or next ones.
btw in fact there are some missing libraries inside this unreal engine build script, for example simpleble, muse_bglib, etc. Need to add them to support all possible devices. You can set all options in build.py to true to see the full list of libs
I think if we deliver pre-built binaries we can just include all in the repo and we can adjust the Build.cs
accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can setup jobs to compile everything and push to marketplace wo pushing binaries to git, but can add binaries to the repo also if needed. The main requirement - it should be simple to update and dont take too much time to maintain, so probably smth like cron job with personal access token will be ok.
Lets fix this ugly hardcode for dyn libs and iterate over files in libs dir as a part of this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I've adjust the DLLs and the libs discovery.
Since this is a bigger change, I would love to know if you can suggest some tests to confirm everything is working well. (I've tried building some bits from https://brainflow.readthedocs.io/en/stable/Examples.html#id2). Or if you know someone who's using this with Unreal, I would be happy to get in touch with them and confirm it works for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test is ok you can use it with synthetic board and see if you can read some data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you manage to get some data using this sample?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Thanks again for all the help both here and on slack. I was able to get the sample data both for at editortime (Play in Editor) and runtime (packaged game).
I've modified the file as discussed, the brainflow.lib is statically linked in the .exe and the 3 DLL files (board controller, ml module, data handler) are dynamically linked. All other DLL are copied over to the final build so they can loaded if needed.
PR with the C++ SDK fix: brainflow-dev/brainflow#672 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what was the exact issue in unreal engine 5? looks like just minor changes in paths and removed win32, but not sure how important they are
I also was expecting bigger problems, but they turned out to be quiet minor. I think the deprecation of |
PublicDelayLoadDLLs.Add("libBoardController.dylib"); | ||
PublicDelayLoadDLLs.Add("libDataHandler.dylib"); | ||
PublicDelayLoadDLLs.Add("libMLModule.dylib"); | ||
PublicDelayLoadDLLs.Add(Path.Combine(PrecompiledFolder, "libBoardController.dylib")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it be the same for linux?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually don't know without testing.
On Windows, I found the PublicDelayLoadDLLs
need to be filename only and manually loaded in the module file to work, but they won't work with full path.
On Mac, I found the PublicDelayLoadDLLs
will automatically get loaded properly if the full path is provided which I found easier and nice.
On Linux, I got no clue what will work, but I hope the full path will be working there too. I will make the adjustments
Here is a draft for the Unreal Engine upgrade.
So far I've tested with Unreal Engine 5.2 Windows & MacOs. Since 5.3 got out this week, I will run the plugin on that version too as well as a Linux machine.
Based on my tests it seems there might be a few changes required in the C++ SDK as well. I will create a PR for that and link it in.
Fixes #8