First version of a new dynamic LADSPA control dialog

[michaelgregorius]

The new dialog shows the LADSPA controls in a matrix layout. Each row
corresponds to a LADSPA parameter. Each parameter can have several
channels which can be linked. Each channel has its own row of controls.

Some base classes have been extended to enable the resizing of the new
control dialog. A new virtual method isResizable was added to the class
EffectControlDialog. The default implementation returns false which means
that for all other types of dialogs the code in EffectView.cpp will still
set the dialogs to a fixed size. LadspaMatrixControlDialog returns true
so the code is skipped. A better solution would be to remove the fixed
size code from EffectView.cpp altogether and to make it an implementation
detail of each dialog whether it is resizable or not.

The class LadspaMatrixControlView was added by copying and modifying
LadspaControlView to get rid of the link buttons which are associated
with some controls and some labels.

Here is a screenshot showing the dialogs in action:

[diizy]

looks good, but we're under feature freeze so this'll have to wait until we separate 1.2 from master

[Wallacoloo]

The Calf EQ currently looks like this for me:

I have to say, I like the current look better than the look in your screenshot. I'm not familiar with the code used to create it though - are the knob positions currently manually programmed and your change makes these dynamic and automatic? If so, excellent and 👍.

But I don't think having one parameter per row looks right. For example, it makes quite a bit more sense to have "HP Active", "HP Freq" and "HP Mode" on a single line, as they are currently, rather than split across rows like this. Is this part of the end goal?

[michaelgregorius]

Hi @Wallacoloo,

the knob positions and the layout are computed dynamically in both implementations. But I think the layout in my new implementation is a bit more tidy. For a comparison I have attached a screenshot with the other two dialogs:

Please notice that all of my screenshots have been made on a display with a DPI of 117.5 so the font may look rather large on other screens. Here is what I find inadequate about the current implementation:

• For plugins with more than one channel the parameter names are repeated which adds to clutter.
• Knobs do not always line up which also adds to a cluttered look.
• Having the link button for each parameter next to the control of the first channel made the layout/groups look unbalanced.
• The unbalanced layout makes it harder to find a corresponding control in another channel because the first and the second channel do not look identical.

I agree that having "HP Active", "HP Freq" and "HP Mode" on a single line is a better grouping in the case of the Calf EQ. This is something that could be improved in the new implementation. I have to check the old code again to see whether LADSPA gives an indication of what should be considered a group and what not. I can also imagine later implementations to have some layout configuration file which describes for a given plugin what type of control should be used for a parameter (knob, slider, etc.) and how they are grouped or layouted. But that's quite some big undertaking which should not be part of this pull request (one step at a time).

[Wallacoloo]

@michaelgregorius I agree with all the inadequacies you pointed out in the current implementation. Given the direction you intend to continue with this, I'm in support of merging this (after 1.2 is forked). The code looks good - no glaring style issues. Some sections could be commented better, but that's me being nitpicky.

[badosu]

Hi @michaelgregorius, really nice to see someone taking a look at the LADSPA view.

These issues might interest you in future iterations:

• Supporting output meters (Missing Meters in FX GUI #75)
• Support additional control data (LADSPA RDF Support #1717)

[Wallacoloo]

Seeing as how we have not been enforcing a feature freeze on any other PRs and we have no clear timeline for procuring a 1.2 release candidate, should we merge this one?

[tresf]

No objections here.

[budislav]

I don't want to be boring with this but in case you didn't notice my work on LADSPA plugins in single-window UI

Old control dialogs are better because of horizontal scrolling. New matrix layout is not good for single-window because dialogs have fixed height.

[Wallacoloo]

@budislav I don't follow 100%. Take the old Calf EQ layout shown in @michaelgregorius 's comment earlier: #2068 (comment) . As is, it seems that both the new implementation as well as the current one are ill-fitted to being placed in a horizontal scrolling environment like that in your proposal.

For what it's worth, I don't imagine that very much of the existing LADSPA layout code will be compatible with your UI proposal anyway. Unless I'm mistaken, that portion of it will pretty much be a rewrite when based against either the current code or the code in this pull request. So in my opinion it's more important to question whether this pull request will improve the existing UI and/or make the code easier to deal with in the future.

I don't mean to discredit your UI proposal or to offend - my perspective is one in which I value small, incremental improvements in order to make growth more manageable at the cost of these improvements occasionally being redundant.

[michaelgregorius]

@Wallacoloo I agree 100% on the value of small, incremental improvements and doing things in a rather agile way. I also agree that this code is something that will have to be rewritten anyway due to the dynamic nature of the dialogs.

@budislav Are you aware that the LADSPA dialogs have dynamic layouting, i.e. they are build at runtime and that it is not possible to design them upfront as in your examples?

[tresf]

@budislav Are you aware that the LADSPA dialogs have dynamic layouting, i.e. they are build at runtime and that it is not possible to design them upfront as in your examples?

FYI - His layout is dynamic too. His W/D, DECAY and GATE are from the effects chain.

His channel stack is already being performed.

... and his linking icon should be possible as well.

So I think his request is rather valid from a layout perspective. It's good foresight to take the future designs into consideration, even if we don't code them now.

[budislav]

@tresf thank you. Exactly.
@Wallacoloo for LADSPA layout every UI pattern like checkbox or slider would be placed one after the other but because of fixed height everything will just go to the right. Width is not fixed in this case.
@michaelgregorius I don't see anything impossible here. It only require gui logic.

[Wallacoloo]

for LADSPA layout every UI pattern like checkbox or slider would be placed one after the other but because of fixed height everything will just go to the right. Width is not fixed in this case.

@budislav I understand that part. Perhaps I misunderstood your earlier post. I was under the impression that you were against merging this pull request, specifically for the reason that the height of the layout is not fixable in @michaelgregorius's implementation. This confused me because one cannot to my knowledge fix the height in the current implementation either, and if that's correct, then how could one use that as a reason to advocate against this PR?

So I have two clarifying questions to ask you:

• Was your post intended as an objection to merging this pull request?
• Can one fix the height of the existing layout implementation? (I would check myself, but I don't have the facilities to run LMMS at this moment)

And if your answers are "yes", followed by "no", then I'd also ask that you elaborate on why you think this pull request is a downgrade from the current implementation.

[budislav]

@Wallacoloo

Was your post intended as an objection to merging this pull request?

No, I misunderstood, it's my fault. I thought that these changes would be unnecessary because in single-window concept LADSPA is designed in mind of present design (no matrix).

[michaelgregorius]

@budislav I guess it will take quite some time to implement the new GUI and as far as I know there are no concrete plans when this will be done/started. I we would stop implementing other stuff because it might be superseded by something else later on then development would come to a grinding halt immediately. 😉

By the way, I like your designs very much. 🎨

[tresf]

... we also need better widget overflow support (toolbar suffering this now) to really have spillable columns content. Hopefully the ZynGUI2 will help provide some of Qt's missing layout managers.

[Wallacoloo]

So should we merge this then? (after somebody pulls the branch, runs $ rebase master and then tests it again?)

[tresf]

@Wallacoloo I think you and @michaelgregorius know best. I fathom introducing this will make future changes easier anyhow.

[michaelgregorius]

@Wallacoloo I have just rebased and tested against master and now it seems that the LADSAP dialogs also suffer from the problem described in #2288, i.e. they open in a very small state. Another problem (I think it was also present in the original pull request) is that the dialogs have no minimal horizontal size and the layouting in general could still be improved, e.g. when making the dialogs very large.

So currently I would advise against a merge.

[tresf]

open in a very small state

@michaelgregorius this should be fixed via #2358 right?

[michaelgregorius]

@tresf Sorry, I cannot test this at the moment because LMMS (still) cannot find the LADSPA plugins. There was a bug report about this but I cannot find it anymore. To reset everything to a clean slate I have removed my lmmsrc.xml and started LMMS with an installed version (make install) but it doesn't find the plugins with the default paths. It doesn't even find them if I point the LADSPA folder to only use the system wide installed plugins (/usr/lib/ladspa/). Any tips?

[michaelgregorius]

@tresf Ok, I was able to check it. Unfortunately the problem is still there.

[tresf]

@michaelgregorius so the LADSPA issue is fixed, but the dialog sizes are still off?

[Wallacoloo]

@michaelgregorius Make sure you're correctly matching 32/64 bit. Somehow, I have both 32 and 64-bit versions of the ladspa plugins on my system, at /usr/local/lib32 and /usr/local/lib. Point lmms to the appropriate directory if you do too. You can verify the architecture of both lmms and the ladspa plugins by using file ./lmms, etc.

[michaelgregorius]

@tresf Yes, exactly. The LADSPA problem seems to be a build issue when building with Qt Creator. The dialog sizes are still off.

[tresf]

@michaelgregorius bumping to 1.3 milestone. Fine to merge sooner if the rebase and testing occurs. 👍

[michaelgregorius]

I can't say I'm a fan of this. Visually I think it looks better at the first glance. But it will waste space (especially on a smaller screens), and on closer inspection, the double title feels a bit weird.

Commit 7c98513 removes the title.

I would even go in the opposite direction and remove the "Channel 1" label if there is only one channel, since it does not really serve any purpose. That could make the title bar feel more like a part of the interface.

I propose to do this once this pull request is merged.

If font size is the problem, maybe the window title should be made larger? I.e. I basically see it as yet another scaling issue.

Yes, it's indeed another scaling issue but fixing the window titles is not in the scope of this pull request.

Also, could the minimum window width be reduced perhaps? Some of the plugins like Aliasing look very oversized, I would much prefer resizing it to around half its default size. You could easily fit 5 Amplifiers into the space it currently occupies:

Commit 4670db0 reduces the minimum width of the bar controller from 200 to 50.

In some cases like the AM pitch shifter, the Link column has also a blank space on either side, which could also instead be used to make the window more narrow. (Although that's probably some issue with the layout: the Channel 1 and Channel 2 columns do not expand when I resize the window. Strange that the two other windows work as expected.. EDIT: Oh, possibly related to the fact that the columns contain a LED checkbox, not just a slider.)

Ah, you're right. Thanks for the hint about the cause. 👍 This is fixed with commit 7ba0098.

[michaelgregorius]

[...] Also, maybe there isn't need for a space between the bar controllers?

What space are you exactly referring to? The space of the grid?

[zonkmachine]

[...] Also, maybe there isn't need for a space between the bar controllers?

What space are you exactly referring to? The space of the grid?

Yes. I couldn't find a setting to tweak to test this but the space of the grid. Maybe it works to not have a space in between at all. Especially vertically.

[michaelgregorius]

@zonkmachine, you can use QGridLayout::setHorizontalSpacing and QGridLayout::setVerticalSpacing to experiment with this. I have just experimented with some values and IMHO no spacing looks absolutely horrible. 😅

I think there should be horizontal and vertical spacing so that the elements are distinguishable.

Here's with a spacing of 3:

And here's the default spacing (I think it's 6):

[zonkmachine]

I have just experimented with some values and IMHO no spacing looks absolutely horrible.

And I would have to agree... 🧙‍♂️

[zonkmachine]

Although, setting the bar background transparency to 0 looks pretty nice.

 lmms--gui--BarModelEditor {
-       qproperty-backgroundBrush: rgba(28, 73, 51, 255);
+       qproperty-backgroundBrush: rgba(28, 73, 51, 0);
        qproperty-barBrush: rgba(17, 136, 71, 255);
 }

[tresf]

Although, setting the bar background transparency to 0 looks pretty nice.

I think a range should always have a visible min/max.

[zonkmachine]

I think a range should always have a visible min/max.

I could probably get used to a theme that used the bar graph controllers with no background. It would be a bit enigmatic if no bar graph at all meant 0, especially if all values are set to 0, but in this case 0 has a tiny sliver of color left. 2 bits wide or so. I'm not about to propose it for a default theme but I think it looks nice.

[michaelgregorius]

I agree with @tresf. People must be able to see that there's a control if the value is set to the minimum (at least in the default theme). Otherwise the text might just look like static labels to them.

[dead-claudia]

I agree with @tresf. People must be able to see that there's a control if the value is set to the minimum (at least in the default theme). Otherwise the text might just look like static labels to them.

And I've seen a few LADSPA plugins in the wild that had labels so poorly thought out as to pass for static labels. This isn't a theoretical.

[michaelgregorius]

Hi @Rossmaxx, any chance that this PR might also make it to 1.3 with regards to #6949? In my opinion there is nothing more to add to this PR.

[Rossmaxx]

I don't know. The devs have the final decision. I labelled the changelog temporary for a reason. Also, it contains only merged changes (with notes which contain changes which are likely to make it.) I can add this pr to the "might be in 1.3" list but doesn't mean it will make it to 1.3 because like i said, the devs should decide that.

[zonkmachine]

OK. Pushed it to 1.3

In my opinion there is nothing more to add to this PR.

Well, it has only been reviewed for some eight years. 😢
The latest layout fixes I think made a lot of difference and I think it's ready to merge.

[michaelgregorius]

Thanks for adding it @Rossmaxx, @zonkmachine! I was referring to the "might make it for 1.3" section anyway. 🙂

Which devs will/can make the decision? I am only asking because the LMMS organization currently lists 44 members/owners but I think several of them might have been inactive for quite a while. It's not really clear to me who's in the core team with some "decisional power". Is it the owners?

[tresf]

Which devs will/can make the decision?

@zonkmachine has raised the question on Discord. 🤞

[zonkmachine]

@michaelgregorius I asked on Discord and there was no objections. This PR has been approved for merge.

[michaelgregorius]

Thanks for managing this, @zonkmachine. I have finally merged this pull request. 🥳

Also thanks to @PhysSong for bringing this branch back up to speed after the code changes about a year ago.

And thanks to everyone else involved of course!

[kamkamkamkamkamkamkamkam]

Does anyone think we can have this for LV2 effects as well until GUI support is finished?

x42 DPL is still kinda fugly...

I'd rather it looking more like Fast Lookahead Limiter.

[zonkmachine]

@kamkamkamkamkamkamkamkam Yes. This has been suggested here; #6798

[zonkmachine]

@michaelgregorius This PR seem to have made the logarithmic response not have any effect. I can set a logarithmic value and it will save and load but the response seem to be linear.

[michaelgregorius]

@zonkmachine, I have checked the difference between Knob and BarModelEditor with regards to the painting code. One difference is that Knob uses AutomatableModel::inverseScaledValue whereas BarModelEditor uses AutomatableModel::value.

The fix seems to be to replace the following line

auto const percentage = range == 0 ? 1. : (mod->value() - minValue) / range;

in BarModelEditor::paintEvent with

auto const percentage = range == 0 ? 1. : (model()->inverseScaledValue(model()->value()) - minValue) / range;

This then reproduced the strange behavior that I have noticed in the old LADSPA dialog while testing this and where I first thought it was a bug. The problem is that linked channels do not necessarily show the same values because one control can be set to "linear" while the linked control is "logarithmic". Steps to reproduce:

1. Start with an empty project.
2. Add a LADSPA effect to the master channel, e.g. "C*PreampIV".
3. Set the "gain" parameter to logarithmic.
4. You should already see a discrepancy between the two linked channels.
5. Change the values of the left channel. You will find that the discrepancy is there until the min or max values are reached.

The same can be observed in the old LADSPA dialog which can be reactivated with the following change in LadspaControls.h. Adjust createView as follows:

gui::EffectControlDialog* createView() override
{
	return new gui::LadspaControlDialog( this );
	//return new gui::LadspaMatrixControlDialog( this );
}

[zonkmachine]

This then reproduced the strange behavior that I have noticed in the old LADSPA dialog while testing this and where I first thought it was a bug. The problem is that linked channels do not necessarily show the same values because one control can be set to "linear" while the linked control is "logarithmic". Steps to reproduce:

This issue: #4936 👍