Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Support for Unreal 5 #9
Changes from 6 commits
b6d6f27
b9a42ed
e0b84cf
a90e787
07f12e3
c852eea
a78d5a2
04f7020
338e7ab
b931801
6c79ff8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
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.
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.