Skip to content
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

Adds support for MIDI CC events inside LMMS #5581

Merged
merged 51 commits into from
Dec 2, 2020

Conversation

IanCaio
Copy link
Contributor

@IanCaio IanCaio commented Jul 14, 2020

This PR implements a feature that I personally consider very useful for LMMS and hope others share the thought!

MidiCC

Feature Description

A MIDI CC rack is added to LMMS where there are Knobs for every single CC controller (0-127) linked to each Instrument Track. When the rack is enabled (LED button) any change to the values of those Knobs will trigger MIDI CC events to that particular track with the current value of the controller, very similar to what happens when we connect a hardware MIDI keyboard with controllers to the track.

Various plugins, like Carla-patchbay (and hence Lv2 plugins as Surge, TAL-Filter, TAL-Reverb, etc) and ZynAddSubFX accept mapping parameters to CC controllers. Since those Knobs can be automated, this feature basically makes automating any mapped parameters possible! This opens the doors to lots of fancy automation possibilities that before were either not possible, required live recording or required fancy hacking to do.

The way it works now, those events are only generated when a Knob's value is changed. If the MIDI CC rack is disabled on a track, the value of the Knob is changed and it's reenabled though, it won't generate a event until the value is once again changed. That behavior resembles MIDI keyboards as well: if you turn on a keyboard with a slider on value 50 for example, it will only trigger a MIDI CC event when you change the value, so I believe it should be kept that way.

Technical Stuff:

For implementing that feature, a new class called MidiCCRackView was created. It basically creates the whole GUI for the MIDI CC rack (ComboBox for tracks and Knobs for controllers). Lots of connections are made to keep the ComboBox up to date with the current Instrument Tracks available and there's also a TrackContainer::TrackList member on the class holding pointers to the Instrument Tracks objects themselves.

The InstrumentTrack class now also have one BoolModel called m_midiCCEnable (which tells if the CC controllers are enabled/disabled) and also a FloatModel for each CC controller. The dataChanged() event of each CC controller FloatModel is connected to a SLOT method called processCCEvent(int controller), which will check if m_midiCCEnable is true, and if it is it will process the MIDI CC events to the output channel of the track.

Every time a track is selected on the MIDI CC rack ComboBox, the LED button and Knobs are pointed to the models of that particular track. The proper code for adding the rack on the MainWindow and GuiApplication classes was also added.

IanCaio added 14 commits July 10, 2020 18:58
This branch will be used to implement a new feature to LMMS, where MIDI CC messages support will be added through the use of a rack holding knobs representing each MIDI controller. The changes to values of those knobs will trigger MIDI CC messages to the corresponding track. There will be the possibility of automating the values.

Summary of changes:
	- Creates a new icon to be used on the MIDI CC rack window.
	- Adds the MIDI CC rack window as a new class.
	- Very basic window toggling functionality.
	- Updates CMakeList to compile this new .cpp file.
Keeps working on the GUI from the MIDI CC Rack. Now we have a track tool bar where we have a label and a combobox where the Instrument tracks will be listed. Below it we have a GroupBox where later the MIDI CC knobs will be added.
This commit basically finishes the basic GUI of the MIDI CC Rack, still not functional. It adds a scrollable area to hold the knobs and a knob for each possible CC controller.
-Adds class members to hold the CC knobs and the track combobox.
-Stores the number of controllers on a constant.
-Changes vertical size of MIDI CC Rack window.
The MIDI CC Rack ComboBox containing the available instrument tracks now updates itself to be in sync with the tracks on the Song Editor and BB Editor.

Summary of changes:
	- Creates a ComboBoxModel to hold the tracks names for the ComboBox.
	- Adds two new signals to the TrackContainer class: trackRemoved and trackRenamed. The trackRenamed signal is connected to the nameChanged signal of each track added to the TrackContainer.
	- Connects the trackAdded, trackRemoved and trackRenamed signals from both the song editor and BB editor to a slot method that will update the ComboBoxModel with the appropriate track names.
	- Added the updateTracksComboBox method that will clear the model and fill it again with the updated track names from both the song editor and BB editor.

To do:
	- Make sure the ComboBox reorder the tracks if we drag the TrackContainerView widget on the song editor or BB editor (in other words, reorder the combobox list if we reorder the tracks).
Now when tracks are moved inside the song editor and BB editor, they get reordered at the ComboBox as well.

Summary of changes:
	- Added a movedTrackView signal to TrackContainerView class, which will be emitted when a track is moved.
	- Connects this signal inherited from the song editor and BB editor, so it triggers the tracks ComboBox update.
	- Moves the construction of the MidiCCRack to the end, since we need the BB Editor to be created before the MIDI CC Rack to be able to connect this signal.
Before merging also resolves a merge conflict on include/MainWindow.h, where the toggleFullscreen and toggleMidiCCRack were being declared on the same line.
Starts the implementation of the models that will hold the CC controller values for each Instrument Track. Selecting a different track now will show the values of the models for that particular track on the knobs.

Observation:
	Read the "TODO" comment. There's line that keeps a SEGFAULT from happening, but the root cause has to be understood (maybe it's not a bug but still worth investigating the reasons behind it).
This commit implements the MIDI CC event handling feature itself. Changing the values on the CC controller knobs inside the MIDI CC Rack now results in MIDI CC event messages being triggered on the selected track. Those can also be automated so those messages are generated during the song play. Some small fixes were also made.

Summary of changes:
	- Changes the number of CC controllers to 128 and fix the numbering so they are counted from 0-127.
	- Adds a name to the FloatModels of the CC controllers so they are more easily identified in the automation tracks.
	- Adds a SLOT method to the InstrumentTrack class that will receive a controller number as an argument and process a MIDI CC Event with the value of this controller to the output channel of the track.
	- Connects the dataChanged signal of every controller FloatModel so they trigger this processing method everytime the value is changed. Maps all the knobs models to the appropriate method call using a QSignalMapper.
Now the GroupBox LED button will enable or disable the MIDI CC events triggering for the selected InstrumentTrack. The LED button is connected to the selected InstrumentTrack's BoolModel. If it's disabled, the callback function that processes the MIDI CC messages will be called but will return before processing the MIDI CC event. The connection isn't destroyed in that case (probably the overhead of having to recreate the connection every time we enable the MIDI CC for the track doesn't compensate the unnecessary function call). When the LED button is enabled, the MIDI CC event is processed normally.

One behavior that needs to be made clear is the following:

If a value is selected while MIDI CC is enabled (i.e.: 120), then it's disabled, and another value is selected while it's disabled (i.e.: 40), when the MIDI CC is enabled again an event WON'T be triggered until a change in the value is made AFTER it was enabled again. That sounds reasonable and what would be expected from a hardware MIDI controller (when a controller is turned on with a knob on a value, it will only trigger an event for that knob when it's moved to a different value). So I believe it should be kept that way.
There was a recent change on master that assigned different shortcuts to the editors. Instead of using F1-F12 those now use Ctrl + 1-7. I assigned CTRL + 8 to the MIDI CC rack for consistency.

Also, some titles were corrected in terms of letter case ("midi" -> "MIDI" and "cc" -> "CC").
@LmmsBot
Copy link

LmmsBot commented Jul 14, 2020

🤖 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

🤖
{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://10983-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.80%2Bga83df37-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10983?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://10982-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.80%2Bga83df37b9-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10982?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://10984-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.80%2Bga83df37b9-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10984?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/kio5gcaspewh2b4j/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36576805"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/thaebmmk6kk74n21/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36576805"}]}, "commit_sha": "291ebc431e83be504fc7bd7e99f5e662414a874b"}

IanCaio added 2 commits July 14, 2020 10:46
A Segfault error was being caused when clicking on the GroupBox label, because instead of setting the model in the GroupBox widget I was setting the model on the LED button widget. The appropriate changes were made to fix the issue.

Also, the MIDI CC rack window is now hidden at start up by default.
With this commit, the models related to MIDI CC are saved in the Instrument Track DOM structure. The models are m_midiCCEnable and m_midiCCModel (for each controller). m_midiCCEnable is saved as an attribute on <instrumenttrack> called "enablecc" and m_midiCCModel is saved in a child element called <midicontrollers>.

This allows the user to save projects with CC automation tracks.

TODO:
	- Check if changes on the DataFile class are necessary to make older projects compatible.
IanCaio added 2 commits July 15, 2020 00:10
When connecting the MIDI CC models to the InstrumentTrack::processCCEvent method I was using QSignalMapper to call the method with the appropriate parameter. Now lambda functions are used instead, which saves lines and makes it more readable.
I had changed the MIDI CC rack shortcut from F12 to Ctrl+8 on an earlier commit, but forgot to update the ToolTip.
After replacing QSignalMapper for lambda functions on a previous commit I forgot to remove the <QSignalMapper> header.
@ryuukumar
Copy link
Member

Hey, I think you've forgotten to add a MIDI CC entry in the view menu. That can be found in MainWindow.cpp I believe, and it's probably some two lines of code.

@IanCaio
Copy link
Contributor Author

IanCaio commented Jul 17, 2020

Hey, I think you've forgotten to add a MIDI CC entry in the view menu. That can be found in MainWindow.cpp I believe, and it's probably some two lines of code.

Thanks, well spotted! I barely use the View menu so I didn't even realize I forgot it. I'll add it and push the changes when I get home.

I forgot to add an action in the View Menu for accessing the MIDI CC Rack. This commit adds it.
@IanCaio
Copy link
Contributor Author

IanCaio commented Jul 18, 2020

Took me a while, but I just added the item in the View Menu for the MIDI CC Rack!

@ryuukumar
Copy link
Member

Amazing!

include/InstrumentTrack.h Outdated Show resolved Hide resolved
include/MidiCCRackView.h Outdated Show resolved Hide resolved
src/gui/widgets/MidiCCRackView.cpp Outdated Show resolved Hide resolved
@PhysSong
Copy link
Member

The icons use the letters "CC". Is this universal, or is someone who doesn't speak English, and maybe doesn't even use the Latin alphabet, going to be confused as to what this is?

I think this term is common for who doesn't speak English.

I recall some discussion about this before, but is processInEvent, rather than processOutEvent, really the right place to send the CC events? Both send them all to the instrument, but processOutEvent means it's possible to control external devices with them too. The only behaviours exclusive to processInEvent are sustain using CC64, and silencing all notes using CC120/CC123-127.

It's a very tricky question. Maybe we call processInEvent() first and then call m_midiPort.processOutEvent() explicitly?

	Moves source file to a more appropriate location.
	Removes the label with the track name, moving the name of the
track owning the MIDI CC rack to the window title.
	Makes it so we have 4 MIDI CC knobs per row, since the total
number is not a multiple of 3 but a multiple of 4 (improves alignment).
	Adds translation calls on the places that were lacking them and
removes some unnecessary QString conversions.
	Changes the MIDI CC models so they are private members of the
InstrumentTrack class (makes MidiCCRackView a friend class so it can
access them without the need of using setters/getters).
	Changes the MIDI CC model pointers to be std::unique_ptr, so we
can move away from the responsability of freeing their allocations on
the destructor.
	I wasn't properly destroying each MidiCCRackView when a track
was removed, causing them to build up. This commit changes the
m_midiCCRackView variable on the InstrumentTrackView class to be a
std::unique_ptr (destroyed when out of scope) and added the appropriate
code on the MidiCCRackView class destructor to remove the parent widget.
	destroyRack() method was removed (it wasn't doing its task
appropriately before and is now unnecessary after the destructor was
fixed) and also unsetModels() which wasn't necessary anymore.
	This commit makes it so the MidiCCRackView isn't created on the
InstrumentTrackView constructor, but instead is only created on the
first time it's toggled.
@IanCaio
Copy link
Contributor Author

IanCaio commented Nov 29, 2020

@DomClark

I finished making the requested changes. I did everything on a separate commit to make it easier to review the changes, would appreciate if you to double checked them, specially the last couple ones: I come from a C background so I haven't used smart pointers much. I was also a little bit in doubt whether the lazy creation of the MidiCCRackView should be done on the InstrumentTrackView toggle method or inside the MidiCCRackView class, but it didn't make the method much verbose so I think it's alright to keep it there.

As for the unsetModels code, I removed the method from MidiCCRackView since it's not needed there anymore, however I kept the unsetModel method on the ModelView class if everyone is alright with that. I elaborate a little on why in this comment (#5581 (comment)). We have a undefined/unclear behavior on our ModelView, where sometimes it's acceptable to pass a nullptr model and sometimes it isn't. This could lead to hard to track bugs (like one I had in the process of working on this PR), so having a unsetModel method gives the developer a more clear way of doing that when necessary. I'd also change the only occurrence where nullptr is passed to a model view to use this method instead, but that is a small change that can be done on a separate PR to avoid going even more out of scope here.

@MaciejMalczyk
Copy link
Contributor

The icons use the letters "CC". Is this universal, or is someone who doesn't speak English, and maybe doesn't even use the Latin alphabet, going to be confused as to what this is?

What do you think about icon simmilar to DIN connector? They are widely used in MIDI hardware setups.
image

@zonkmachine
Copy link
Member

What do you think about icon simmilar to DIN connector?

Great! There are din style icons around after the work with the instrument tabs: #3569 (comment)

@IanCaio
Copy link
Contributor Author

IanCaio commented Nov 29, 2020

Forgot to mention, regarding the processInEvent vs processOutEvent, it was indeed discussed before and it was hard to get to a definite decision, each having a different pro/con. I went for the processInEvent (after initially using processOutEvent) because it is the closest we would be to an actual hardware controller behavior, with LMMS handling some special CC as if they had originated from one. The drawback as you mentioned is that we can't forward those. We could try what @PhysSong suggested and manually forward CC events if they aren't marked for being ignored on export (which means they originated from the CC rack), adding this line on the end of case MidiControlChange of processInEvent:

if (!event.ignoreOnExport())
{ // Ignore space indenting
   m_midiPort.processOutEvent(event, time);
}

@wujekbrezniew
I like the idea, think it would look nice! I tried to get the icons from the PR linked by @zonkmachine , but they looked a bit odd in the small resolution we have for icons (20x20), which I think was why Umcaruje moved away from them throughout the PR. I'm not sure if the CC icon would confuse people that speak languages without latin characters (PhysSong being Korean would probably have better insight on this), but if someone manages to make an icon that suits this I can change it.

@MaciejMalczyk
Copy link
Contributor

@IanCaio I created two icons in res 256x256. They should match LMMS style:
DIN.zip

Copy link
Member

@DomClark DomClark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made a few suggestions that I missed the first time.

  • Include ordering should be updated to follow the coding conventions, and I think some includes are no longer used and could be removed.
  • How about adding the CC rack icon to the menu item used to open it?
  • What's the purpose of disabling the CC knobs? It's not like disabling an effect, where it no longer changes the sound - it just freezes them at their current positions instead.

As for the unsetModels code, I removed the method from MidiCCRackView since it's not needed there anymore, however I kept the unsetModel method on the ModelView class if everyone is alright with that.

That's fine - I still think we should disallow having a view without a model, but that's out of scope here, so if this change improves the existing code, I think it's good to have it for now.

Forgot to mention, regarding the processInEvent vs processOutEvent, it was indeed discussed before and it was hard to get to a definite decision, each having a different pro/con. I went for the processInEvent (after initially using processOutEvent) because it is the closest we would be to an actual hardware controller behavior, with LMMS handling some special CC as if they had originated from one. The drawback as you mentioned is that we can't forward those.

Yeah - CCs from actual hardware controllers don't get forwarded to external devices either, so there's room for more general improvement here. This is out of scope I feel, so again what you've got here is fine for now.

include/InstrumentTrack.h Outdated Show resolved Hide resolved
include/MidiCCRackView.h Outdated Show resolved Hide resolved
include/MidiCCRackView.h Show resolved Hide resolved
src/tracks/InstrumentTrack.cpp Outdated Show resolved Hide resolved
	Changes the access specifiers of methods on the MidiCCRackView
(renameWindow from public slot to private slot) and InstrumentTrack
(processCCEvent from public slot to private method) and fixes the
MidiCCRackView destructor keyword being used (from virtual to override).
	Changes the icon used on the MIDI CC rack window to the one
created by @wujekbrezniew.
	Removes headers that weren't used anymore and organizes the
remaining ones to conform to the code style.
@IanCaio
Copy link
Contributor Author

IanCaio commented Nov 30, 2020

Made a few suggestions that I missed the first time.

* Include ordering should be updated to follow the [coding conventions](https://github.com/LMMS/lmms/wiki/Coding-Conventions#include-order), and I think some includes are no longer used and could be removed.

Reorganized them according to the coding conventions and removed ones that weren't being used anymore (leftovers from changing the code quite a bit from the original concept).

* How about adding the CC rack icon to the menu item used to open it?

Done! Using the icon provided by @wujekbrezniew on both the window and menu action.

* What's the purpose of disabling the CC knobs? It's not like disabling an effect, where it no longer changes the sound - it just freezes them at their current positions instead.

Initially it was thought as a convenience for the user to be able to disable the triggering of CC messages (any change in values trigger them, so if there are automations connected to the CC knob you can just disable the MIDI CC rack if you wish to play the track without the CC automations for example). But later it was also useful when loading the instrument settings, to avoid a SegFault being caused by MIDI CC events being triggered while the instrument was still loading (6abd130).

	Instead of using the std::unique_ptr constructor with the new
allocator, we are now using C++14 std::make_unique when creating the
unique pointers to the MIDI CC models.
@IanCaio IanCaio merged commit 3c36365 into LMMS:master Dec 2, 2020
@zonkmachine
Copy link
Member

zonkmachine commented Dec 3, 2020

@IanCaio This somehow breaks the track preview button (m_activityIndicator).

@IanCaio
Copy link
Contributor Author

IanCaio commented Dec 4, 2020

Just quoting @DomClark regarding the m_activityIndicator issue on Discord:

MidiEvent( MidiNoteOn, 0, DefaultKey, MidiDefaultVelocity ) in InstrumentTrackView::activityIndicatorPressed is now calling the wrong constructor of MidiEvent.
It used to call this one:

MidiEvent(MidiEventTypes type = MidiActiveSensing,
            int8_t channel = 0,
            int16_t param1 = 0,
            int16_t param2 = 0,
            const void* sourcePort = nullptr,
            bool ignoreOnExport = true)

but now it calls this one:

MidiEvent(MidiEventTypes type, const char* sysExData, int dataLen, bool ignoreOnExport = true)

We are still discussing the best way to fix this problem. It wasn't a direct result of this PR, but an issue that got uncovered by the fact that now that the other constructor has 4 arguments the ADL results in the wrong constructor being picked.

IanCaio added a commit to IanCaio/lmms that referenced this pull request Dec 5, 2020
	This attempts to fix a bug uncovered by LMMS#5581, where the wrong
MidiEvent constructor is selected due to ambiguity in the arguments that
are used in the call.
	Part of the solution is to use a enum class on the last
parameter instead of a boolean to avoid implicit conversions and a more
strict method selection through the ADL. One of the parameters type on a
constructor was also changed from int to std::size_t which more
accurately represents it.
	"ignoreOnExport" was replaced with "source", which for now can
be Source::Internal or Source::External, and its value is still used on
the processInEvent to define whether a MidiEvent should be ignored
during the song export.
IanCaio added a commit to IanCaio/lmms that referenced this pull request Mar 28, 2021
devnexen pushed a commit to devnexen/lmms that referenced this pull request Apr 10, 2021
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants