Fix knob linking / refactor linking

[szeli1]

closes #7869

This PR aims to fix linking bugs and it aims to make linking code faster. In the future I would like to replace controller and automation code with linked models, so it is essential for this feature to work as efficiently as possible.

Before this PR:

1. AutomatableModels store a list of AutomatableModels that they are linked to.
2. setValue() and other functions make recursive calls to themself resulting in the #7869 crash
3. Each AutomatableModel can unlink form other AutomatableModels, unlinking is the inverse operation to linking.

After this PR:

1. AutomatableModels store a pointer to an other AutomatableModel making a linked list. The end is connected to the first element resulting in a "ring"
2. setValue() and others are now recursion free, the code runs for every linked model, more efficiently than before.
3. Each AutomatableModel can NOT unlink form other AutomatableModels, unlinking is NOT the inverse operation to linking. AutomatableModels can unlink themself from the linked list, they can not unlink themself from single models.

Issues:

• Const AutomatableModel::value() uses a hack non const setValue() to update linked widgets if a controller is connected.
• Linked widgets do not clamp each other unlike before this PR.

How to test:

1. Press ctrl and drag a Knob onto an other Knob. They should now be linked.
2. Change the first knob's value, the second knob should change.
3. Click on "remove all linked controls" for the first knob, if done, the knobs should be unlinked.
4. Repeat these steps and link together combo boxes, mute buttons, "lcd" displays, and anything that can be automated.
5. Make sure to link different kinds of widgets together.
6. See if #7869 is fixed.

[firiox]

Hey Bro! Nice job I was just testint It and I have a problem I think. When you link mixer channels to the volume fader It changes them so fast that It was jumping for o to 4 really fast thought I think a better way to do this would be to divide the volume range by the number of mixers channels so It is more responsive I guess I don't know if thats challenging but I guess so anyway I got ninja and could test it that for each volume step mixer channels got update right so this is very subjective right but I think It'll feel alot better. Another thing following up the testing I added the mute toggle to the volume and mixer channel link and It got gimmick like the mute toggle gets toggled before the mixer gets it's first value It is kinda strange like it's not the same method for changing 1 value for the mute toggl and the mixer channel kinda feels off Do you understand me? anyway It's a step forward and this is just final tunning I guess. So to be more clear you can do a half step and the mute toggle gets triggered but the mixer channel is still in 0 and then another half step and the mixer channels finally reaches his next value. Weird right? (It is kinda a ninja movement thought but puts me off)

[szeli1]

Another thing following up the testing I added the mute toggle to the volume and mixer channel link and It got gimmick like the mute toggle gets toggled before the mixer gets it's first value It is kinda strange like it's not the same method for changing 1 value for the mute toggl and the mixer channel kinda feels off Do you understand me?

This is because internally everything is stored as a float even the mute buttons, but these internal values do not range from 0 to 1, but from a custom min value to a custom max value. Each widget receives the same value and interprets it with their own min-max. For example the mixer channel lcd widget gets "2" as input and sends it over to the mute button, the mute button ranges from 0 to 1 so it clamps the "2" to "1" and turns on. If you connect a mixer channel lcd widget to a volume knob, the volume knob ranges from 0 to 200 but the lcd widget ranges from 0 to the number of mixer channels, for example 4. This leads to the behavior you are experiencing.

Its feels badly designed, I will see what I can do.

[regulus79]

I tested this, and it definitely seems to fix the bug.

Also it seems like you also addressed #7621, somehow.

One thing I noticed is that now if two knobs with different ranges are linked (like volume and panning), if you move one knob out of the range of the other knob (like moving panning to negative), it lets you do that and just sets the value of the other knob to within its own bounds (volume doesn't go below 0). However, on master, if you try to turn one knob past the end of a linked knob, it won't let you, it will stop you from turning it. If you try to turn panning below 0, it won't let you. Well, sometimes. Sometimes it would still let you do it if you did some complicated linking between multiple knobs.

Maybe that's a good thing in this PR? It feels like it probably gives users more freedom. Though of course it means if you move the other knob, the first knob will snap back, which might not be desirable idk. But if it's also in master sometimes... idk, this PR feels like a much cleaner system.

[bratpeki]

Tried to cause the crash described in the corresponding issue, and couldn't! :tada:

It seems the image for the "Remove all linked controls" is missing and I get this issue:

Error loading icon pixmap "edit-delete": "File not found"

I also observe this on master.

[bratpeki]

LCD linking to other LCD items isn't possible. Interestingly, you can link LCDs to knobs.

[szeli1]

Thanks for testing @regulus79 and @bratpeki

[szeli1]

LCD linking to other LCD items isn't possible. Interestingly, you can link LCDs to knobs.

Yes sounds like LCD widgets do not accept the link for some reason.

[szeli1]

Maybe that's a good thing in this PR? It feels like it probably gives users more freedom

I can make it work like in old lmms, but currently linking isn't saved so this new behavior won't change anyone's project and It will add an additional loop for setValue which would make setValue more computationally expensive. This isn't ideal for the next step: removing controller connection and replacing it with linking.

[bratpeki]

Error loading icon pixmap "edit-delete": "File not found"

I also observe this on master.

I find this very strange, TBH. Can someome check they're also getting this in the terminal when removing linking in the context menu?

[szeli1]

I find this very strange, TBH. Can someome check they're also getting this in the terminal when removing linking in the context menu?

I got this message also, there is no "edit-delete" file in the theme. I could fix it if you want it, but I'm unsure what kind of picture to use / make.

[regulus79]

I also get that message. edit-delete.png doesn't appear to exist in the default or classic theme, and doing git log -- data/themes/default/edit-delete.png gives nothing, so I'm not sure it existed in the past either.

Also, 1.2.2 doesn't appear to have the icon either, although I don't get any warning in the terminal.

[szeli1]

I added a new svg for unlinking @bratpeki

[bratpeki]

Cool! Any idea why the old one was missing? Or maybe it was never there?

[bratpeki]

I also get that message. edit-delete.png doesn't appear to exist in the default or classic theme, and doing git log -- data/themes/default/edit-delete.png gives nothing, so I'm not sure it existed in the past either.

Also, 1.2.2 doesn't appear to have the icon either, although I don't get any warning in the terminal.

I see!

[szeli1]

Thanks for the reviews! I will work on fixing the remaining issues.

[regulus79]

Also it seems like you also addressed https://github.com/LMMS/lmms/pull/7621, somehow.

Oh wait nevermind, I tested it again and maybe it didn't(?) If it did I was very confused how it would because you didn't touch any of those lines.

[bratpeki]

I would love to have knob-fader linking work both ways, but other than that, great PR. Fader-fader is whatever but if we plan to eventually make Peak Controller use an OUT fader, it would be nice to be able to drag the knob to the fader as well as the other way around.

[szeli1]

When linking two models, and connecting one to a controller and to an automation track, the one with the controller connection is OK but the other linked model freaks out. That didn't happen before.

This seems like an edge case scenario. I would like to fix this in the next PR, where controller connections will be removed and replaced with linking.

Thanks for reviewing this @allejok96

[allejok96]

This breaks #5657. That should probably be fixed. The other thing could be dealt with later, as you said.

[bratpeki]

When linking two models, and connecting one to a controller and to an automation track, the one with the controller connection is OK but the other linked model freaks out. That didn't happen before.

I totally forgot to test this, thanks! I'll verify.

These connections are not kept and that would be part of a future PR, IIRC. @szeli1 please confirm.

[szeli1]

This breaks https://github.com/LMMS/lmms/pull/5657. That should probably be fixed. The other thing could be dealt with later, as you said.

It can not be fixed currently. The next PR that removes controller connection could fix it.

[allejok96]

in the next PR, where controller connections will be removed and replaced with linking

Could you elaborate, in what scenario will they be removed? How will this work?

[szeli1]

Could you elaborate, in what scenario will they be removed? How will this work?

Controller connections will be removed, instead an "output knob" will be added that can be linked to other knobs. Controllers will receive these output knobs. The user could add a new "midi controller" and connect it to knobs just like before. I will fix the https://github.com/LMMS/lmms/pull/5657 issue by adding a "math controller" that allows 2 output knobs to combine their outputs.

[regulus79]

I may have found a bug

1. Add a lowpass filter
2. Add an lfo controller
3. Connect the lfo controller to one of the lowpass channel knob/bar things
4. One of the channels moves smoothly, the other one moves abruptly
5. If you play a note, LMMS crashes

[szeli1]

I may have found a bug

Thanks for finding this @regulus79 ! It was caused by the controller workaround (in AutomatableModel.h value()) and a JO bug.

[allejok96]

seems like an edge case scenario

I realized the purpose of useControllerConnection. If you want to record a MIDI controller you can connect the knob to the controller and then drag the knob to an automation clip and set the clip to recording mode. When you record, the knob should use the controller value, but when you play it should use the automation value.

It can not be fixed currently

I haven't investigated why this PR breaks that behavior, but can be fixed by this change in Song.cpp. There may be a better solution that doesn't call setUseControllerValue on every iteration. This also fixes the flicker bug I mentioned before.

 	// Checks if an automated model stopped being automated by automation clip
 	// so we can move the control back to any connected controller again
 	for (auto it = m_oldAutomatedValues.begin(); it != m_oldAutomatedValues.end(); it++)
 	{
 		AutomatableModel * am = it.key();
-		if (am->controllerConnection() && !values.contains(am))
+		if (am->controllerConnection())
 		{
-			am->setUseControllerValue(true);
+			am->setUseControllerValue(!values.contains(am));
 		}
 	}

[szeli1]

I haven't investigated why this PR breaks that behavior, but can be fixed by this change in Song.cpp.

Unfortunately controller values are ignored, so this doesn't fix the issue. I tested your suggestion 3 times and the flickering was fixed only one time.

As I've said there are plans that fix these issues in future PRs, I would rather not delay this PR more than needed.

How to record automation?

[allejok96]

How to record automation?

Connect a model to a controller and to an automation clip. Right click the clip and select Set recording. Play the song. Move/use the controller. The controller value is recorded.

Unfortunately controller values are ignored

If a model is connected to both a controller and an automation clip, the controller should be ignored, except when 1) playback is stopped or 2) the automation clip at playback position is in recording mode.

there are plans that fix these issues in future PRs

Are you sure it's not possible to solve this, at least in some dirty temporary way? It feels bad to merge a PR that breaks a feature, and say we will fix it later, just because we didn't understand or didn't want to investigate the issue. It may be really simple.

I tested your suggestion 3 times and the flickering was fixed only one time.

I don't mind the flickering as long as recording works like it should. Unfortunately I don't have access to a MIDI controller, so I just tested it with the LFO controller...

[szeli1]

If a model is connected to both a controller and an automation clip, the controller should be ignored, except when 1) playback is stopped or 2) the automation clip at playback position is in recording mode.

I meant that it is unknown where the information comes from. Models receive values both from automation and from controllers (and from user input) independently from each other, it is unknown what is causing the change in values.

Are you sure it's not possible to solve this, at least in some dirty temporary way? It feels bad to merge a PR that breaks a feature, and say we will fix it later, just because we didn't understand or didn't want to investigate the issue. It may be really simple.

It is called "nightly" for a reason. I could make a "hack" but I would rather not write bad code. value() already has a const cast in it. Please tell me how to proceed.

(edit: Also in the next PR, I'm going to add "output knobs", then I could just add this knob (or normal knob) to clips, so they can receive midi)

[allejok96]

Models receive values both from automation and from controllers (and from user input) independently

It doesn't matter, they should have some kind of order of priority. That's what useControllerValue() was designed for.

in the next PR, I'm going to add "output knobs", then I could just add this knob (or normal knob) to clips, so they can receive midi

If you add an output knob to an automation clip... would that mean you want to control the output knob with the automation clip? Then you are still facing the same issue with having two input values for the knob: MIDI and automation... Or would dragging an output knob to a automation clip mean you want to record that knob? Then dragging knobs to clips have two different meanings: control the knob or record the knob. Which is a bit confusing... I'm not against such a design. It's just... you'll have to think about how to solve this problem, either now or later, and it's better to do it now before breaking features.