Lmms: After linking models, values are swapped instead of being the same

[JohannesLorenz]

There's a "Link Channels" LED in LADSPA mono effects. The following does not work:

• Create a LADSPA mono effect with >= 2 knobs
• Unlink knob 1, change its value on one side
• Change the value of knob 2 (it's currently linked)
• Hit the "Link Channels" LED 2n+1 times for any n>=0 (e.g. once), and knob 1 is displayed as linked, but the knobs show different values and the models do have different values

This sounds like a bug to me, because the LED looks like that knob was linked.

The issue also occurs on the Lv2 branch, since the code was copied from LADSPA.

[JohannesLorenz]

Can anyone confirm that this occurs, and consider it a bug?

It's hard to believe to have a bug in there for such a long time...

[karmux]

I can reproduce it already with a single click to "Link Channels" LED.

But for me there's also some drawing problem. I need to reopen LADSPA modal window or cover knob 2 with other modal window for a moment to visually see different values of knobs when they are linked.

Knob values get switched. Left knob gets the value of right knob and vice versa from what I see after redrawing the modal window. I would expect to see both knobs have the value of knob 2.

[PhysSong]

It seems like there's a bug with AutomatableModel::linkModels. You can trigger a similar bug without any LADSPA plugins:

1. Add two instrument tracks
2. Set the volume of the first track to 100% and the second one to 0%
3. Drag the volume knob of the first track, and drop it to the second one
4. Notice the volume of each instrument is changed

It means this bug has nothing to do with LADSPA stuff. This one looks like a design flaw in AutomatableModel.

[PhysSong]

If we try to link model A and model B. Let's consider some situations:

1. User Ctrl+drags A and drops on B
2. A is for the left channel of a mono LADSPA effect, and B is for the right channel (the original case)
   Assuming the two models should have the same value after linking, which value should we take?

[JohannesLorenz]

@PhysSong I'm not sure why you changed the title. In the OP, I can't see where values are swapped. They are not changed visibly at all.

Also, what do you mean by "B is for the left channel(the original case)"? Do you mean B is a knob of a left channel of another LADSPA stereo effect?

[PhysSong]

In the OP, I can't see where values are swapped. They are not changed visibly at all.

Try this:

1. Add a LADSPA mono amplifier
2. Unlink channels
3. Set the gain of the left channel to 0
4. Re-link the channel

You can see that the values effectively gets swapped. The visualization doesn't reflect this as @karmux reported, though.

Also, what do you mean by "B is for the left channel(the original case)"?

Sorry for the typo, it's the control for the right channel of the same effect.

[JohannesLorenz]

You can see that the values effectively gets swapped. The visualization doesn't reflect this as @karmux reported, though.

Wow, didn't see this until now!

User Ctrl+drags A and drops on B
A is for the left channel of a mono LADSPA effect, and B is for the right channel (the original case)
Assuming the two models should have the same value after linking, which value should we take?

As dragging a knob in LMMS usually means copying the value of that knob somewhere else, I'd expect that the resulting value for both would be the value of "A". I'd expect this both if A is on the left and if A is on the right side, because you probably want a fast way to set the values of both channels to either A or B.

[JohannesLorenz]

As dragging a knob in LMMS usually means copying the value of that knob somewhere else, I'd expect that the resulting value for both would be the value of "A". I'd expect this both if A is on the left and if A is on the right side, because you probably want a fast way to set the values of both channels to either A or B.

@PhysSong I want to change my opinion. Both should have the value of B, because: If you drag a knob on an automation pattern, you want the knob to be automated by the pattern. So if you drag a knob A on a knob B, you want the knob A to be automated by knob B. Can you agree?

[JohannesLorenz]

@PhysSong I did some debugging. AutomatableModel::m_value is never being changed during the linking - not for any of the two. However, AutomatableModel::linkModel pushes back the other model to m_linkedModels for each, and then AutomatableModel<float>::value() returns AutomatableModel::controllerValue(), which returns m_linkedModels.first(). This should explain the crossover values. Do you think that explenation is correct?

[JohannesLorenz]

I'd like to provide a fix, but there are a few points...

1. As PhysSong asked above, should the shared value of dragging A and dropping on B be the one of A or the one of B?
2. If you unlink the controls, should both controls keep the shared value, or should the one that got its value removed (at the time of linking) now get its old value back?
3. There is currently no way to unlink the controls (at least I don't see any). I think it should be added to the knob's right-click-menu? -> Moved to #5285

Maybe this requires a poll :smile:

[JohannesLorenz]

Poll:

• Dragging one knob onto another, you can link both. Should the shared value of dragging A and dropping on B be the one of A (:eyes:) or the one of B (:heart:)?
• If you unlink two knobs, should both controls keep their shared value (:tada:), or should the one that got its value removed (at the time of linking) now snap back to its old value (the one before removal) ? (:rocket:)

Discussions are also possible and may outplay the voting.

[softrabbit]

You ctrl-drag to put a knob under automation control, so dragging A to B should make A the slave.

When unlinking, I'd be pretty surprised if the slave knob snapped back to a value it might've had before a lot of twiddling happened, maybe days ago.

[JohannesLorenz]

You ctrl-drag to put a knob under automation control, so dragging A to B should make A the slave.

True, but some people have noted that ctrl-dragging an automation pattern to a knob should be enabled and do the same as ctrl-dragging a knob to an automation pattern.

[Spekular]

Intuitively for me, I would expect to drag the Master knob onto the Slave. This is also how Serum behaves. It also means there's no ambiguity about what happens if I drag a knob onto several other knobs, the first simply controls all the rest.

I can't deny that this would be inconsistent with the current automation behavior, but I think its worth it.

[JohannesLorenz]

Thanks for voting! Looks like most people want that dragging A on B makes B take A's value.

Now, to the implementation:

• First, I thought the information about who is slave and who is master is only required at linking time, since they act equally afterwards. However, imagine that a user has two knobs linked, has one knob controlled and now tries to control the other knob. Having a hierarchy will make this easy to prevent. So I'd make m_linkedModels somehow differ between linked slaves and linked masters.
• For LADSPA, I'd simply choose the left knob to be the master if a link LED is being clicked.
• The bug is then being fixed by letting AutomatableModel::controllerValue() only use the first master in m_linkedModels, and in case none exists, return its own value.

@PhysSong are you OK with this?

I'd push a PR the next days to test if that works.

[JohannesLorenz]

To reflect PR #5336: This PR is a minimal solution to this issue (more minimal than I planned above). Minimal but sufficient, so it fixes the bugs but does not have large impact on the stable code branch.

[PhysSong]

@JohannesLorenz Do you mean you want to add a hierarchy to AutomatableModel eventually?

[JohannesLorenz]

@PhysSong Not in the fixing PR. Like mentioned, the PR is just a minimal solution. A hierarchy would be nice, but after the PR I think we have more important issues.

[JohannesLorenz]

Solved by #5336