show effect parameter units in parameter name label #11041

ronso0 · 2022-11-05T14:41:07Z

Follow-up for #11032

adopt LV2 units http://lv2plug.in/ns/extensions/units
get parameter units from LV2 plugins and translate them to enum EffectManifestParameter::UnitsHint
Note: custom unit constructors are ignored.
blacklist known, undesired custom LV2 units, log unknown units (to either include or ignore them)
translate EffectManifestParameter::UnitsHint to display strings
show units in parameter name label
trim parameter value decimals to avoid unnecessary widget expansion
add UnitsHint to built-in effects

Nice to have:

scale / convert units depending on range, for example convert Hz > 10.000 to kHz?

ronso0 · 2022-11-05T23:05:56Z

(rebasing & rebasing while testing QStringLiteral vs. QLatin1String in unitsHintToString until clazy is happy (and other builders, too))))

ronso0 · 2022-11-05T23:25:16Z

reload unit after swapping parameters

ronso0 · 2022-11-07T18:44:44Z

Unofficial LV2 units are now logged if lv2ParamDebug is true.
Anyone following this: please test this branch and report any custom units you consier helpful.
I only came across G for all sorts of gains / levels, and st for "Frequency shift" with LSP plugins 🤷

JoergAtGithub · 2022-11-07T21:05:10Z

I tried this, but couldn't find any parameter with unit other than Hz. I'm not sure if other units doesn't work, or if the effects have no unit specified.
I tried with built in effects and effects from https://github.com/DISTRHO/PawPaw/releases/tag/v1.1

ronso0 · 2022-11-07T21:26:24Z

Yeah, built-in effects have Hz, dB and ms IIRC.
The debug output only shows non-standard LV2 units.

daschuer

This works already nice. I have left some comments.

There is a minor issue with the layouts.
The unit is jumping when crossing a trailing 0 that is omitted.
All knobs are jumping in case a long label is replaced by a shorter value text.

Not sure if this is worth to fix.

The filter knobs can make use of a more complex logic for showing the significant numbers and a conditional kilo k. But that is also quite advanced, just an idea.

src/effects/backends/effectmanifestparameter.h

daschuer · 2022-11-07T19:59:34Z

src/effects/backends/lv2/lv2manifest.cpp

+                        // EffectManifestParameter::unitsHintToString
+                        unitsHint = EffectManifestParameter::lv2UnitToUnitsHint(
+                                lilv_node_as_string(customUnit));
+                        if (lv2ParamDebug &&


I think this one should be always enabled. If it happens on a user machine it will go to the mixxx.log and we can request this during bug hunting.

well, yes, but in my case with the LSP suite it prints hundreds of lines with G

we can leave it enabled until the release so we can easily look for missing units?

Since we know of the G case and have decided to use no unit for it. How about skip the message in this case. This also gives a chance to drop a comment about to our decision.

okay, for G it's a simple check. if the blacklist grows we need to rework this though.

src/widget/weffectparameternamebase.cpp

ronso0 · 2022-11-07T22:29:05Z

The unit is jumping when crossing a trailing 0 that is omitted.

not sure how to fix that. Using a fixed number of decimals wouldn't fix the position since the integer part of the value cn change, too, for example a Center1/2 of the param. EQ

All knobs are jumping in case a long label is replaced by a shorter value text.

Not sure if this is worth to fix.

nope, see above.
figuring out how to fix the pixel postion of either . or the unit is a PITA and not worth the effort IMO.
guessing the width from the range etc.
IMO the benefit of showing the value is to get the "final" value after stopping the adjustment for a second.

ronso0 · 2022-11-08T12:02:55Z

With the current unit blacklist I get notified only about

plugin "MaBitcrush" has custom unit "bits" for parameter "resolution"
plugin "LFO" has custom unit "(coef)" for parameter "Speed"
plugin "LFO" has custom unit "(coef)" for parameter "Multiplier"
plugin "LSP Delay Compensator Mono" has custom unit "°C" for parameter "Temperature"
plugin "LSP Delay Compensator Stereo" has custom unit "°C" for parameter "Temperature"
plugin "LSP Delay Compensator x2 Stereo" has custom unit "°C" for parameter "Temperature L"
plugin "LSP Delay Compensator x2 Stereo" has custom unit "°C" for parameter "Temperature R"
plugin "LSP Slapback Delay Mono" has custom unit "°C" for parameter "Tem"
plugin "LSP Slapback Delay Stereo" has custom unit "°C" for parameter "Tem"

ronso0 · 2022-11-08T13:38:06Z

A question about performance vs. easy maintenance:
are there other performant solutions to have all parameter unit enums & strings in effectmanifest.h, besides the if/else getter?
or something fast like a QHash?

ronso0 · 2022-11-08T17:31:11Z

@daschuer As soon the fixup look good to you I'll squash them.

daschuer · 2022-11-10T08:12:59Z

A QHash would be faster.

This topic has made me rethink the whole custom units topic:
For my understanding implementing it is just a few lines away and it will release us from some maintenance. Instead of storing an enum along with the pure unit, we can use always custom units by replacing it with the printf format string, the LV2 renderer.
Using QString::asprintf()

What do you think?

ronso0 · 2022-11-10T11:03:55Z

I'm not sure I understand. Where exactly you want to use the LV2 render method?
Besides, that would reintroduce mins instead of min http://lv2plug.in/ns/extensions/units#min
and that's just the 'official' units. How should we filter out undesired units?

daschuer · 2022-11-10T16:58:26Z

In case we decide to extend this for custom units, most of the current code and replacements can remain working. I will comment the places inline.

daschuer

I have added the comments.
Just as suggestion. This PR is also good without.

daschuer · 2022-11-10T17:01:33Z

src/effects/backends/effectmanifestparameter.h

+        }
+    }
+
+    static QString unitsHintToString(const UnitsHint unitsHint) {


This table can be changed to return the format string (or LB2 renderer).
Like "%f ms"

daschuer · 2022-11-10T17:06:58Z

src/widget/weffectparameternamebase.cpp

+    if (m_unitString.isEmpty()) {
+        setText(QString::number(dispVal));
+    } else {
+        setText(QString::number(dispVal) + QChar(' ') + m_unitString);


Suggested change

setText(QString::number(dispVal) + QChar(' ') + m_unitString);

setText(QString::asprintf(m_unitString, dispVal));

src/effects/backends/lv2/lv2manifest.cpp

ronso0 · 2022-11-10T22:31:13Z

ok, I understand your proposal and why you think it could be an improvement.

1 get rid of the lv2UnitToUnitsHint() conversion and store the format string set by the LV2 param manifest:
I'd agree if we could rely on that, though we can't: there's the min/mins exception and handling such cases requires more complexity IIUC

Using the lilv methods to get the LV2 format strings also from UnitsHint of built-in effects would work, too, of course, but IMO that's much more code (and runtime effort?) than simply returning a pre-set string. Regarding maintenance: the units list won't change very often. Tbh I don't expect anyone to ask for adding a custom unit since no one really cared much about the LV2 UI (looking at the forums requests and filed bugs). And if someone does, it's rather easy (if we annotate the methods well).
So IMO it makes more sense to keep the current implementation but improve the performance of the lv2UnitToUnitsHint() and unitsHintToString()

2 store the param unit (format) string as soon as an effect is registered, not in the widget when it's loaded to the parameter slot (load effect, or move or show parameter)
I agree, that makes sense. Even if it only accumulates the many potential singular (potentially repetetive) UnitsHint > string conversions happening during performance in the startup procedure. So it's more efficient, but the conversion is done for all available effects than just for those being actually loaded.
Probably notable only --if at all-- with many LV2 plugins
with many LV2 plugins

Re asprintf()
so instead of trimming/rounding the display value depending on its int part we'd need to adjust %f to %.nf with n is 0, 1 or 2

daschuer · 2022-11-10T22:58:27Z

Hey, it was just an idea for improvement. If you do not get hooked on it, we can keep the original mode of this. I don't even think that there is a performance bottle neck to solve.

Using "%.nf" will unfortunately introduce an compatibility issue with the LV2 units, because they do not have this. So It is probably better to adjust the value before formatting.

I do not see an issue with "mins" and "min", because we can use for all non custom units our internal format string.

ronso0 · 2022-11-10T23:28:21Z

hehe, I did get hooked on it, but you were referring only to custom units, while I assumed you want to use the LV2 units lib for built-in effects. Anyway...

Using "%.nf" will unfortunately introduce an compatibility issue with the LV2 units, because they do not have this. So It is probably better to adjust the value before formatting.

Yes, that's what I meant: adjust %f to %.0f, %.1f or %.2f as required by the value (and don't adjust the value). Because just %f will not work with adjusted values, for example double 6000.0 would be 6000.0000..(and probably a 1 somewhere down the line) , compared to 6000 Hz with QString::Number

requires checking for 'lv2' package if LILV cmake option is enabled

daschuer

LGTM and works good. Thank you.

ronso0 · 2023-01-09T09:07:46Z

🎉 thanks for your review and ideas!

github-actions bot added build ui labels Nov 5, 2022

ronso0 marked this pull request as draft November 5, 2022 14:44

ronso0 force-pushed the effect-parameter-units branch 3 times, most recently from 1d806e8 to 0d9a4fc Compare November 5, 2022 23:02

ronso0 force-pushed the effect-parameter-units branch 2 times, most recently from e30b12a to 99df207 Compare November 7, 2022 18:41

daschuer requested changes Nov 7, 2022

View reviewed changes

ronso0 linked an issue Nov 9, 2022 that may be closed by this pull request

Show units of effect parameter knobs #11033

Closed

ronso0 mentioned this pull request Nov 9, 2022

discretely valued effect parameter knobs #8756

Open

daschuer reviewed Nov 10, 2022

View reviewed changes

ronso0 force-pushed the effect-parameter-units branch from ab1c1cb to 7b6dcfa Compare November 12, 2022 13:23

ronso0 added 2 commits November 12, 2022 21:51

Effect parameters: adopt LV2 units

c97c551

LV2: read parameter units from effects manifest...

da6583e

requires checking for 'lv2' package if LILV cmake option is enabled

ronso0 added 2 commits November 12, 2022 21:51

Effect parameters: translate UnitHint to string

61b7cbb

WEffectParameterNameBase: show parameter unit if available

5f002a0

ronso0 force-pushed the effect-parameter-units branch from 7b6dcfa to 271dea9 Compare November 12, 2022 22:36

ronso0 added 4 commits November 12, 2022 23:50

WEffectParameterNameBase: adjust value decimals per range

0c6f770

Effects: add some units to built-in effects

d66bad2

LV2: read custom paramter units

5ce555b

LV2: remove unneeded QStrings and redundant property checks

450773e

ronso0 force-pushed the effect-parameter-units branch 2 times, most recently from 5280012 to dbcffab Compare November 17, 2022 21:07

ronso0 added 2 commits November 18, 2022 00:23

Effect parameter: QHash for unit strings lookup

057b1eb

LV2 parameter: QHash for UnitsHint lookup

702871a

ronso0 force-pushed the effect-parameter-units branch from dbcffab to 702871a Compare November 17, 2022 23:23

ronso0 mentioned this pull request Nov 18, 2022

Knob: hide cursor on wheel event for .8s #11077

Merged

ronso0 marked this pull request as ready for review November 22, 2022 20:42

ronso0 added this to the 2.4.0 milestone Dec 4, 2022

daschuer approved these changes Jan 9, 2023

View reviewed changes

daschuer merged commit a7f2901 into mixxxdj:main Jan 9, 2023

ronso0 deleted the effect-parameter-units branch January 9, 2023 09:07

ronso0 mentioned this pull request Sep 27, 2023

Show meaningful value for effect parameters #11040

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

show effect parameter units in parameter name label #11041

show effect parameter units in parameter name label #11041

ronso0 commented Nov 5, 2022 •

edited

Loading

ronso0 commented Nov 5, 2022

ronso0 commented Nov 5, 2022 •

edited

Loading

ronso0 commented Nov 7, 2022

JoergAtGithub commented Nov 7, 2022 •

edited

Loading

ronso0 commented Nov 7, 2022 •

edited

Loading

daschuer left a comment

daschuer Nov 7, 2022

ronso0 Nov 7, 2022

ronso0 Nov 7, 2022

daschuer Nov 8, 2022

ronso0 Nov 8, 2022

ronso0 commented Nov 7, 2022 •

edited

Loading

ronso0 commented Nov 8, 2022

ronso0 commented Nov 8, 2022

ronso0 commented Nov 8, 2022

daschuer commented Nov 10, 2022

ronso0 commented Nov 10, 2022

daschuer commented Nov 10, 2022

daschuer left a comment

daschuer Nov 10, 2022

daschuer Nov 10, 2022

ronso0 commented Nov 10, 2022

daschuer commented Nov 10, 2022

ronso0 commented Nov 10, 2022

daschuer left a comment

ronso0 commented Jan 9, 2023

	setText(QString::number(dispVal) + QChar(' ') + m_unitString);
	setText(QString::asprintf(m_unitString, dispVal));

show effect parameter units in parameter name label #11041

show effect parameter units in parameter name label #11041

Conversation

ronso0 commented Nov 5, 2022 • edited Loading

ronso0 commented Nov 5, 2022

ronso0 commented Nov 5, 2022 • edited Loading

ronso0 commented Nov 7, 2022

JoergAtGithub commented Nov 7, 2022 • edited Loading

ronso0 commented Nov 7, 2022 • edited Loading

daschuer left a comment

Choose a reason for hiding this comment

daschuer Nov 7, 2022

Choose a reason for hiding this comment

ronso0 Nov 7, 2022

Choose a reason for hiding this comment

ronso0 Nov 7, 2022

Choose a reason for hiding this comment

daschuer Nov 8, 2022

Choose a reason for hiding this comment

ronso0 Nov 8, 2022

Choose a reason for hiding this comment

ronso0 commented Nov 7, 2022 • edited Loading

ronso0 commented Nov 8, 2022

ronso0 commented Nov 8, 2022

ronso0 commented Nov 8, 2022

daschuer commented Nov 10, 2022

ronso0 commented Nov 10, 2022

daschuer commented Nov 10, 2022

daschuer left a comment

Choose a reason for hiding this comment

daschuer Nov 10, 2022

Choose a reason for hiding this comment

daschuer Nov 10, 2022

Choose a reason for hiding this comment

ronso0 commented Nov 10, 2022

daschuer commented Nov 10, 2022

ronso0 commented Nov 10, 2022

daschuer left a comment

Choose a reason for hiding this comment

ronso0 commented Jan 9, 2023

ronso0 commented Nov 5, 2022 •

edited

Loading

ronso0 commented Nov 5, 2022 •

edited

Loading

JoergAtGithub commented Nov 7, 2022 •

edited

Loading

ronso0 commented Nov 7, 2022 •

edited

Loading

ronso0 commented Nov 7, 2022 •

edited

Loading