Play automation pattern when midi controller connected

[serdnab]

fix for #4554

When a button is connected to a midi controller it now plays the automation patterns.

I added a flag that when it is playing an automation pattern it does not look for controllerValue() but m_value.

Also I added:

• isControllerMidi() to ControllerConnection.h that is set when the controller is midi

• this code so that when an automatable model exits automation the midi controller takes back control

• a signal connection so that when the song stops the midi controller takes back control

[LmmsBot]

🤖 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

• Linux (AppImage): lmms-1.3.0-alpha.1.68%2Bgbd850b8-linux-x86_64.AppImage (build link)

Windows

• Windows 32-bit: lmms-1.3.0-alpha.1.68%2Bgbd850b8d3-mingw-win32.exe (build link)
• Windows 64-bit: lmms-1.3.0-alpha.1.68%2Bgbd850b8d3-mingw-win64.exe (build link)
• Windows 32-bit: lmms-1.3.0-alpha-msvc2017-win32.exe (build link)
• Windows 64-bit: lmms-1.3.0-alpha-msvc2017-win64.exe (build link)

🤖

{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://12187-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.68%2Bgbd850b8-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12187?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://12188-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.68%2Bgbd850b8d3-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12188?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://12189-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.68%2Bgbd850b8d3-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12189?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/amntmm6u8b73mcqm/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/37363583"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/w3orkxotxbvr43c3/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/37363583"}]}, "commit_sha": "406575f780048978350b33febb3a57edd499e600"}

[qnebra]

It just works 👍 Love it :D

[ryuukumar]

Tested by @qnebra and style looks okay to me. Sorry for the massive delay.

[IanCaio]

Sorry about the late review!

The PR improves the behavior to be closer to what an user would expect, thanks a lot for putting some effort on it! From a first review I didn't notice any bugs on the code, but there are a few points that concern me on its current state:

1. I think this PR adds too much complexity for the task and the lack of comments made it harder to follow the logic which is a bit spread.

2. In line 397 from src/core/AutomatableModel.cpp (AutomatableModel::setAutomatedValue) you add the requirement that the linked model isn't connected to a controller before notifying it. What was the reason behind that? I ask because it doesn't seem to be the behavior before and I didn't figure out any reasons to have that condition.

if (!((*it)->controllerConnection()) && (*it)->m_setValueDepth < 1 &&
	(*it)->fittedValue(m_value) != (*it)->m_value)

3. The use of the signal emitted by Song::stopped() to make the values used be the ones from the MIDI controller and the setAutomatedValue being the one bringing the control back to the Automation ended up spreading the code related to this feature in more methods than I believe is necessary, or even in methods that I think shouldn't be related to this (i.e.: sounds out of scope for setAutomatedValue to define whether controllers should rule over Automations).

4. From what I gather from the logic there's no way to record Automation Patterns using a MIDI controller, because when the song starts playing setAutomatedValue will disable the controller values. This is likely not wanted since using MIDI controllers to record an automation is very useful on the production standpoint. The way the logic is laid out makes accounting for this third situation a bit more complicated (as in it would spread the code more).

What I think could be done instead (there's a small very raw sketch with an example later):

• Have a method on the AutomatableModel called useControllerValues that returns a boolean telling whether the controller values should have precedence in that moment (similar to m_controllerValue, but the name is more descriptive and the logic for checking it would be moved inside that method instead of being set by signals and setAutomatedValue).
• Have a boolean member variable called m_isRecording inside AutomatableModel. That variable should be true if any Automation Pattern that is connected to that model is recording. In the sketch linked below, there's a isRecording() method instead that loops through all Automation Patterns and returns true if any is recording, but in terms of performance it's better to have the Automation Patterns emit a signal when they start/stop recording and having that signal update the m_isRecording variable on the connected models. That code to connect and disconnect the signal should be added on AutomationPattern::addObject and AutomationPatternView::disconnectObject (even better if you extract the model logic to AutomationPattern::disconnectObject), while the code that emits the signal should be added to AutomationPattern::setRecording.
• useControllerValue would return true if Engine::getSong()->isPlaying() is false OR if m_isRecording is true. Otherwise it would return false (a.k.a. controller values will be used when the song is not playing or when the song is playing but we are recording values). It should also return true if the model isn't automated at all, maybe using AutomatableModel::isAutomated() (the linked example doesn't do that).
• Then you can use useControllerValue on AutomatableModel::value and AutomatableModel::valueBuffer as you did.

That logic can be used just for MIDI controllers as you're doing on your PR too, though I don't think it's strictly necessary: The user should know that connecting a model to a LFO controller and having automations simultaneously will cause conflicts, so having the automation override the LFO is just one of the alternatives, no better and no worse IMHO.

Here's the example, it's very raw and needs some improvements, but gives a broad idea:
https://gist.github.com/IanCaio/e358fc680b04c47f51d431c479640e01/revisions

I'm at the disposal to help if you'd like to give this alternative a try!

[serdnab]

Sorry for the delay.
@IanCaio I am considering your suggestions, but the link to the example code isn't working.

In line 397 from src/core/AutomatableModel.cpp (AutomatableModel::setAutomatedValue) you add the requirement that the linked model isn't connected to a controller before notifying it. What was the reason behind that? I ask because it doesn't seem to be the behavior before and I didn't figure out any reasons to have that condition.

I assumed (maybe wrong) that controllers would have to take precedence over linked automations.

From what I gather from the logic there's no way to record Automation Patterns using a MIDI controller

If an automation pattern is recording, Song::processAutomations() doesn't call setAutomatedValue(), so it records normally. Here.

As for the other points you made I would like to see your example code to comment.
Thanks for your observations.

[IanCaio]

Sorry for the delay.
@IanCaio I am considering your suggestions, but the link to the example code isn't working.

No problem!
Sorry about that, I think I cleaned my gist files a few days ago and probably deleted that one too. After I wrote the review I wasn't sure you were still active, so I tried to work on an alternative approach. I didn't submit it as a PR though in case you'd answer. Here's the link to it. In the end, there were some caveats that I had to come up with a solution for (as you might see in the commit history), but overall I think it's a little bit more contained in terms of where's the code handling this.

If it happens that you like this alternative solution and want to apply it here you can just copy it, I don't mind authorship at all and this has been your PR from the beginning, the approach I came up with even though different was derived from your work anyways.

I think other devs should share what they think as well though, I shouldn't be the only one weighting in.

I assumed (maybe wrong) that controllers would have to take precedence over linked automations.

I wasn't sure neither, later I found out that linked controllers are an abstraction of those LADSPA plugins linked channels, where one channel's parameters are linked to the other channel's parameters (to be used with Lv2). Then I believe it makes sense they have precedence over the controllers.

If an automation pattern is recording, Song::processAutomations() doesn't call setAutomatedValue(), so it records normally.

I missed that, ignore that point then! 😁

[serdnab]

@IanCaio I checked your code and I think it's a more elegant approach, but this PR approach I think is more flexible, e.g. :

Before entering any automation patterns you can use the midi controller and then play / listen to those automation patterns all in one song play. This avoids deleting / muting the automation patterns in order to use the midi controller.

If there are recording patterns between other automation patterns, you can record after playing / listening to the automatic patterns, also all in one song play.

[IanCaio]

@IanCaio I checked your code and I think it's a more elegant approach, but this PR approach I think is more flexible, e.g. :

Before entering any automation patterns you can use the midi controller and then play / listen to those automation patterns all in one song play. This avoids deleting / muting the automation patterns in order to use the midi controller.

If there are recording patterns between other automation patterns, you can record after playing / listening to the automatic patterns, also all in one song play.

I haven't tested both in a while but from what I remember of the code you're right, in terms of UX I think your approach behave better. Still, if we are to use it, I think we should find a way to reorganize the code in a clearer way to future readers, in its current state it can be a bit confusing.

I'll read the diff again and try to think of something.

[serdnab]

I made an improved version of this PR. Here.
I centered the emission of the Song::stopped() signal and the change of AutomatableModel::setAutomatedValue() to AutomatableModel::setUseControllerValue().
What do you think?

[IanCaio]

I made an improved version of this PR. Here.
I centered the emission of the Song::stopped() signal and the change of AutomatableModel::setAutomatedValue() to AutomatableModel::setUseControllerValue().
What do you think?

Nice! I like the changes, they made it easier to understand than the previous version.
There are a couple of things I think could be changed further to simplify it even more:

• Treating all controllers the same, instead of having this behavior only for MIDI controllers. I asked other devs and the one that answered agreed that makes sense to treat LFO controllers the same way as MIDI controllers, that way you'll be able to remove some code and logic related to checking if a controller is MIDI or not.
• I'm double checking if linked models are being handled correctly. To be fair, the issue with this part of the code being a bit confusing isn't related to your PR. I think linked models should be handled separately from controllers, which they currently are not. That's something for us to think about changing in the future, but as far as this PR goes, I think it's handling it correctly, or as good as it can with the current code layout. I'm a little in doubt about the if (!m_controllerConnection) conditional before the linked models logic on valueBuffer though, think it might not be necessary. I'll look further into it.

For now that's what I noticed, could you commit those changes to the PR? Then I can help you out by suggesting the changes for removing the MIDI logic for example.

[serdnab]

In line 397 from src/core/AutomatableModel.cpp (AutomatableModel::setAutomatedValue) you add the requirement that the linked model isn't connected to a controller before notifying it. What was the reason behind that? I ask because it doesn't seem to be the behavior before and I didn't figure out any reasons to have that condition.

Moving the linked button of a button with a controller doesn't affect it, so I think it's not very intuitive for automation to do it.That was the behavior before this PR, so I added that clause because with the code changes it was necessary to keep the old behavior (forgot to mention that).

I will commit the new version and remove the specific midi controller code soon.

[qnebra]

Hopefully it would still work after commiting new version.

[IanCaio]

Moving the linked button of a button with a controller doesn't affect it, so I think it's not very intuitive for automation to do it.That was the behavior before this PR, so I added that clause because with the code changes it was necessary to keep the old behavior (forgot to mention that).

That happens because the linked models are being handled on controllerValue and it returns earlier if there's a controller connected to the model. The code related to linked models will have to be fixed soon, as I think it misbehaves because of how it's currently implemented. Just an example I found while testing here:

1. Add a GLAME Butterworth Lowpass effect and leave the cutoff controls linked
2. If you automate one model, the other will follow
3. If you add a controller to the second model the other will not follow. But if you automate any of the two models while the second has a controller, the first will follow the controller values
4. If you add a controller to the first model the second will follow even without any automations.

The current behavior is messy, I'd advise we touch this the least possible so we have less code to fix later. I still need to figure out which parts of your code that adds conditionals to the linked models are necessary now and which might not be. Again, this problem comes from before your PR, I'm just trying to make sure it doesn't add complexity for something that will have to be fixed later! Thanks for being patient with my reviewing 😬

I will commit the new version and remove the specific midi controller code soon.

Awesome, that will definitely make it even simpler! Let me know when you commit it by commenting here. While you're at it I'll be still trying to figure out that last linked models issue, and once that's done I think it will be ready for merge.

[serdnab]

Done. I commited the code.

[IanCaio]

Left another review on the updated code. It's being hard to review the parts related to linked models because I think their current implementation needs some fixing, but I'm trying to make sure the behavior is close to the expected.

Do you happen to use Discord? Maybe using LMMS server Dev channels would make discussions about this PR faster than Github comments.

[IanCaio]

Suggested change

	if (!((*it)->controllerConnection()) && (*it)->m_setValueDepth < 1 &&
	if ((*it)->m_setValueDepth < 1 &&

Considering a strict link between models, if one model is affected by the automation its linked models probably should have their values set as well, even if they are connected to a controller.

However, that suggested change has a side effect I still don't know how to deal with: Currently the m_useControllerValue variable is set through signals, one of them being triggered when an automation ceases being part of a particular time frame of the song being played. If the linked model has setAutomationValue called m_useControllerValue will be set to false. But since this was triggered from the automation of another model, there will be no signal throughout the rest of the track to set m_useControllerValue back to true (the code using m_oldAutomatedValues), only when the song stop.

I'm not sure that side effect is much of an issue, since the models should be strictly linked, meaning that if one of them is automated the other should follow its value as well. That is not true even before that PR: controllers end up preceding linked models, which goes back to the matter of having to reorganize the code of linked models.

[serdnab]

However, that suggested change has a side effect I still don't know how to deal with:

We can notify the linked models in setUseControllerValue().

But before we get into the linked code, I think we need to decide whether to keep the old behavior or to touch it a minimum (even if this is buggy) to fix it later.

[IanCaio]

I think that would do it, but you're right. I'll ask other devs what they think. Maybe it would be simpler to keep the code as you wrote it and fix the linked models behavior later.

[IanCaio]

I think that conditional could be removed. If this model has a controller, but the value of the controller wasn't used (useControllerValue == false, meaning previous conditional was skipped), shouldn't it check for the linked models?

[serdnab]

If useControllerValue == false it means that it's playing an automation pattern so it shouldn't check the linked models

[serdnab]

Do you happen to use Discord? Maybe using LMMS server Dev channels would make discussions about this PR faster than Github comments.

I prefer to keep the discussion here, I'm not in a hurry.

[IanCaio]

I prefer to keep the discussion here, I'm not in a hurry.

No problem! I'll check with other devs about the linked models issue and let you know what they say. Once that last detail is decided we can ask people to test it again and approve it.

Sorry about the nitpicking on the linked models code, I know it's currently a mess so this isn't your PRs fault at all. Just trying to look on the long run what will make fixing the code less demanding in the future.

[IanCaio]

I didn't receive much feedback on the Linked Models issue. I'm approving those changes since I think the code was improved a lot in terms of being clearer.

One note not directly related to this PR: We will have to refactor the Linked Models code in the future, it's not very well designed IMHO and overlaps too much with the controller code. Because of that it's hard to make changes on the way we handle controllers without affecting the linked models, as can be see on the reviewing history.

That aside, I think this is good code-wise and ready for testing.

[qnebra]

Quick test:

• It still works really fine with midi controllers, main function of PR is still preserved.

• It works also with LFO controllers, but I would recommend manually disconnect current LFO controller to leave only automation as source of LFO.

Not big deal and doesn't break overall functionality. When "automated LFO" and "controller LFO" for example had different 'speed' values during playing some sort of "values fight" occurs. I think mainly because LFO constantly sends signals to connected parameters. Not big deal, and for me not related to this PR. It was more in terms of how LFO works in lmms "as is".

[IanCaio]

@serdnab Thanks for the work and patience with the reviewing! And sorry about the delay with merging, I missed that there was a second approval and only recently noticed it.

[IanCaio]

It's possible that I'll have to revert the commit that merges this PR. I noticed a bug, where some projects will crash when they are reloaded and the Play button is pressed. The backtrace points to setUserControllerValue and I can reproduce the crash reliably with a build that has the PR merged, but can't reproduce it without it. Unfortunately I still can't figure out the reason for the crash, and it also depends on something specific in the project for it to be triggered (I've 3 personal projects I know I can reproduce the crash with), but the fact that it happens with the PR merged but not with the commit immediately behind it means that something likely went unnoticed.

Thread 16 "Mixer::fifoWrit" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fff94000700 (LWP 4918)]
doActivate<false> (sender=0x55555b5a3530, signal_index=3, argv=0x0)
    at kernel/qobject.cpp:3754
3754    kernel/qobject.cpp: No such file or directory.
(gdb) backtrace
#0  doActivate<false>(QObject*, int, void**)
    (sender=0x55555b5a3530, signal_index=3, argv=0x0)
    at kernel/qobject.cpp:3754
#1  0x000055555575dbef in Model::dataChanged() (this=0x55555b5a3530)
#2  0x0000555555778802 in AutomatableModel::setUseControllerValue(bool)
    (this=0x55555b5a3530, b=true)
#3  0x0000555555817345 in Song::processAutomations(QVector<Track*> const&, TimePos, short) (this=0x555556424280, tracklist=..., timeStart=...)
#4  0x0000555555816c95 in Song::processNextBuffer() (this=0x555556424280)
#5  0x00005555557e2da8 in Mixer::renderNextBuffer() (this=0x555555e9a400)
#6  0x00005555557e5b23 in Mixer::fifoWriter::run() (this=0x7fffec017650)
#7  0x00007ffff5b97375 in QThreadPrivate::start(void*) (arg=0x7fffec017650)
    at thread/qthread_unix.cpp:342
#8  0x00007ffff7f91609 in start_thread (arg=<optimized out>)
    at pthread_create.c:477

[qnebra]

Can you send a project, or describe how to reproduce it? Just wanted to check if it crashed for me too.

[qnebra]

I can confirm there is something or introduced by this PR, or exposed by this PR, which can cause crashes in specific workflow.