Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Faders with a dB scale (instead of linear/percentage) #7636

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

michaelgregorius
Copy link
Contributor

@michaelgregorius michaelgregorius commented Dec 27, 2024

This pull request changes the faders to use a dB scale. They have the following behavior:

  • Clicking on the knob lets the users drag the knob to change the position. The position only changes on move.
  • Clicking on the fader next to the knob immediately moves the knob to that position.

The faders can be adjusted between -120 dB and +6 dB. Pulling a fader down further sets it to -inf dB, i.e. to zero amplification.

The faders scale is scaled by the cube function so that users have more space to work in the "interesting" area around 0 dB and less space in the area where we tend to -inf dB anyway. Fader ticks are painted as of commit 344fae2.

Below you can find a demo video of some of the features that will be described in the following.

Adjustment via mouse wheel or keyboard

The faders can be adjusted via scrolling of the mouse wheel or by pressing the up/plus or down/minus keys. Thebehavior with regards to modifier keys and adjustments is as follows:

  • No modifier key: adjusts in increments of 1 dB
  • Shift key: adjusts in increments of 3 dB
  • Control key: adjusts in increments of 0.1 dB

If the value goes below the minimum positive dB value that is allowed by the fader then the fader is set to "-inf dB", i.e. zero amplification. If the fader is set to "-inf dB" and the users want to increase the value then it is first set to the minimum positive value that is allowed by the fader.

Tool tips

A fader now show its current value as its tool tip. Please note that this value is always shown in dB because the goal is to get rid of the linear values for the faders (see below). Values are now shown with the unit "dB" instead of "dBFS" and as "-inf dB" instead of "-∞ dB". These changes will also be visible in the text float that is used to show the new values as the fader is being dragged.

Direct entry via dialog

The dialog that opens when the fader is double clicked now let users enter values in dB instead of a percentage value between 0 and 200.

The minimum value that can be entered is the minimum value that the fader allows to set, i.e. -120 dB. The maximum value is the maximum value of the model converted to dB. As of now this is ~6 dB. The current value is converted to dB. If it corresponds to "-inf dB", i.e. if the amplification is 0 then the minimum value is shown as the initial value. Although it might be considered to change this to 0 dB to be able to quickly activate a channel using the dialog.

Remove "Display volume as dBFS" and "Volume as dBFS"

Remove the option "Display volume as dBFS" from the settings dialog and the check box option "Volume as dBFS" from the "View" menu. Volumes are now always treated as dB.

Models that are already in dB

Support for models that are already in dB is preserved. This includes the following effects: Crossover Equalizer, Equalizer, Compressor and Delay. They also support the mouse wheel behavior as described above now.

Demo video

The following video demonstrates some of the aspects that have been described above:

Demo.-.Faders.with.dB.scale.webm

Proposal for integration into 1.3

I propose to integrate the changes into 1.3 as it will make LMMS more similar to other DAWs with regards to that rather fundamental feature.

Edit: adjustments with regards to the new commits

Make the faders use a linear dbFS scale. They now also change position on first click and can then be dragged.

The `Fader` class now has a new property called `m_faderMinDb`. It's the minimum value before the amplification drops down fully to 0. It is needed because we cannot represent a scale from -inf to maxDB.

Rename `knobPosY` to `calculateKnobPosYFromModel` and move the implementation into the cpp file. The method now first converts the model's current amplification value to dbFS and then computes a ratio based on that values and the minimum and maximum dbFS values.

Add the method `setVolumeByLocalPixelValue` which takes a local y coordinate of the widget and sets the amplification of the model based on a dbFS scale.

Adjust `mousePressEvent` and `mouseMoveEvent` so that they mostly take the local y coordinate of the event and use that to adjust the model via `setVolumeByLocalPixelValue`.

Remove `m_moveStartPoint` and `m_startValue` and they are not needed anymore.
Apply a curve, i.e. the cube function and its inverse, to the fader
positions to that we have more space to work with in the "interesting"
areas around 0 dB and less space in the area where we tend to "-inf dB".

Set the minimum dB value of the fader to -120 dB to increase the potential
headroom.
Add support for models that are in dB.

There's a new member `m_modelIsLinear` which can be set via the
constructor. The default value in the constructor is `true`.

Add the method `modelIsLinear` which can be used to query the parameter.
It is used in `calculateKnobPosYFromModel` and
`setVolumeByLocalPixelValue`. Both methods got extended to deal with
models in dB. They were also refactored to extract code that is common
for both cases.

Ensure that the constructor of `Fader` is called with `false` for
`CrossoverEQControlDialog` and `EqFader` because these use models that
are in dB.
Show the current value of the fader in its tool tip. Please note that
this value is always shown in dB because the goal should be to get rid
of the linear values for the faders.

Some of the code is extracted into the new method
`Fader::getModelValueAsDbString` which is shared by
`Fader::modelValueChanged` (also new) and `Fader::updateTextFloat`.

Please note that `getModelValueAsDbString` will use "dB" instead of
"dBFS" as the unit and that it will format "-inf dB" instead of "-∞ dB".
These changes will also be visible in the text float that is used to
show the new values as the fader is being dragged.
Let users enter values in dB in the dialog that opens with a double
click.

The minimum value that can be entered is the minimum value that the
fader allows to set, i.e. -120 dB. The maximum value is the maximum
value of the model converted to dB. As of now this is ~6 dB. The
current value is converted to dB. If it corresponds to "-inf dB", i.e.
if the amplification is 0 then the minimum value is used.
Remove the option "Display volume as dBFS" from the settings dialog and
the check box option "Volume as dBFS" from the "View" menu. Volumes are
now always treated as dB, i.e. all evaluations of the property
"displaydbfs" have been removed which results in assuming that this
option is always true.

The upgrade code in `ConfigManager::upgrade_1_1_91` was left as is.
However, a note was added which informs the reader that the value of
"displaydbfs" is not evaluated anymore.
Extend `Fader::wheelEvent` for the case where the model is not a dB
model (`modelIsLinear() == true`), e.g. for the mixer faders. In that
case it works as follows. Each step increments or decrements by 1 dB if
no modifier is pressed. With "Shift" pressed the increment value is 3 dB.
With "STRG" ("CTRL") pressed the increment value is reduced to 0.1 dB. If
the value goes below the minimum positive dB value that is allowed by the
fader then the fader is set to "-inf dB", i.e. zero amplification. If the
fader is set to "-inf dB" and the users want to increase the value then
it is first set to the minimum positive value that is allowed by the
fader.

If the model is a dB model then the same behavior as before is used.
Although it should be considered to adjust this case as well. These
models are used by the faders of the Crossover Equalizer, Equalizer,
Compressor and Delay and they are not very usable with the mouse wheel.
Make the faders of the Crossover Equalizer, Equalizer, Compressor and
Delay behave like the mixer faders, i.e. step in sizes of 3 dB, 1dB and
0.1 dB depending on whether a modifier key is pressed or not.

Extract some common code to do so and add some `const` keywords.
Implement a more stable knob behavior.

Remove the jumping behavior if the users directly click on a volume
knob. By storing the offset from the knob center and taking it into
account during the changes it now also feels like the users are
dragging the knob.

Changes to the amplification are now only applied when the mouse is
moved. This makes the double click behavior much more stable, i.e. if
users click on the knob when it is at 0 dB the dialog will also show
0 dB and not something like 0.3 dB because the first click is already
registered as a change of volume.

If the users click next to the knob the amplification will still be
changed immediately to that value.

## Technical details

To make the knobs more stable a variable called `m_knobCenterOffset` was
introduced. It stores the offset of the click from the knob center so
that this value can be taken into account for in the method
`setVolumeByLocalPixelValue`.
Add an indication that a float value is assigned to a float variable.
@michaelgregorius
Copy link
Contributor Author

Here's a screenshot which demonstrates/shows the cubic scaling of the dB scale by rendering some fader tick marks. The topmost mark represents 6 dB and then each tick decreases by 6 dB until -120 dB are reached. For example the master fader is at 0 dB in the screenshot while the kick fader is close to -6 dB. Some other faders even slightly amplify the signal:

7636-FadersWithTickMarks

Please note that due to the implementation the 0 dB fader tick and the 0 dB mark of the level meters are independent of each other and therefore not aligned.

If wanted I can commit these changes to this PR.

Introduce the `constexpr c_dBScalingExponent` which describes the
scaling exponent that's used to scale the dB scale in both directions.
This will simplify potential adjustments by keeping the values
consistent everywhere.
Draw fader ticks in case the model is a linear one. This means that for
now they should only be painted for the mixer faders but not for the
faders of the Compressor, Delay, etc.

Extract the computation of the scaled ratio between the maximum model dB
value and the minimum supported fader dB value into the new private
method `computeScaledRatio`. This is necessary because it is needed to
paint the fader knob at the correct position (using the knob bottom as
the reference) and to paint the fader ticks at the correct position
(using the knob center).

Introduce the private method `paintFaderTicks` which paints the fader
ticks.

Note: to paint some non-evenly spaced fader ticks replace the `for`
expression in `paintFaderTicks` with something like the following:
```
for (auto & i : {6.f, 0.f, -6.f, -12.f, -24.f, -36.f, -48.f, -60.f, -72.f, -84.f, -96.f, -108.f, -120.f})
```
Allow the adjustment of the faders via the keyboard. Using the up or
plus key will increment the fader value whereas the down or minus key
will decrement it. The same key modifiers as for the wheel event apply:
* No modifier: adjust by 1 dB
* Shift: adjust by 3 dB
* Control: adjust by 0.1 dB

Due to the very similar behavior of the mouse wheel and key press
handling some common functionality was factored out:
* Determinination of the (absolute) adjustment delta value by
  insprecting the modifier keys of an event. Factored into
  `determineAdjustmentDelta`.
* Adjustment of the model by a given dB delta value. Factored into
  `adjustModelByDBDelta`.
@michaelgregorius
Copy link
Contributor Author

Fader ticks have been committed with commit 344fae2.

Starting with commit aa74df2 it's now also possible to move the faders using the keyboard. The up or plus key will increment whereas the down or minus key will decrement. The same modifier keys as for the mouse wheel event apply.

The scaling exponent was extracted into a constexpr in commit 795c681. This allows for consistent changes and experimenting. The following images show linear scaling, quadratic scaling and cubic scaling:
7636-FaderScale17636-FaderScale27636-FaderScale3

@michaelgregorius
Copy link
Contributor Author

@sakertooth, requesting a review from you because you also did some work on the mixer and are active here. If there are more suiting people to review this please feel free to add them.

@regulus79
Copy link
Contributor

I did some testing of this PR.

  • Scrolling while holding alt always moves the fader down, even when scrolling up.

  • I noticed that rightclicking on the faders still shows percentages.
    image

  • When controlling the fader with +/- keys, it works fine on the keypad, but when using the +/- keys on the normal keyboard, you have to hold shift while pressing the + key (that makes sense, since it's normally the = key). But I noticed that when doing that, the fader also moves 3x as fast. I suppose isn't really important, since most people will probably use the keypad.

  • I also noticed that controlling the Mixer faders with the up/down keys only changes the last modified fader, not necessarily the currently selected channel. I guess that's fine, but since you can change the currently selected channel using the left/right arrow keys, it feels like maybe the up/down arrow keys should control the selected channel?

  • Also, just personally, I feel like it would be nice if the faders had more room for amplification. Right now 0db is quite high up on the fader, and it doesn't give much space to make the channel louder.

Move the fader of the selected channel instead of the fader that has
focus when the up/plus or down/minus keys are pressed. Doing so also
feels more natural because users can already change the selected
channel via the left and right keys and now they can immediately adjust
the volume of the currently selected channel while doing so.

Key events are now handled in `MixerView::keyPressEvent` instead of
`Fader::keyPressEvent` and the latter is removed. `MixerChannelView`
now has a method called `fader` which provides the associated fader.
This is needed so that the event handler of `MixerView` can act upon
the currently selected fader.

## Changes in Fader
The `Fader` class provides two new public methods.

The `adjust` method takes the modifier key(s) and the adjustment
direction and then decides internally how the modifier keys are mapped
to increment values. This is done to keep the mapping between modifier
keys and increment values consistent across different clients, e.g. the
key event of the `MixerView` and the wheel event of the `Fader` itself.
The direction is provided by the client because the means to determine
the direction can differ between clients and cases, e.g. a wheel event
determines the direction differently than a key event does.

The method `adjustByDecibelDelta` simply adjusts the fader by the given
delta amount. It currently is not really used in a public way but it
still makes sense to provide this functionality in case a parent class
or client wants to manipulate the faders by its very own logic.

Because the `Fader` class does not react itself to key press events
anymore the call to `setFocusPolicy` is removed again.
Let the users enter the fader value via dialog when the space key is
pressed.

Extract the dialog logic into the new method `adjustByDialog` and call
it from `MixerView::keyPressEvent`.

Make `Fader::mouseDoubleClickEvent` delegate to `adjustByDialog` and
also fix the behavior by accepting the event.
@michaelgregorius
Copy link
Contributor Author

* I also noticed that controlling the Mixer faders with the up/down keys only changes the last modified fader, not necessarily the currently selected channel. I guess that's fine, but since you can change the currently selected channel using the left/right arrow keys, it feels like maybe the up/down arrow keys should control the selected channel?

Thanks for testing @regulus79!

You are right, having a concept of focus and a selected channel made it feel disconnected. I have adjusted this with commit 4a720cb. Using the keys now adjusts the fader of the currently selected channel instead of one with focus.

Commit d451531 enables users to directly enter a value via the dialog if the space key is pressed with a currently selected channel.

This should give users much flexibility to adjust the mixer faders using only the keyboard:

  • Left/right: select channel
  • Up/down (or plus/minus): Adjust fader of current channel
  • Space: Enter value

@michaelgregorius
Copy link
Contributor Author

Mouse wheel and the "Alt" key

* Scrolling while holding alt always moves the fader down, even when scrolling up.

I cannot reproduce the problem and it might be something related to the system. If I remember correctly Qt has some strange behavior with regards to the mouse wheel and the Alt key.

However, the Alt key is not really evaluated anyway when handling the fader changes. It only supports Shift, Control and no modifier keys. So the fix here is: "Don't press the Alt key because it seems to invoke strange Qt behavior on some systems." 😉

Percentages in the model

* I noticed that rightclicking on the faders still shows percentages.

That's because the underlying model still uses the linear scale, i.e. it is not a model of decibel values. Introducing such a model would be quite some work because you'd somehow have to support the special case of -inf dB. This is why it is outside of the scope of this PR. The goal of this PR is to introduce lots of improvements while going into the right direction.

It's something that can be tackled once this PR is merged. However, we should not hold this PR until it is perfect and every "little" thing is implemented because "perfect is the enemy of the good".

Keyboard layout issues

* When controlling the fader with +/- keys, it works fine on the keypad, but when using the +/- keys on the normal keyboard, you have to hold shift while pressing the + key (that makes sense, since it's normally the = key). But I noticed that when doing that, the fader also moves 3x as fast. I suppose isn't really important, since most people will probably use the keypad.

I guess this is a problem with the keyboard layout. Sometimes I also have this problem with my German keyboard layout for which I have to press modifier keys for keys that are accessible without any modifier keys on a US layout. So the solution here is to advise users to use the "free-standing" keys like "+" and "-" on the numpad/keypad and the up and down arrows.

Fader headroom

* Also, just personally, I feel like it would be nice if the faders had more room for amplification. Right now 0db is quite high up on the fader, and it doesn't give much space to make the channel louder.

Do you mean that the fader should let users amplify the channel by more than 6 dB or do you mean that the graphical ratios between 6 dB, 0 dB and -inf db should be distributed differently?

In case you mean the latter: There is already quite some mapping going on and therefore I'd prefer not to add some additional mapping so that the positive dB scale gets some additional space.

IMO the new implementation of this PR is already quite some improvement because the old implementation gave you half of the space to make adjustments between the finite space of 0 dB to 6 dB and the other half to make an adjustment in the infinite space of 0 dB and -inf dB. Put differently: it was much too detailed in the space between "change nothing" and "double the volume".

Please also note that it is rather "normal" to pull most faders below unity anyway so that you can mix with some headroom. If you need to make large adjustments you can always insert a plugin which is called "Utility" or "Tool" in other DAWs which lets you adjust the volume with a gain larger than 6 dB. I guess in LMMS this would be the "Amplifier" plugin although even that plugin only allows to add measly 6 dB.

Using a "Utility"/"Tool" plugin is also recommended if you want to automate volume levels because if you directly automate the fader itself you might for example get problems if you adjust other volumes.

Last but not least users can use the modifier keys for fine adjustment or just enter the positive values via dialog.

@Rossmaxx Rossmaxx added this to the 1.3 milestone Jan 18, 2025
@bratpeki
Copy link
Member

Volumes are now always treated as dB.

Awesome! I'll be testing this when I get the current PR tests off my plate.

@bratpeki bratpeki self-assigned this Jan 27, 2025
@bratpeki
Copy link
Member

Glad to be working on this! I get my builds off the LMMS site.

I looked at everything Regulus mentioned.

Scrolling while holding alt always moves the fader down, even when scrolling up.

Can confirm! Happens on the Compressor, Equalizer, Delay and Compression EQ as well. I'm guessing this is implemented directly on the "Fader" class, or however it's implemented in the code! No particular idea how this should be fixed, maybe just make alt-scrolling behave like no modifier was applied? Maybe 0.01dB for some hyper-precision? If 0.01 is a genuine step applicable to the LMMS fader, that might be best, since it's another choice for the user :)

I noticed that rightclicking on the faders still shows percentages.

image

Seems to be fine now. Checked with the Delay plugin.

When controlling the fader with +/- keys...

The +/- keystrokes needed to change the volume are keyboard layout dependent, it seems. I tried it with two layouts, it uses the appropriate layout for both. So it seems fine to me, especially with how it's localized to the user, since this would exclusively be for users who don't have either a mouse or a numpad. However, if we can get the "keypress ID", we could make it universal!

@bratpeki
Copy link
Member

it feels like maybe the up/down arrow keys should control the selected channel

Seems to have been patched (commit 4a720cb).

Also, just personally, I feel like it would be nice if the faders had more room for amplification. Right now 0db is quite high up on the fader, and it doesn't give much space to make the channel louder.

Given how the mixer is now resizable, I wouldn't think of this as a big issue. Maybe if users complain, but since you can fullscreen your mixer, it might be enough room.

@michaelgregorius FWICT, the distance between each of these is half of the previous distance, so 6dB-ish? Or is it exactly 6dB?

If it's something specific like that, we would need a different method of scaling. But a different method of scaling might be unconventiontional. IDK, there is discussion to be had there.

@sakertooth
Copy link
Contributor

@sakertooth, requesting a review from you because you also did some work on the mixer and are active here. If there are more suiting people to review this please feel free to add them.

Apologies, I'm taking a look right now.

@sakertooth
Copy link
Contributor

So, I have a couple concerns/questions.

  • When moving the fader knob normally with the mouse, it still changes by some fractional amount like it does in master. Is this expected? How is the step size in dB determined? Is it fixed?
  • Scrolling with the mouse wheel does not move the fader at all. Neither does using the up or down arrow keys.
  • Clicking the mouse wheel resets the fader to 0 dB. I think we should explain this somewhere in the PR description (unless I missed this detail).
  • Clicking and holding the mouse wheel while moving the mouse moves the fader, but I am not sure by how much.
  • As you resize the Mixer, the ticks and the 0 dB peak reference become more and more unaligned. I think we should address this in this PR.
  • Curious why we have fader ticks just for the Mixer and not for the faders in other places like the Equalizer plugin?
  • There is still some inconsistently involving use of "dBFS", "dB", "dBv", etc.
  • When right clicking the fader, it still shows the fader value in percentages. For the faders in the plugins, right clicking them shows it in dB (or some variant of dB like dBFS or dBv).
  • Any reason why we cant specify the maximum value at exactly 6dB but ~6dB? Having it exactly at 6dB feels more "correct".
  • Should we make the fader behavior more uniform with other things, like volume knobs for tracks?

Will do a code review some time later. If you need any videos/pictures, let me know.

Fix the handling of wheel events without any modifier key being pressed.

Commit ff435d5 accidentally tested against Alt using a logical OR
instead of an AND.
@michaelgregorius
Copy link
Contributor Author

Thanks for checking @sakertooth!

So, I have a couple concerns/questions.

* When moving the fader knob normally with the mouse, it still changes by some fractional amount like it does in master. Is this expected? How is the step size in dB determined? Is it fixed?

If you move the fader with the left mouse button then the amplification is calculated by taking the fader position into account. That's the reason why you get fractional values and IMO it is expected. Pixel positions are integer values and if you map these to [-120 dB, 6 dB] then this will yield fractional values.

* Scrolling with the mouse wheel does not move the fader at all. Neither does using the up or down arrow keys.

Oops, good catch! That's a bug that was introduced with commit ff435d5 and is fixed with 5a66348.

* Clicking the mouse wheel resets the fader to 0 dB. I think we should explain this somewhere in the PR description (unless I missed this detail).

This behavior is caused by the call to AutomatableModelView::mousePressEvent(mouseEvent) in the else of Fader::mousePressEvent. It existed before and was not introduced by this PR. It's a good idea to document it though as it seems to have surprised you. 😉

* Clicking and holding the mouse wheel while moving the mouse moves the fader, but I am not sure by how much.

Can you elaborate on this one?

* As you resize the Mixer, the ticks and the 0 dB peak reference become more and more unaligned. I think we should address this in this PR.

This is explained at the end of this comment: #7636 (comment)

It would be quite some effort to adjust everything in such a way that the fader and the meters always match perfectly. Currently they are two different objects with two different tasks, i.e. adjusting the volume and rendering the current volume. Please note that they do not align in master as well where the logical 0 dB position is in the middle of the fader. So this is out of scope of this PR as far as I am concerned.

* Curious why we have fader ticks just for the Mixer and not for the faders in other places like the Equalizer plugin?

The other plugins use a dB model instead of an amplification model and present it in a linear way due to the rather small intervals like for example [-18 dB, 18 dB].

* There is still some inconsistently involving use of "dBFS", "dB", "dBv", etc.

There's an issue for that and this is out of scope of this PR because it applies to the whole application.

* When right clicking the fader, it still shows the fader value in percentages. For the faders in the plugins, right clicking them shows it in dB (or some variant of dB like dBFS or dBv).

This is explained here: #7636 (comment) (see the section "Percentages in the model").

* Any reason why we cant specify the maximum value at exactly 6dB but ~6dB? Having it exactly at 6dB feels more "correct".

The reason is that the amplification model is defined in the range [0, 2], i.e. no sound and twice the amplification. And mathematically 20*log(2) ~ 6,02059991328.

It's also needed to keep backwards compatibility with projects that have some of the faders set to maximum, i.e. to 2 in the amplification model.

* Should we make the fader behavior more uniform with other things, like volume knobs for tracks?

From a logical point of view the volume knobs of the tracks should present themselves in a very similar way if you set them to logarithmic. Due to their implementation they do not let you do a fine-grained adjustment like the faders do in this PR. Setting the knobs to logarithmic they do a rather big jump from -inf to around -35 dB if I move the mouse.

Fixing the volume knobs is not part of this PR though.

Will do a code review some time later. If you need any videos/pictures, let me know.

Thanks!

This PR is intended to move LMMS into the right direction with regards to using a decibel scale for the faders. It's not in the scope of this PR to fix all problems at once. To me it is more important that things move in the right direction because other things can be adjusted from there on. So the motto is "perfect is the enemy of the good".

@sakertooth
Copy link
Contributor

sakertooth commented Feb 2, 2025

Clicking and holding the mouse wheel while moving the mouse moves the fader, but I am not sure by how much. Can you elaborate on this one?

When you click and hold the mouse wheel over a fader and move the mouse up or down, it moves the fader, but I wasn't sure how much. I thought this was new behavior, but this also seems to be in master as well, and it looks like it just moves the fader normally. Nevermind.

It would be quite some effort to adjust everything in such a way that the fader and the meters always match perfectly. Currently they are two different objects with two different tasks, i.e. adjusting the volume and rendering the current volume. Please note that they do not align in master as well where the logical 0 dB position is in the middle of the fader. So this is out of scope of this PR as far as I am concerned.

If that is out of scope, then IMO fader ticks are out of scope because that is closely related to rendering the volume, not adjusting the volume to work logarithmically instead of linearly. I think we should not add fader ticks until they are properly implemented to provide accurate readings of the volume being rendered.

Why do we have to add them now? What good are the ticks if they are not accurate?

The reason is that the amplification model is defined in the range [0, 2], i.e. no sound and twice the amplification. And mathematically 20*log(2) ~ 6,02059991328. It's also needed to keep backwards compatibility with projects that have some of the faders set to maximum, i.e. to 2 in the amplification model.

That's fair. I don't have a huge problem with that. Though one could downplay the backward compatibility issue since the difference is practically imperceptible.

Edit: Sidebar, but I was thinking about this a bit more, and I suppose its more preferable to use the raw amplification multiplier model instead of the dB model for performance (the latter could potentially require costly mathematical conversions for no real benefit).

The other plugins use a dB model instead of an amplification model and present it in a linear way due to the rather small intervals like for example [-18 dB, 18 dB].

We might consider changing these intervals to better reflect the expectation coming from other DAWs. Doesn't have to be done here though, just something to ponder.

This PR is intended to move LMMS into the right direction with regards to using a decibel scale for the faders. It's not in the scope of this PR to fix all problems at once. To me it is more important that things move in the right direction because other things can be adjusted from there on. So the motto is "perfect is the enemy of the good".

I agree, we shouldn't address all the issues here, but I think introducing a feature that isn't implemented in a state that can be improved immediately but instead has to be fundamentally fixed along the way isn't the right way to go. This is just my perspective. I share the same sentiment about things like audio recording for example.

@michaelgregorius
Copy link
Contributor Author

It would be quite some effort to adjust everything in such a way that the fader and the meters always match perfectly. Currently they are two different objects with two different tasks, i.e. adjusting the volume and rendering the current volume. Please note that they do not align in master as well where the logical 0 dB position is in the middle of the fader. So this is out of scope of this PR as far as I am concerned.

If that is out of scope, then IMO fader ticks are out of scope because that is closely related to rendering the volume, not adjusting the volume to work logarithmically instead of linearly. I think we should not add fader ticks until they are properly implemented to provide accurate readings of the volume being rendered.

Why do we have to add them now? What good are the ticks if they are not accurate?

Proposal to make rendering of ticks a preference

It should be easy to comment out the tick rendering call. In that case I'd like to leave the code of the actual rendering method though in case someone wants to work on that feature later.

Another option would be to provide a new preference that lets users switch the ticks rendering on and off. The default could be to turn it off. I'd prefer that option because it would simply let the individual users choose if it's helpful for them.

What do you think?

Faders and meters

I'd like to stress that the fader ticks are accurate though. It's important that one has to distinguish between two elements that are currently mashed into a single Fader class:

  • The knob that can be used to adjust the amplification. I will call this thing "fader" in the following because the knob with the track it moves in actually is the fader.
  • The meters which are used to render the current volume levels. Meters are not faders.

Both of them have ticks in the current implementation of this PR. The meter only has one tick at the 0 dB level which is rendered in the middle of the meters. The fader has several ticks in steps of 6 dB levels which indicate the level of change and the cubic mapping (see below). They are rendered to the left and right of the fader.

Both elements also have two different implementations and specifications (as implied by the code):

  • Fader: Provides adjustments in the interval [-120 dB, 6 dB] if it is not set to -inf dB. The dB values of that interval are mapped via a cubic function, i.e. x^3, so that users have more leeway in the more "interesting" and "louder" areas. You can see the effect of that mapping in the ticks. The middle of the fader knob graphics is taken as the reference of where the fader currently is. This means that in sum one knob height cannot be used for the actual setting of the values, i.e. one half knob at the top and a half knob at the bottom.
  • Meter: It currently renders the levels in the interval [-42 dB, 9 dB]. It uses a linear dB scale, i.e. not a cubic one, and makes use of the full widget height, i.e. it is not restrained by the knob height.

Consequences of merging them even more

Merging both elements into one would also mean that there would be less vertical room for the meters, i.e. they would be rendered smaller, because they would "inherit" the knob height constraint of the faders. It would also mean that the meters cannot show values that exceed 6 dB because that's the maximum of the model which in turn is tied to the fader knob implementation.

Separation of concerns

There are also DAWs which separate these two elements by putting the fader next to the meters. Doing so makes sense IMO because they are concerned with two completely different tasks. In the long-term it might even make sense to separate them into two classes as it might for example be nice to simply draw some meters at the inputs and outputs of plugins, etc. without rendering the knobs.

The reason is that the amplification model is defined in the range [0, 2], i.e. no sound and twice the amplification. And mathematically 20*log(2) ~ 6,02059991328. It's also needed to keep backwards compatibility with projects that have some of the faders set to maximum, i.e. to 2 in the amplification model.

That's fair. I don't have a huge problem with that. Though one could downplay the backward compatibility issue since the difference is practically imperceptible.

Edit: Sidebar, but I was thinking about this a bit more, and I suppose its more preferable to use the raw amplification multiplier model instead of the dB model for performance (the latter could potentially require costly mathematical conversions for no real benefit).

IMO the dB model should be used. It definitively has a benefit because it better describes how humans, i.e. our users, perceive loudness. Therefore it would also be better if the automations would use a dB model. If users want to decrease the volume in a way that is perceived as linear then they would simply draw a line in the dB model. With the current amplification model they must draw a curve to achieve the same effect, i.e. they must approximate the logarithmic effect.

The above also illustrates why it will be hard to fully switch to a dB model and why it is out of scope of this PR. To do so the automations of older project files will have to be mapped to the dB model and this involves non-linear mappings. An alternative would be to support both types of models for backwards compatibility. IMO it might be easier to replace the linear model with the dB model and do this in a release that contains or implies breaking changes.

With a good implementation the performance impacts due to the dB to amplification conversion should be negligible because other DAWs also present the automations as described above and therefore will have to do these conversions a lot of times.

The other plugins use a dB model instead of an amplification model and present it in a linear way due to the rather small intervals like for example [-18 dB, 18 dB].

We might consider changing these intervals to better reflect the expectation coming from other DAWs. Doesn't have to be done here though, just something to ponder.

👍

This PR is intended to move LMMS into the right direction with regards to using a decibel scale for the faders. It's not in the scope of this PR to fix all problems at once. To me it is more important that things move in the right direction because other things can be adjusted from there on. So the motto is "perfect is the enemy of the good".

I agree, we shouldn't address all the issues here, but I think introducing a feature that isn't implemented in a state that can be improved immediately but instead has to be fundamentally fixed along the way isn't the right way to go. This is just my perspective. I share the same sentiment about things like audio recording for example.

There is almost always room for improvement. That's what I mean with "perfect is the enemy of the good" and why it is important to know the direction that the project wants to go so that one can go into that direction with several manageable steps which all bring successive improvements.

As shown above there is no simple and obvious solution due to all the complexities and intricacies.

Making the rendering of the fader ticks configurable would be a good compromise IMO.

@sakertooth
Copy link
Contributor

Merging both elements into one would also mean that there would be less vertical room for the meters, i.e. they would be rendered smaller, because they would "inherit" the knob height constraint of the faders. It would also mean that the meters cannot show values that exceed 6 dB because that's the maximum of the model which in turn is tied to the fader knob implementation.

There are also DAWs which separate these two elements by putting the fader next to the meters. Doing so makes sense IMO because they are concerned with two completely different tasks. In the long-term it might even make sense to separate them into two classes as it might for example be nice to simply draw some meters at the inputs and outputs of plugins, etc. without rendering the knobs.

Sorry, I was indeed merging these ideas together and misunderstanding things. If I understand correctly, the faders are only saying "I am making the signal louder or quieter by x dB" and the meter is saying "The volume level of the signal is x dB compared to the 0 dB reference).

I've also checked with other software like PulseAudio Volume Control and indeed they keep the faders and the meter separate.

The above also illustrates why it will be hard to fully switch to a dB model and why it is out of scope of this PR. To do so the automations of older project files will have to be mapped to the dB model and this involves non-linear mappings. An alternative would be to support both types of models for backwards compatibility. IMO it might be easier to replace the linear model with the dB model and do this in a release that contains or implies breaking changes.

Since we seem to be already making such mappings internally anyways, I do think we can get away with any potential inaccuracies involved in the conversion. However, I do agree that it is probably out of scope to do here.

With a good implementation the performance impacts due to the dB to amplification conversion should be negligible because other DAWs also present the automations as described above and therefore will have to do these conversions a lot of times.

We seemed to have already optimized these conversions, so I guess it should be negligible.

IMO the dB model should be used. It definitively has a benefit because it better describes how humans, i.e. our users, perceive loudness. Therefore it would also be better if the automations would use a dB model. If users want to decrease the volume in a way that is perceived as linear then they would simply draw a line in the dB model. With the current amplification model they must draw a curve to achieve the same effect, i.e. they must approximate the logarithmic effect.

I failed to consider this situation. The automation should infact be drawn linearly, so we would need a logarithmic model to do that.

Making the rendering of the fader ticks configurable would be a good compromise IMO.

We can wait to see if people want it to be configurable, because otherwise we would be adding a new option that is most commonly always on by the users, which wouldn't be that useful. Me personally, I am fine with the fader ticks, and I assume most DAWs always have them displayed.

Copy link
Contributor

@sakertooth sakertooth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good for the most part. Hopefully we can get this merged soon enough.

include/Fader.h Outdated Show resolved Hide resolved
include/Fader.h Outdated Show resolved Hide resolved
include/Fader.h Outdated Show resolved Hide resolved
src/core/ConfigManager.cpp Outdated Show resolved Hide resolved
src/gui/MainWindow.cpp Outdated Show resolved Hide resolved
src/gui/MainWindow.cpp Outdated Show resolved Hide resolved
src/gui/widgets/Fader.cpp Outdated Show resolved Hide resolved
@bratpeki
Copy link
Member

bratpeki commented Feb 3, 2025

Got one PR off my plate, so consider that this now has my undivided attention, @michaelgregorius! Writing more soon.

@michaelgregorius
Copy link
Contributor Author

@sakertooth, @bratpeki, thanks for checking and for your input so far. I don't have much time during the week so I hope that can come back to this next weekend.

@bratpeki
Copy link
Member

bratpeki commented Feb 4, 2025

@sakertooth:
There is still some inconsistently involving use of "dBFS", "dB", "dBv", etc.

I can see this in the mixer faders (dB) and in the track volume knobs and Amplifier plugin volume knob (dBFS).


I'll try making the 0dB mark just a bit bolder and see how it looks (8f0d877)! :)

@bratpeki
Copy link
Member

bratpeki commented Feb 5, 2025

const QPen zeroPen(QColor(255, 255, 255, 216), 2.5);

image
image

Gotta love magic numbers! 😎

@bratpeki
Copy link
Member

bratpeki commented Feb 5, 2025

Anyway, a slightly larger indicator is in order, I would think, simply since it makes the UI a bit more "forward", 1.5 is still a bit tiny to me.

Open to suggestions, though, the line is somewhere below line 700 at src/gui/widgets/Fader.cpp!

@sakertooth
Copy link
Contributor

I have some new thoughts I wanted to discuss:

If you move the fader with the left mouse button then the amplification is calculated by taking the fader position into account. That's the reason why you get fractional values and IMO it is expected. Pixel positions are integer values and if you map these to [-120 dB, 6 dB] then this will yield fractional values.

This implies that you can get more accurate fader changes without having to be so precise with the mouse by simply making the Mixer bigger. This seems to be incorrect behavior. FL for example has the behavior such that changes in the fader using the mouse change the volume in steps of 0.1 dB.

IMO the dB model should be used. It definitively has a benefit because it better describes how humans, i.e. our users, perceive loudness. Therefore it would also be better if the automations would use a dB model. If users want to decrease the volume in a way that is perceived as linear then they would simply draw a line in the dB model. With the current amplification model they must draw a curve to achieve the same effect, i.e. they must approximate the logarithmic effect.

I'm changing my mind about this. I think internally, using the linear scale [0, 200] was the right choice as handling the -inf dB case is a lot simpler, just as an example. Even other DAWs like FL seem to use the linear scale internally. Its just that they primarily show the volume in dB, while also ensuring that the fader changes the model value by an amount in dB and not by a linear one. Automations wouldn't be a problem here because fader changes should apply a change in dB, even if internally they use a linear scale. The model system in LMMS is horrifically broken, but I think this is how it should ideally work.

Another option would be to provide a new preference that lets users switch the ticks rendering on and off. The default could be to turn it off. I'd prefer that option because it would simply let the individual users choose if it's helpful for them.

IMO, if we want the ticks, I think they should be a bit more prominent while also having a better visual indication of what the current level the fader is at in dB (bigger, number markers, etc). Because unfortunately, I think that the tooltip does a much better job of conveying that information better than the ticks currently, so the ticks feel like unnecessary fashion.

First set of code review changes:
* Use Doxygen style documentation comments
* Remove comment about `displaydbfs` from upgrade routine
* White-space changes for touched lines.
Make the minimum dB value a constexpr in the implementation file because
currently it's an implementation detail that should not be of any
interest to any other client.

So `m_faderMinDb` becomes `c_faderMinDb`.
Paint the fader ticks in a more systematic and flexible way. This also
removes some "magic numbers", e.g. by using `c_faderMinDb` instead of
`-120.f` as the lower limit.

The upper limit, i.e. the "starting point" is now also computed using
the maximum value of the model so that the fader will still paint
correctly if it ever changes.
Make the zero indicator tick of the fader bolder.
@michaelgregorius
Copy link
Contributor Author

michaelgregorius commented Feb 8, 2025

const QPen zeroPen(QColor(255, 255, 255, 216), 2.5);

[...]
Gotta love magic numbers! 😎

Adjusted with commit 41598a9, @bratpeki.

Make rendering of fader ticks a preference which is off by default.

Introduce the new option "Show fader ticks" to the setup dialog and save
it to the UI attribute `showfaderticks`.

The configuration value is currently evaluated in `Fader::paintEvent`.
If this leads to performance problems it might be better to introduce a
boolean member to the `Fader` class which caches that value.
@michaelgregorius
Copy link
Contributor Author

I have some new thoughts I wanted to discuss:

If you move the fader with the left mouse button then the amplification is calculated by taking the fader position into account. That's the reason why you get fractional values and IMO it is expected. Pixel positions are integer values and if you map these to [-120 dB, 6 dB] then this will yield fractional values.

This implies that you can get more accurate fader changes without having to be so precise with the mouse by simply making the Mixer bigger. This seems to be incorrect behavior.

I don't see how this is incorrect behavior. Mouse events are reported in integer coordinates. The more pixels you have available the more precision you gain. It's not that the scale suddenly changes when you increase the mixer height or that the behavior starts to change. A certain portion of the mixer is still mapped to the same amplification factors. So I see this increased fidelity as a bonus.

FL for example has the behavior such that changes in the fader using the mouse change the volume in steps of 0.1 dB.

I do not have FL available to check its behavior but I'd say that due to master also not implementing the stepping by 0.1 dB this is out of scope of this PR. I assume that implementing something like this will lead to similar complexities like with the knob where you have to either be able to consume raw mouse events (which Qt does not support as far as I know) or you have to reposition the mouse pointer constantly (which I am not even sure if that is supported under Wayland).

IMO the dB model should be used. It definitively has a benefit because it better describes how humans, i.e. our users, perceive loudness. Therefore it would also be better if the automations would use a dB model. If users want to decrease the volume in a way that is perceived as linear then they would simply draw a line in the dB model. With the current amplification model they must draw a curve to achieve the same effect, i.e. they must approximate the logarithmic effect.

I'm changing my mind about this. I think internally, using the linear scale [0, 200] was the right choice as handling the -inf dB case is a lot simpler, just as an example. Even other DAWs like FL seem to use the linear scale internally. Its just that they primarily show the volume in dB, while also ensuring that the fader changes the model value by an amount in dB and not by a linear one.

At a certain point the dB values will of course have to be transformed into "linear" values because that's what the signal is ultimately multiplied with. It's only a question of where this should happen and how it is implemented. One option might be that a model has an internal representation but that it might also represent itself differently to the outside. For example the amplification model might use linear values inside but it might have methods which transform these values into different "representational" values, e.g. dB, that are then used to represent that model in the GUI and certain other contexts.

Automations wouldn't be a problem here because fader changes should apply a change in dB, even if internally they use a linear scale.

If I understand correctly you mean that the automation will linearly automate the logarithmic fader which will then compute the corresponding linear values. Please note though that IMO this is a not the correct way to implement automations as explained here: #6954 (comment)

Automations should work directly on the parameters, i.e. [automation] -> [parameter] instead of automating some GUI representation, i.e. [automation] -> [knob] -> [parameter]. Automations, knobs and faders should just be different means to influence a parameter. Another reason to do so is the separation of concerns between core and GUI.

The model system in LMMS is horrifically broken, but I think this is how it should ideally work.

I don't know if it is horribly broken but it seems to need some work.

Another option would be to provide a new preference that lets users switch the ticks rendering on and off. The default could be to turn it off. I'd prefer that option because it would simply let the individual users choose if it's helpful for them.

IMO, if we want the ticks, I think they should be a bit more prominent while also having a better visual indication of what the current level the fader is at in dB (bigger, number markers, etc). Because unfortunately, I think that the tooltip does a much better job of conveying that information better than the ticks currently, so the ticks feel like unnecessary fashion.

Commit 57ad0e6 makes the rendering of the fader ticks a preference where the default is that it's turned off.

I agree that a better visual representation with numbers, etc. is desirable but it is also outside of the scope of this PR which is intended to mainly implement functional aspects. I even consciously decided against implementing such broad visual changes because I remembered discussion in the past where people reacted very sensitive to any size changes of the faders. The ticks were implemented such that the do not even increase the width of the faders by a pixel.

So if these visual changes are wanted I propose to implement them in another PR and then perhaps also in conjunction with an option/preference like "slim faders" and "detailed faders".

@sakertooth
Copy link
Contributor

I don't see how this is incorrect behavior. Mouse events are reported in integer coordinates. The more pixels you have available the more precision you gain. It's not that the scale suddenly changes when you increase the mixer height or that the behavior starts to change. A certain portion of the mixer is still mapped to the same amplification factors. So I see this increased fidelity as a bonus.

It may not be incorrect behavior necessarily, but being able to get more accurate fader changes just by having a bigger display and larger Mixer feels unusual to me. If you compare that to a normal slider (like QSlider) it works the way you would expect.

If I understand correctly you mean that the automation will linearly automate the logarithmic fader which will then compute the corresponding linear values. Please note though that IMO this is a not the correct way to implement automations as explained here: #6954 (comment)

Automations should work directly on the parameters, i.e. [automation] -> [parameter] instead of automating some GUI representation, i.e. [automation] -> [knob] -> [parameter]. Automations, knobs and faders should just be different means to influence a parameter. Another reason to do so is the separation of concerns between core and GUI.

You're right, not sure where I was going with that. We need to implement automation in the core, so we have to influence the parameter directly and not the UI representation.

I don't know if it is horribly broken but it seems to need some work.

Probably an exaggeration, but I agree it does need work.

So if these visual changes are wanted I propose to implement them in another PR and then perhaps also in conjunction with an option/preference like "slim faders" and "detailed faders".

That's fair. We can make improvements later.

@michaelgregorius
Copy link
Contributor Author

I don't see how this is incorrect behavior. Mouse events are reported in integer coordinates. The more pixels you have available the more precision you gain. It's not that the scale suddenly changes when you increase the mixer height or that the behavior starts to change. A certain portion of the mixer is still mapped to the same amplification factors. So I see this increased fidelity as a bonus.

It may not be incorrect behavior necessarily, but being able to get more accurate fader changes just by having a bigger display and larger Mixer feels unusual to me. If you compare that to a normal slider (like QSlider) it works the way you would expect.

QSlider is implemented to represent an interval of stepped integer values. You can for example set the minimum to 0 and the maximum to 999. This will allow you to represent the 1000 integer values in that interval. So it implements a discrete scale.

The Fader class on the other hand is intended to represent or simulate an analog fader which is not discrete but continuous. So I personally see no benefit in artificially limiting its precision by making it stepped.

@sakertooth
Copy link
Contributor

The Fader class on the other hand is intended to represent or simulate an analog fader which is not discrete but continuous. So I personally see no benefit in artificially limiting its precision by making it stepped.

Fair. There's no specifics on how a DAW should behave, which is probably why I see other DAWs doing different things all the time. And the user can still change the dB in steps if they want to with the modifier keys, so I guess its not really an issue.

Will do some testing before approving.

Copy link
Contributor

@sakertooth sakertooth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found an issue where in an empty project, you cant move the master channel fader up or down with the arrow keys until you make a new channel and toggle the selection between them. Overall though, the PR looks good.

src/gui/widgets/Fader.cpp Outdated Show resolved Hide resolved
@michaelgregorius
Copy link
Contributor Author

Found an issue where in an empty project, you cant move the master channel fader up or down with the arrow keys until you make a new channel and toggle the selection between them. Overall though, the PR looks good.

This is a problem that is already present in master. If you start LMMS, click the add channel button once or more and then press the right arrow then nothing happens. To make it work one can for example click into the grey area next to the channels so that the widget starts receiving key press events.

Unfortunately, I was not able to find a quick fix for this so the problem might be a bit more complex and perhaps worthy of its own bug report.

@michaelgregorius
Copy link
Contributor Author

michaelgregorius commented Feb 8, 2025

Thanks for the review and the approval @sakertooth!

@bratpeki, can I merge or would you like to give an explicit approval as well?

Edit: Changed wording.

@bratpeki
Copy link
Member

bratpeki commented Feb 9, 2025

Thanks for making this! Currently, 7477 is very close to getting merged, so I'm focused on that, then I'd like to take another proper look at this.

@tresf
Copy link
Member

tresf commented Feb 11, 2025

I don't see how this is incorrect behavior. Mouse events are reported in integer coordinates. The more pixels you have available the more precision you gain. It's not that the scale suddenly changes when you increase the mixer height or that the behavior starts to change. A certain portion of the mixer is still mapped to the same amplification factors. So I see this increased fidelity as a bonus.

It may not be incorrect behavior necessarily, but being able to get more accurate fader changes just by having a bigger display and larger Mixer feels unusual to me. If you compare that to a normal slider (like QSlider) it works the way you would expect.

Knobs, not sliders, but somewhat related convo:
#5360 (comment)

It can be changed later, but I don't think the cursor following the knob is as important as it sounds and the above link has some examples. (Not trying to slow progress here, just adding some validity to @sakertooth's instincts, which can be addressed at a later time).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants