-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Enable Lv2 Atom ports #5691
Enable Lv2 Atom ports #5691
Conversation
1d736f9
to
c5a4397
Compare
🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩
Linux
Windows
macOS🤖{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://9685-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-735%2Bg5e873da-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/9685?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://9687-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-735%2Bg5e873da14-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/9687?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://9688-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-735%2Bg5e873da14-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/9688?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/rfowgksa52r9n2ch/artifacts/build/lmms-1.2.2-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/35839173"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/5nin0we8f7608fi3/artifacts/build/lmms-1.2.2-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/35839173"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://9689-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-735%2Bg5e873da14-mac10.13.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/9689?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "bec5e25132bf9d83156257ebca12751671c5e51e"} |
c5a4397
to
64cb96c
Compare
CI finally passed 😃 Ready for review. |
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.
Generally looks good to me.
07857ad
to
7e059b3
Compare
All comments re-worked. Let's see what CI says... |
CI passed 🎉 @DomClark can you please check my rework? |
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.
Although m_midiIn
is set to the required MIDI in port if one is present, this doesn't seem to be taken into account when actually sending events to the plugin - they all go to the first visited MIDI in port instead.
|
b1c73bc
to
3a79181
Compare
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.
Everything looks correct now, as far as I can tell. I've suggested a couple of small improvements.
3a79181
to
95bf46c
Compare
95bf46c
to
3227be9
Compare
All Atom ports are now being created and connected. Currently only MIDI input ports are supplied with data. Major contribution to LMMS#562 ("lv2 support").
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.
Not a code review (not very familiar with the Lv2 core right now), but a tester review.
Everything seems to be working fine. I tried testing other plugins besides Surge (drumkv1
, ZynAddSubFX
, samplv1
, Black Pearl Drumkit
and Red Zeppelin Drumkit
) but those couldn't be loaded due to missing features. If requested I could test with another plugin known to not require those.
Surge has worked perfectly though, the Instrument Widget's, Piano Roll's and QWERTY MIDI keyboards played the notes smoothly. Tried adding lots of notes simultaneously (above 20 I guess) and it worked fine. Also tested using a hardware MIDI keyboard and it behaved properly.
No misbehavior or instabilities noticed. Very excited to see how Lv2 is growing!
3227be9
to
bec5e25
Compare
This commit enables atom ports and feeds them with MIDI events, if the atom ports accepts them. This enables Lv2 instruments with piano roll, e.g. the MDA Piano.
Reviewer hints:
Please reviewTester hints:
Wait with final testing until review is done