Scalable consistent faders with themeable gradients, marker at unity, dbFS by default #7045

michaelgregorius · 2024-01-02T19:26:32Z

The changes mostly affect the mixer and the LMMS plugins Compressor, EQ and Delay.

How does it look/feel?

A video is worth a thousand words:
https://github.com/LMMS/lmms/assets/9293269/d74e2070-2e54-4a4d-8351-34bce4a215b1

FadersWithGradientsAndUnityLines.mp4

Detailed overview of the changes

The main changes are:

Fader levels are rendered in code using a gradient instead of pixmaps. This ensures a consistent rendering. Code that used the pixmaps has been removed completely. The pixmaps have also been deleted.
A marker at unity position is rendered, i.e. at position 1.0 in linear levels and 0 dbFS in dbFS scale. Rendering of the unity marker can be turned on and off via a style sheet property.
It's now possible to specify the pixmap that's used to render the knob (again). So the "Crossover EQ" now renders with its own knob images again.
Faders can be scaled/resized. The knob always renders along the mid line and the available space is used for the level indicators.
The peak indicators do not use the three distinct colors anymore but instead use the color of the gradient at that position of the peak. This makes everything look "smooth".
All faders now render in dbFS by default. This also improves the faders of the LMMS plugins Compressor, EQ and Delay.
The default min and max values for the faders has been adjusted to -42 dbFS and 9 dbFS. This also improves Compressor, EQ and Delay.
The style sheet names for the colors have been renamed to something more neutral: peakOk, peakClip, peakWarn. The colors are used for the stops of the gradient.

The problem with the pixmap approach was that the pixmaps don't "know" how a fader instance is configured with regards to the minimum and maximum value. This means that the display can give quite a wrong impression. Artists that create pixmaps also don't know the technical details how to design them in a way that the code would render them correctly.

The minimum size of the faders is set to the previous size of the background pixmap.

Levels can technically still be rendered in linear but it should be considered to remove that feature.

Some code simplifications have also been applied: constructor delegation, remove constructors that are not needed anymore, remove Fader::init.

Some more technical details can be found in the commit messages of the individual commits.

Here's how larger faders render/look:

A clipping fader that shows the full scale of the gradient:

Look of the LMMS Compressor:

Look of the LMMS Equalizer:

Look of the LMMS Delay:

Look of the LMMS Crossover EQ:

Render the fader level in code using a gradient instead of using pixmaps. The problem with the pixmaps is that they don't "know" how a fader instance is configured with regards to the minimum and maximum value. This means that the display can give quite a wrong impression. The rendering of levels has been unified in the method `paintLevels`. It can render using dbFS and linear scale. The method `paintLinearLevels` has been removed completely, i.e. there's no more code that renders using pixmaps. Much of the previous code relied on the size of the background image `fader_background.png`, e.g. the initialization of the size. For now the `Fader` widget is initially resized to the size of that background image as it is present in the default and classic theme (see `Fader::init`). All rendering uses the size of the widget itself to determine where to draw what. This means that the widget is prepared to be resizable. The method `paintLevels` first renders the background of the level indicators and uses these as clipping paths for all other rendering operations, e.g. for the rendering of the levels themselves. Levels are rendered using a gradient which is defined with the following stops: * Two stops for the ok levels. * One stop for warning levels. * One stop for clipping levels. Peak indicators do not use the three distinct colors anymore but instead use the color of the gradient at that position of the peak. This makes everything look "smooth". The code now also renders a marker at unity position, i.e. at position 1.0 in linear levels and 0 dbFS in dbFS scale. The painting code makes lots of use of the class `PaintHelper`. This class is configured with a minimum and maximum value and can then return linear factors for given values. There are two supported modes: * Map min to 0 and max to 1 * Map min to 1 and max to 0 It can also compute rectangles that correspond to a given value. These methods can be given rectangles that are supposed to represent the span from min to max. The returned result is then a rectangle that fills the lower part of the source rectangle according to the given value with regards to min and max (`getMeterRect`). Another method returns a rectangle of height 1 which lies inside the given source rectangle at the corresponding level (`getPersistentPeakRect`). The method `paintLevels` uses a mapping function to map the amplitude values (current peak value, persistent peak, etc.) to the display values. There's one mapper that keeps the original value and it is used to display everything in a linear scale. Another mapper maps everything to dbFS and uses these values as display everything in a dbFS scale. The following values must be mapped for the left and right channel to make this work: * Min and max display values (min and max peak values) * The current peak value * The persistent peak value * The value for unity, i.e. 1.0 in linear levels and 0 dbFS in dbFS scale. Remove the method `calculateDisplayPeak` which was used in the old method to render linear levels. `Fader::setPeak` now uses `std::clamp` instead of doing "manual" comparisons. The LMMS plugins Compressor, EQ and Delay are still configured to use linear displays. It should be considered to switch them to dbFS/logarithmic displays as well and to remove the code that renders linearly.

Remove the now unused pixmaps for the background and the LEDs from the `Fader` class and remove the files from the default and classic theme directories.

Rename the peak properties as follows: * peakGreen -> peakOk * peakRed -> peakClip * peakYellow -> peakWarn The reasoning is that a style might for example use a different color than green to indicate levels that are ok. Use the properties to initialize the gradient that is used to render the levels. Initialize the properties to the colors of the current default theme so that it's not mandatory to set them in a style sheet. Up until now they have all been initialized as black.

Render the knob in the middle of the fader regardless of the width. The previous implementation was dependent on the fader pixmap having a matching width because it always rendered at x=0.

Set the size policy of the fader to minimum expanding in both directions. This will make the fader grow in layouts if there is space.

Default to dbFS levels for all faders and set some better minimum and maximum peak values.

Fix the faders of the Crossover EQ which were initialized and rendered much too wide and with a line at unity. The large width also resulted in the knobs being rendered outside of view. Resize the fader to the minimum size so that it is constructed at a sane default. Introduce a property that allows to control if the unity line is rendered. The property is available in style sheets and defaults to the unity lines being rendered. Adjust the paint code to evaluate the property. Initialize the faders of the Crossover EQ such that the unity line is not drawn.

Remove the constructor of `EqFader` that takes the pixmaps to the fader background, leds and knob. The background and leds pixmaps are not used by the base class `Fader` for rendering anymore to make the `Fader` resizable. A pixmap is still used to render the knob but the constructor that takes the knob as an argument does not do anything meaningful with it, i.e. all faders are rendered with the default knob anyway. Remove the resources for the fader background, leds and knob as they are not used and the knob was the same image as the default knob anyway. Remove the static pixmaps from the constructor of `EqControlsDialog`. Switch the instantiations of the EQ's faders to use the remaining constructor of `EqFader`. This constructor sets a different fixed size of (23, 116) compared to the removed constructor which set a size of (23, 80). Therefore all faders that used the removed constructor are now set explicitly to a fixed size of (23, 80). The constructor that's now used also calls a different base constructor than the removed one. The difference between the two base constructors of `Fader` is that one of them sets the member `m_conversionFactor` to 100.0 whereas the other one keeps the default of 1.0. The adjusted faders in `EqControlsDialog` are thus now constructed with the conversion factor set to 100. However, all of them already call `setDisplayConversion` with `false` after construction which results in the conversion factor being reset to 1.0. So the result should be the same as before the changes.

Remove the parameters for the background and LEDs pixmap from the second `Fader` constructor. Make the knob pixmap parameter in the constructor a const reference. Assign the reference to the knob pixmap of the `Fader` itself. This enables clients to use their own fader knobs as is the case with the Crossover EQ. The EQ now renders using it's own knobs again. Make the second constructor delegate to the first one. This will additionally set the conversion factor to 100 but this is not a problem with the current code because the only user of the second constructor, the Crossover EQ, already calls `setDisplayConversion` with the parameter set to `false`, hence reinstating a conversion factor of 1. Remove the resources for the background and LEDs from the Crossover EQ as they are not used anymore. Remove the three QPixmap members from `CrossoverEQControlDialog` as they are not needed. The background and LEDs are not used anyway and the knob is passed in as a constant reference which is copied. Hence we can use a local variable in the constructor of `CrossoverEQControlDialog`.

Remove the `init` method from `Fader` as it is not needed anymore due to the constructor delegation. Tidy up the parameter lists and use of spaces in the constructor.

…adients Conflicts: * include/Fader.h * src/gui/widgets/Fader.cpp

he29-net · 2024-01-02T23:42:20Z

Some small issues at UHD resolution (2x scale):

the Mixer is a bit oversized: each fader is a bit wider (possibly related to the increased padding in LCDs?) and taller (probably because of increased padding between SEND/knob/arrows);
some of the longer labels are cropped at the beginning;
LED toggles seem to be drawn at 2x size but within 1x boundary.

I'm not sure about the smooth gradients: the sharp contrast between green and yellow was very easy to notice at a glance, drawing attention to channels that may be approaching clipping. Not so much with the gradient. Maybe the gradient stops could be doubled, for example using thresholds like peakWarnLow = 0.75, peakWarnHigh = 0.8, resulting in a sharper change and areas that are fully green, yellow, and red? (The number is completely random, no idea what thresholds are currently used). But it may be just a personal preference.

Great work overall; a nice step towards fully scalable UI.

he29-net · 2024-01-02T23:47:53Z

One even smaller issue: the unity line overlaps with the borders, resulting in bright pixels where they cross:

michaelgregorius · 2024-01-03T15:29:07Z

Thanks for checking @he29-net!

Some small issues at UHD resolution (2x scale):

* the Mixer is a bit oversized: each fader is a bit wider (possibly related to the increased padding in LCDs?) and taller (probably because of increased padding between SEND/knob/arrows);

* some of the longer labels are cropped at the beginning;

* LED toggles seem to be drawn at 2x size but within 1x boundary.

I think all these issues are related to the layout changes that have been made with pull request #6591. So I propose to create a new issue for these so that we do not mix topics (mixer channels with layouts vs. fader changes).

I'm not sure about the smooth gradients: the sharp contrast between green and yellow was very easy to notice at a glance, drawing attention to channels that may be approaching clipping. Not so much with the gradient. Maybe the gradient stops could be doubled, for example using thresholds like peakWarnLow = 0.75, peakWarnHigh = 0.8, resulting in a sharper change and areas that are fully green, yellow, and red? (The number is completely random, no idea what thresholds are currently used). But it may be just a personal preference.

We can also double the yellow area , similar to how it's done for the green area (see here in the code).

With the current code everything below -12 dbFS is fully green. Then it becomes yellow as it approaches unity and from there on it goes into red. So as part of the fix we could also increase the dbFS value of the last fully green signal, e.g. -6 dbFS or -3 dbFS.

Any preferences with regards to the transitions? What should be the last fully green dbFS value? At how many dbFS should we approach the full on warning area? How far should it exceed? At what dbFS value are we at full clipping?

Great work overall; a nice step towards fully scalable UI.

Thanks! 😃

michaelgregorius · 2024-01-03T15:50:27Z

One even smaller issue: the unity line overlaps with the borders, resulting in bright pixels where they cross:
[snip]

I think this might be caused by the order of rendering. Currently I render as follows:

Draw the rounded background (and use it to clip the remaining draw calls)
Draw the levels
Draw the unity lines
Finally do some white-ish overdraw of the background's outline to approach the look of the original pixmap.

I think it might help if I draw the unity lines as the very last step.

Draw the unity lines last so that they are not affected by the white-ish overdraw in line 423 (after the changes).

michaelgregorius · 2024-01-03T15:53:50Z

With commit 045a2cd the unity lines are now drawn last.

Introduce a second point in the gradient for the warn colors so that we get a certain range with the full/solid warn color. The colors are distributed as follows now. The solid ok range goes from -inf dbFS to -12 dbFS. The warn range goes from -6 dbFS to 0 dbFS. In between the colors are interpolated. Values above 0 dbFS interpolate from the warn color to the clip color. This is now quite similar to the previous implementation. # Analysis of the previous pixmap implementation The pixmap implementation used pixmaps with a height of 116 pixels to map 51 dbFS (-42 dbFS to 9 dbFS) across the whole height. The pixels of the LED pixmap were distributed as follows along the Y-axis: * Margin: 4 * Red: 18 * Yellow: 14 * Green: 76 * Margin: 4 Due to the margins the actual red, yellow and green areas only represent a range of (1 - (4+4) / 116) * 51 ~ 47,48 dbFS. This range is distributed as follows across the colors: Red: 7.91 dbFS Yellow: 6.16 dbFS Green: 33.41 dbFS The borders between the colors are located along the following dbFS values: * Red/yellow: 9 - (4 + 18) / 116 * 51 dbFS ~ -0.67 dbFS * Yellow/green: 9 - (4 + 18 + 14) / 116 * 51 dbFS ~ -6.83 dbFS * The green marker is rendered for values above -40.24 dbFS.

michaelgregorius · 2024-01-06T14:51:51Z

@he29-net, with commit a644bdc there is now a warn range with a solid yellow color. It goes from -6 dbFS to 0 dfBS. I have analyzed the previous implementation (see below) and have chosen the values such that they look very similar.

Here's how it looks:

Analysis of the previous pixmap implementation

The pixmap implementation used pixmaps with a height of 116 pixels to map 51 dbFS (-42 dbFS to 9 dbFS) across the whole height. The pixels of the LED pixmap were distributed as follows along the Y-axis:

Margin: 4
Red: 18
Yellow: 14
Green: 76
Margin: 4

Due to the margins the actual red, yellow and green areas only represent a range of (1 - (4+4) / 116) * 51 ~ 47,48 dbFS. This range is distributed as follows across the colors:
Red: 7.91 dbFS
Yellow: 6.16 dbFS
Green: 33.41 dbFS

The borders between the colors are located along the following dbFS values:

Red/yellow: 9 - (4 + 18) / 116 * 51 dbFS ~ -0.67 dbFS
Yellow/green: 9 - (4 + 18 + 14) / 116 * 51 dbFS ~ -6.83 dbFS
The green marker is rendered for values above -40.24 dbFS.

Adjust the `Fader` so that it can correctly render arbitrary ranges of min and max peak values, e.g. that it would render a display range of [-12 dbFS, -42 dbFS] correctly. Until now the gradient was defined to start at the top of the levels rectangle and end at the bottom. As a result the top was always rendered in the "clip" color and the bottom in the "ok" color. However, this is wrong, e.g. if we configure the `Fader` with a max value of -12 dbFS and a min value of -42 dbFS. In that case the whole range of the fader should be rendered with the "ok" color. The fix is to compute the correct window coordinates of the start and end point of gradient using from the "window" of values that the `Fader` displays and then to map the in-between colors accordingly. See the added comments in the code for more details. Add the templated helper class `LinearMap` to `lmms_math.h`. The class defines a linear function/map which is initialized using two points. With the `map` function it is then possible to evaluate arbitrary X-coordinates.

Remove the now unused mapping methods from `PaintHelper`. Their functionality has been replaced with the usage of `LinearMap` in the code.

Include `cassert` for some builds that otherwise fail.

michaelgregorius · 2024-01-06T19:49:04Z

Hi @LMMS/developers,

I consider this pull request done now and would like to request a review so that the changes can be merged. Please note that this PR is not just a cosmetic change but that it fixes fundamental functionality of the Fader class. Therefore I propose to include this PR in the next 1.3 release.

The fix can be demonstrated by configuring the Fader with a minimum peak value of -42 dbFS and a maximum peak value of -12 dbFS. In this case the correct rendering is an all green line if the fader is maxed out. This in fact now happens with the PR:

FadersRangeCorrected.mp4

And here's how the setup with a range of [-42, -12] dbFS renders in the current master:

FadersRangeWrong.mp4

The master just renders the whole pixmap which makes it look as it there's some heavy clipping going on when in fact it isn't.

Please refer to the original PR message for all other additional improvements.

he29-net · 2024-01-06T20:13:10Z

with commit a644bdc there is now a warn range with a solid yellow color.

Great, subjectively I think it looks better now. Maybe the orange just above the unity line could more aggressively inform the user that "hey, danger, we are clipping now", but I think the addition of the unity line itself takes care about that. Combined with the colored peak markers I think it is sufficient.

Speaking of the unity line, 045a2cd did not really change anything for me. This is how it looks in the mixer:

It's not as pronounced on the lighter background compared to EQ, but the bright dots are still there. I think the issue is simply the alpha of 127 in unityMarkerColor, which allows it to mix with the dark (but not black) border under it. So maybe the line should be made completely opaque, or drawn one pixel shorter on each side so there is no overlap?

As a side note, I almost complained about uneven alignment on the red faders; turns out it is real, but it's caused by the physical distance of red and green subpixels on my LCD... :) So not a code issue.

(8 white pixels were added in editor)

Make the unity marker opaque by default and enable to style it with the style sheets. None of the two style sheets uses this option though.

michaelgregorius · 2024-01-06T21:00:56Z

Hi @he29-net,

with commit 99113c3 the unity marker now defaults to an opaque color. It is now also possible to style this color via the style sheet.

With regards to warning the users about clipping: I think in some situations, especially with regards to mixing in digital, it might be okay for one or more channels to "clip", for example if they get routed to another channel which attenuates the mix again. On the master channel on the other hand it's interesting to get warned about clipping though. So it might all be relative anyway.

he29-net · 2024-01-06T21:05:21Z

After 99113c3 the unity line is now pixel-perfect. 👍

Maybe (63, 63, 63, 255) would be a better choice for the default though, if we are not updating the styles? It would more or less match the previous appearance, i.e. it would blend better with the dark background.

(Sorry for constantly pointing out tiny graphical issues.. ^^; This is why I prefer working on embedded systems: there is usually no graphical interface the user can complain about, haha..)

michaelgregorius · 2024-02-22T19:37:01Z

Thanks for the further input @curlymorphic!

I will then try to adjust the Fader class so that it can be configured to also take dbFS values as input and render them.

Doing so will enable the following changes to EqEffect::peakBand:

It can return the raw dbFS value instead of some mapped value.
It will also make sense to reduce the minimum decibel value to something like -144 dbFS because that seems to be the noise floor for float values. After all we are working in digital and not with noisy consoles from the 60ies. 😉 How these values are displayed, i.e. their minimum and maximum range, should be implemented in the presentation layer and not in the "business"/core layer.

I am also wondering about several things:

Is the current mapping correct in the first place? I am asking because it seems that it will map a peak of 40 dbFS to unity instead of 0 dbFS: (40 + 60) / 100 = 1.
Why is the maximum peak of a single bin taken as the peak value? I'd argue that the other bins will in many cases have an effect as well. If I'd take the IFFT of the bins in the frequency range then the individual waves could interfere constructively or destructively due to phase, etc.

curlymorphic · 2024-02-22T20:31:21Z

I was looking yesterday for an old post where Diizy, the lead dev at the time was insistent as to how this should be displayed. I seem to remember stating that this behavior is not the same as all the other DAW's. I agree the range is not what I would expect. This could well be the time for change.

I have no clue as to why this line of code was ever to use (40+60) / 100, but I would assume at some point someone didn't like the original implementation. The original code that I found using git blame was return (peak+100)/100;

Why is the maximum peak of a single bin taken as the peak value?
Because this was a simple visualisation, that would give values that matched the large fft display area. You are correct that multiple bins within the range could have better analysis, but you may wish to deal with that as a separate issue, as it may well be outside the scope of this PR.

As a bit of history, The EQ plugin was my first ever released module, and you may well find many areas that need improvement. I looked at the images in the original PR and it looks very different today, a credit to all the developers that have done revisions since. Thank you for continuing the effort.

The peak values for the shelves and peaks of the Equalizer plugin are computed in `EqEffect::peakBand`. The previous implementation evaluated the bins of the corresponding frequency spectrum and determined the "loudest" one. The value of this bin was then converted to dbFS and mapped to the interval [0, inf[ where all values less or equal to -60 dbFS were mapped to 0 and a value of 40 dbFS was mapped to 1. So effectively everything was mapped somewhere into [0, 1] yet in a quite "distorted" way because a signal of 40 dbFS resulted in being displayed as unity in the fader. This commit directly returns the value of the maximum bin, i.e. it does not map first to dbFS and then linearize the result anymore. This should work because the `Fader` class assumes a "linear" input signal and if the value of the bin was previously mapped to dbFS it should have some "linear" character. Please note that this is still somewhat of a "proxy" value because ideally the summed amplitude of all relevant bins in the frequency range would be shown and not just the "loudest" one. ## Other changes Rename `peakBand` to `linearPeakBand` to make more clear that a linear value is returned. Handle a potential division by zero by checking the value of `fft->getEnergy()` before using it. Index into `fft->m_bands` so that no parallel incrementing of the pointer is needed. This also enables the removal of the local variable `b`.

michaelgregorius · 2024-02-24T13:03:56Z

I have solved this differently now. Instead of extending the Fader for this single case I decided to remove the mapping to [0, 1] (or technically rather [0, inf[) that's done at the end and to convert the dbFS values to linear values because that's what the Fader expects with regards to the peaks. Then I realized that the code would effectively do linear -> dbFS -> linear if I did this and that I can simply use the maximum bin as calculated in the for loop.

The corresponding changes are introduced via commit 449ab02 which also introduces the handling of a potential division by zero. It also removes the clamping to -60 dbFS so that the fader can decide how to present the raw data.

Thanks for introducing the Equalizer in the first place, @curlymorphic! 🙂

michaelgregorius · 2024-02-24T16:37:42Z

I consider this pull request ready for review and merge now. There are several other things that could be done in separate PRs once this one is merged:

Render and evaluate the fader knob on a dbFS scale instead of a linear one. The models should be kept linear though because they can be automated and switching them to dbFS values would likely introduce too much data upgrade efforts for save files. A linear model with the interval [0, 2] also enables real silence whereas a dbFS model would have to map -∞ somehow.
Make the mixer vertically resizable so that additional available space could be use by the faders.
Unification of Fader and EqFader (if it is really wanted)
More accurate computation of the Equalizer peak levels (if it is efficiently possible in FFT space)

RoxasKH · 2024-03-05T17:20:04Z

@RoxasKH, I have added some commits which render the clipping area as a flat/solid region. The full scale currently looks as follows:

The change also showed that there was a problem with the unity marker. This was fixed by replacing the helper class PaintHelper with another mechanism.

Here's a test file that can be used to test the fader. It consists of a sine that's played at unity: MixerDevelopmentSineAtZero.zip

I know i'm late but I'm in favour of this cause LMMS needs everything it can have to be dinamically rendered, so i'll add a bit from a design perspective.

Here's a 1:1 view against the current faders

Now, aside from the sizes of the mixer buses itself

We're losing both the slight outline and the curvature at the ends of the faders, which made it more polished than the abrupt cut which is now present to m

The volume levels dimensions changed? Now bars have a width of 6px, while the old one were of 7. This is inherently bad, but while there are other daws with thin levels, they do not have the volume fader overlayed on the levels themselves, so i believe having them larger is actually better.

The bars is slightly larger than the fader so it's 7 px. This creates a weird effect where the bar seems to be getting thinner when it peaks, as the light color of the bar kind of merges with the yellow part of the volume. Now there's more than 1 single possible fix; FL Studio which is my reference cause it's the only one having bars on the volume levels, keep them thinner than the level, so that the bar is actually visible only when it's not hovered: at the end of the day once the bar gets covered the red color itself will tell the producer about the clipping.

On active faders the color of the bar makes it merge with the background as well, so it might be worth changing color? Not sure actually, once made them thinner there should be a better separation.
Actually, by bringing back the volume levels to 7px wide they should cover the line by themselves with no need to make them thinner i guess.

Obviously this are probably all minor design things, but i believe they should be addressed when this will substitute the current faders.

The levels rendering now more explicitly distinguished between the rendering of the level outline/border and the level meters. The level rectangles are "inset" with regards to the borders so that there is a margin between the level borders and the meter readings. This margin is now also applied to the top and bottom of the levels. Levels are now also rendered as rounded rectangles similar to the level borders. Only render the levels and peaks if their values are greater than the minimum level. Make the radius of the rounded rectangles more pronounced by increasing its value from 1 to 2. Decrease the margins so that the level readings become wider, i.e. so that they are rendered with more pixels. Add the lambda `computeLevelMarkerRect` so that the rendering of the level markers is more decoupled from the rendering of the peak markers.

michaelgregorius · 2024-03-10T19:01:33Z

Thanks for your feedback, @RoxasKH!

Most of your points should have been addressed by commit b4bfc71:

It introduces a margin in all directions between the level borders and the level meters.
Levels are now also rendered as rounded rectangles similar to the level borders.
The radius of the rounded rectangles was increased to make it more pronounced.
The margins have been decreased so that the level readings become wider.
The unity indicator is now rendered with the same width as the level meters so that they become fully covered by them.

Other improvements:

Only render the levels and peaks if their values are greater than the minimum level. Previously there was always a slight green line at the bottom which I did not see due to fractional scaling.

Here's how it looks:

michaelgregorius · 2024-03-10T19:13:04Z

The failing MSVC builds seem to be a problem with Cloudflare, see here.

…adients Conflicts: * plugins/Eq/EqEffect.cpp

michaelgregorius · 2024-03-22T17:05:06Z

I have merged the current master to fix the self-inflicted merge conflict that was introduced with my pull request #7141. 😅

Ready for review. 😉

RoxasKH · 2024-03-22T20:17:14Z

Thanks for your feedback, @RoxasKH!

Most of your points should have been addressed by commit b4bfc71:

* It introduces a margin in all directions between the level borders and the level meters.

* Levels are now also rendered as rounded rectangles similar to the level borders.

* The radius of the rounded rectangles was increased to make it more pronounced.

* The margins have been decreased so that the level readings become wider.

* The unity indicator is now rendered with the same width as the level meters so that they become fully covered by them.

Other improvements:

* Only render the levels and peaks if their values are greater than the minimum level. Previously there was always a slight green line at the bottom which I did not see due to fractional scaling.

Here's how it looks:

Looks good to me.

We're just losing the small border highlight around them, but i guess it's not a big deal, you choose if you feel like it's worth being perfectionist and working on that or not.

I have some other concerns about the mixer stuff placement, positioning and width (and cursor not changing to pointer when hovering the send button), but i'm sure those were merged with another PR as i saw the same things happening in the current nightly build.

It's stuff that can be fixed later somewhere else and doesn't need to be addressed here tho.

include/Fader.h

plugins/Eq/EqEffect.cpp

include/lmms_math.h

src/gui/widgets/Fader.cpp

Reduce code repetition in `EqEffect::setBandPeaks` by introducing a lambda. Adjust code formatting.

Code review changes in `Fader.cpp`. Mostly whitespace adjustments. Split up the calculation of the meter width to make it more understandable. This also reduces the number of parentheses.

Use `MEMBER` instead of `READ`/`WRITE` for some properties that are not called explicitly from outside of the class.

Use default member initializers for the members in `Fader` that have previously been initialized in the constructor list. Mark `ampToDbfs` and `dbfsToAmp` as `constexpr`. The function `dbfsToAmp` is used in the initialzation and this way nothing has to be computed at runtime.

Remove `constexpr` from `ampToDbfs` and ` dbfsToAmp` again because the MSVC compiler does not like it. Seems that for some reason `log10f` and `std::pow` are not `constexpr` there (at least in the version used on the build server).

include/Fader.h

plugins/Eq/EqEffect.cpp

…adients

Make code clearer in `Fader::FadermouseDoubleClickEvent`. Only divide if the dialog was accepted with OK.

… dbFS by default (LMMS#7045) * Render fader levels in code with a gradient Render the fader level in code using a gradient instead of using pixmaps. The problem with the pixmaps is that they don't "know" how a fader instance is configured with regards to the minimum and maximum value. This means that the display can give quite a wrong impression. The rendering of levels has been unified in the method `paintLevels`. It can render using dbFS and linear scale. The method `paintLinearLevels` has been removed completely, i.e. there's no more code that renders using pixmaps. Much of the previous code relied on the size of the background image `fader_background.png`, e.g. the initialization of the size. For now the `Fader` widget is initially resized to the size of that background image as it is present in the default and classic theme (see `Fader::init`). All rendering uses the size of the widget itself to determine where to draw what. This means that the widget is prepared to be resizable. The method `paintLevels` first renders the background of the level indicators and uses these as clipping paths for all other rendering operations, e.g. for the rendering of the levels themselves. Levels are rendered using a gradient which is defined with the following stops: * Two stops for the ok levels. * One stop for warning levels. * One stop for clipping levels. Peak indicators do not use the three distinct colors anymore but instead use the color of the gradient at that position of the peak. This makes everything look "smooth". The code now also renders a marker at unity position, i.e. at position 1.0 in linear levels and 0 dbFS in dbFS scale. The painting code makes lots of use of the class `PaintHelper`. This class is configured with a minimum and maximum value and can then return linear factors for given values. There are two supported modes: * Map min to 0 and max to 1 * Map min to 1 and max to 0 It can also compute rectangles that correspond to a given value. These methods can be given rectangles that are supposed to represent the span from min to max. The returned result is then a rectangle that fills the lower part of the source rectangle according to the given value with regards to min and max (`getMeterRect`). Another method returns a rectangle of height 1 which lies inside the given source rectangle at the corresponding level (`getPersistentPeakRect`). The method `paintLevels` uses a mapping function to map the amplitude values (current peak value, persistent peak, etc.) to the display values. There's one mapper that keeps the original value and it is used to display everything in a linear scale. Another mapper maps everything to dbFS and uses these values as display everything in a dbFS scale. The following values must be mapped for the left and right channel to make this work: * Min and max display values (min and max peak values) * The current peak value * The persistent peak value * The value for unity, i.e. 1.0 in linear levels and 0 dbFS in dbFS scale. Remove the method `calculateDisplayPeak` which was used in the old method to render linear levels. `Fader::setPeak` now uses `std::clamp` instead of doing "manual" comparisons. The LMMS plugins Compressor, EQ and Delay are still configured to use linear displays. It should be considered to switch them to dbFS/logarithmic displays as well and to remove the code that renders linearly. * Remove unused pixmaps from `Fader` Remove the now unused pixmaps for the background and the LEDs from the `Fader` class and remove the files from the default and classic theme directories. * Rename peak properties and use them to render levels Rename the peak properties as follows: * peakGreen -> peakOk * peakRed -> peakClip * peakYellow -> peakWarn The reasoning is that a style might for example use a different color than green to indicate levels that are ok. Use the properties to initialize the gradient that is used to render the levels. Initialize the properties to the colors of the current default theme so that it's not mandatory to set them in a style sheet. Up until now they have all been initialized as black. * Always render the knob in the middle of the fader Render the knob in the middle of the fader regardless of the width. The previous implementation was dependent on the fader pixmap having a matching width because it always rendered at x=0. * Set size policy of fader to minimum expanding Set the size policy of the fader to minimum expanding in both directions. This will make the fader grow in layouts if there is space. * Default dbFS levels and better peak values Default to dbFS levels for all faders and set some better minimum and maximum peak values. * Fix faders of Crossover EQ Fix the faders of the Crossover EQ which were initialized and rendered much too wide and with a line at unity. The large width also resulted in the knobs being rendered outside of view. Resize the fader to the minimum size so that it is constructed at a sane default. Introduce a property that allows to control if the unity line is rendered. The property is available in style sheets and defaults to the unity lines being rendered. Adjust the paint code to evaluate the property. Initialize the faders of the Crossover EQ such that the unity line is not drawn. * Remove EqFader constructor with pixmaps Remove the constructor of `EqFader` that takes the pixmaps to the fader background, leds and knob. The background and leds pixmaps are not used by the base class `Fader` for rendering anymore to make the `Fader` resizable. A pixmap is still used to render the knob but the constructor that takes the knob as an argument does not do anything meaningful with it, i.e. all faders are rendered with the default knob anyway. Remove the resources for the fader background, leds and knob as they are not used and the knob was the same image as the default knob anyway. Remove the static pixmaps from the constructor of `EqControlsDialog`. Switch the instantiations of the EQ's faders to use the remaining constructor of `EqFader`. This constructor sets a different fixed size of (23, 116) compared to the removed constructor which set a size of (23, 80). Therefore all faders that used the removed constructor are now set explicitly to a fixed size of (23, 80). The constructor that's now used also calls a different base constructor than the removed one. The difference between the two base constructors of `Fader` is that one of them sets the member `m_conversionFactor` to 100.0 whereas the other one keeps the default of 1.0. The adjusted faders in `EqControlsDialog` are thus now constructed with the conversion factor set to 100. However, all of them already call `setDisplayConversion` with `false` after construction which results in the conversion factor being reset to 1.0. So the result should be the same as before the changes. * Remove background and LEDs pixmap from Fader constructor Remove the parameters for the background and LEDs pixmap from the second `Fader` constructor. Make the knob pixmap parameter in the constructor a const reference. Assign the reference to the knob pixmap of the `Fader` itself. This enables clients to use their own fader knobs as is the case with the Crossover EQ. The EQ now renders using it's own knobs again. Make the second constructor delegate to the first one. This will additionally set the conversion factor to 100 but this is not a problem with the current code because the only user of the second constructor, the Crossover EQ, already calls `setDisplayConversion` with the parameter set to `false`, hence reinstating a conversion factor of 1. Remove the resources for the background and LEDs from the Crossover EQ as they are not used anymore. Remove the three QPixmap members from `CrossoverEQControlDialog` as they are not needed. The background and LEDs are not used anyway and the knob is passed in as a constant reference which is copied. Hence we can use a local variable in the constructor of `CrossoverEQControlDialog`. * Remove the init method from Fader Remove the `init` method from `Fader` as it is not needed anymore due to the constructor delegation. Tidy up the parameter lists and use of spaces in the constructor. * Introduce range with solid warn color Introduce a second point in the gradient for the warn colors so that we get a certain range with the full/solid warn color. The colors are distributed as follows now. The solid ok range goes from -inf dbFS to -12 dbFS. The warn range goes from -6 dbFS to 0 dbFS. In between the colors are interpolated. Values above 0 dbFS interpolate from the warn color to the clip color. This is now quite similar to the previous implementation. # Analysis of the previous pixmap implementation The pixmap implementation used pixmaps with a height of 116 pixels to map 51 dbFS (-42 dbFS to 9 dbFS) across the whole height. The pixels of the LED pixmap were distributed as follows along the Y-axis: * Margin: 4 * Red: 18 * Yellow: 14 * Green: 76 * Margin: 4 Due to the margins the actual red, yellow and green areas only represent a range of (1 - (4+4) / 116) * 51 ~ 47,48 dbFS. This range is distributed as follows across the colors: Red: 7.91 dbFS Yellow: 6.16 dbFS Green: 33.41 dbFS The borders between the colors are located along the following dbFS values: * Red/yellow: 9 - (4 + 18) / 116 * 51 dbFS ~ -0.67 dbFS * Yellow/green: 9 - (4 + 18 + 14) / 116 * 51 dbFS ~ -6.83 dbFS * The green marker is rendered for values above -40.24 dbFS. * Remove unused method Fader::clips * Fader: Correctly render arbitrary ranges Adjust the `Fader` so that it can correctly render arbitrary ranges of min and max peak values, e.g. that it would render a display range of [-12 dbFS, -42 dbFS] correctly. Until now the gradient was defined to start at the top of the levels rectangle and end at the bottom. As a result the top was always rendered in the "clip" color and the bottom in the "ok" color. However, this is wrong, e.g. if we configure the `Fader` with a max value of -12 dbFS and a min value of -42 dbFS. In that case the whole range of the fader should be rendered with the "ok" color. The fix is to compute the correct window coordinates of the start and end point of gradient using from the "window" of values that the `Fader` displays and then to map the in-between colors accordingly. See the added comments in the code for more details. Add the templated helper class `LinearMap` to `lmms_math.h`. The class defines a linear function/map which is initialized using two points. With the `map` function it is then possible to evaluate arbitrary X-coordinates. * Remove unused methods in PaintHelper Remove the now unused mapping methods from `PaintHelper`. Their functionality has been replaced with the usage of `LinearMap` in the code. * Fix some builds Include `cassert` for some builds that otherwise fail. * Opaque unity marker with styling option Make the unity marker opaque by default and enable to style it with the style sheets. None of the two style sheets uses this option though. * Darker default color for the unity line * Move code Move the computation of most mapped values at the top right after the definition of the mapper so that it is readily available in all phases of the painting code. * Render unity lines in background Render the unity lines before rendering the levels so that they get overdrawn and do not stick out when they are crossed. * Don't draw transparent white lines anymore Don't draw the transparent white lines anymore as they were mostly clipped anyway and only create "smudge". * Full on clip color at unity Adjust the gradient so that the full on clip color shows starting at unity. There is only a very short transition from the end of warning to clipping making it look like a solid color "standing" on top of a gradient. * Fix discrepancy between levels and unity markers This commit removes the helper class `PaintHelper` and now uses two lambdas to compute the rectangles for the peak indicators and levels. It uses the linear map which maps the peak values (in dbFS or linear) to window coordinates of the widget. The change fixes a discrepancy in the following implementation for which the full on clip rectangle rendered slightly below the unity marker. * Fix fader display for Equalizer shelves and peaks The peak values for the shelves and peaks of the Equalizer plugin are computed in `EqEffect::peakBand`. The previous implementation evaluated the bins of the corresponding frequency spectrum and determined the "loudest" one. The value of this bin was then converted to dbFS and mapped to the interval [0, inf[ where all values less or equal to -60 dbFS were mapped to 0 and a value of 40 dbFS was mapped to 1. So effectively everything was mapped somewhere into [0, 1] yet in a quite "distorted" way because a signal of 40 dbFS resulted in being displayed as unity in the fader. This commit directly returns the value of the maximum bin, i.e. it does not map first to dbFS and then linearize the result anymore. This should work because the `Fader` class assumes a "linear" input signal and if the value of the bin was previously mapped to dbFS it should have some "linear" character. Please note that this is still somewhat of a "proxy" value because ideally the summed amplitude of all relevant bins in the frequency range would be shown and not just the "loudest" one. ## Other changes Rename `peakBand` to `linearPeakBand` to make more clear that a linear value is returned. Handle a potential division by zero by checking the value of `fft->getEnergy()` before using it. Index into `fft->m_bands` so that no parallel incrementing of the pointer is needed. This also enables the removal of the local variable `b`. * Improve the rendering of the levels The levels rendering now more explicitly distinguished between the rendering of the level outline/border and the level meters. The level rectangles are "inset" with regards to the borders so that there is a margin between the level borders and the meter readings. This margin is now also applied to the top and bottom of the levels. Levels are now also rendered as rounded rectangles similar to the level borders. Only render the levels and peaks if their values are greater than the minimum level. Make the radius of the rounded rectangles more pronounced by increasing its value from 1 to 2. Decrease the margins so that the level readings become wider, i.e. so that they are rendered with more pixels. Add the lambda `computeLevelMarkerRect` so that the rendering of the level markers is more decoupled from the rendering of the peak markers. * Reduce code repetition Reduce code repetition in `EqEffect::setBandPeaks` by introducing a lambda. Adjust code formatting. * Code review changes Code review changes in `Fader.cpp`. Mostly whitespace adjustments. Split up the calculation of the meter width to make it more understandable. This also reduces the number of parentheses. * Use MEMBER instead of READ/WRITE Use `MEMBER` instead of `READ`/`WRITE` for some properties that are not called explicitly from outside of the class. * Use default member initializers for Fader Use default member initializers for the members in `Fader` that have previously been initialized in the constructor list. * Make code clearer Make code clearer in `Fader::FadermouseDoubleClickEvent`. Only divide if the dialog was accepted with OK.

michaelgregorius added 11 commits December 31, 2023 15:55

Remove unused pixmaps from Fader

dac9208

Remove the now unused pixmaps for the background and the LEDs from the `Fader` class and remove the files from the default and classic theme directories.

Always render the knob in the middle of the fader

c9e83a3

Render the knob in the middle of the fader regardless of the width. The previous implementation was dependent on the fader pixmap having a matching width because it always rendered at x=0.

Set size policy of fader to minimum expanding

ec4830f

Set the size policy of the fader to minimum expanding in both directions. This will make the fader grow in layouts if there is space.

Default dbFS levels and better peak values

8051134

Default to dbFS levels for all faders and set some better minimum and maximum peak values.

Remove the init method from Fader

50781de

Remove the `init` method from `Fader` as it is not needed anymore due to the constructor delegation. Tidy up the parameter lists and use of spaces in the constructor.

Merge remote-tracking branch 'upstream/master' into FaderInCodeWithGr…

0126c02

…adients Conflicts: * include/Fader.h * src/gui/widgets/Fader.cpp

Draw unity lines last

045a2cd

Draw the unity lines last so that they are not affected by the white-ish overdraw in line 423 (after the changes).

michaelgregorius added 5 commits January 6, 2024 17:49

Remove unused method Fader::clips

381257b

Fix whitespace

e4b8b6b

Remove unused methods in PaintHelper

e48bbb8

Remove the now unused mapping methods from `PaintHelper`. Their functionality has been replaced with the usage of `LinearMap` in the code.

Fix some builds

e1fd07f

Include `cassert` for some builds that otherwise fail.

he29-net mentioned this pull request Jan 6, 2024

Mixer LEDs and labels are cropped at 2x scaling #7052

Closed

Opaque unity marker with styling option

99113c3

Make the unity marker opaque by default and enable to style it with the style sheets. None of the two style sheets uses this option though.

Merge remote-tracking branch 'upstream/master' into FaderInCodeWithGr…

6d7e7c4

…adients Conflicts: * plugins/Eq/EqEffect.cpp

michaelgregorius requested a review from Veratil April 2, 2024 19:01

Veratil reviewed Apr 2, 2024

View reviewed changes

michaelgregorius added 6 commits April 3, 2024 11:52

Reduce code repetition

f2d63ec

Reduce code repetition in `EqEffect::setBandPeaks` by introducing a lambda. Adjust code formatting.

Code review changes

2bec339

Code review changes in `Fader.cpp`. Mostly whitespace adjustments. Split up the calculation of the meter width to make it more understandable. This also reduces the number of parentheses.

Move const to the left of types

b3a1dd9

Use MEMBER instead of READ/WRITE

1bd81d8

Use `MEMBER` instead of `READ`/`WRITE` for some properties that are not called explicitly from outside of the class.

Remove constexpr again

a65b8aa

Remove `constexpr` from `ampToDbfs` and ` dbfsToAmp` again because the MSVC compiler does not like it. Seems that for some reason `log10f` and `std::pow` are not `constexpr` there (at least in the version used on the build server).

Veratil reviewed Apr 3, 2024

View reviewed changes

include/Fader.h Outdated Show resolved Hide resolved

plugins/Eq/EqEffect.cpp Outdated Show resolved Hide resolved

plugins/Eq/EqEffect.cpp Outdated Show resolved Hide resolved

plugins/Eq/EqEffect.cpp Outdated Show resolved Hide resolved

michaelgregorius added 4 commits April 3, 2024 22:47

Comprehensive code formatting in Fader class

dea9423

Improved capture and formatting

bb212ee

Merge remote-tracking branch 'upstream/master' into FaderInCodeWithGr…

a74b214

…adients

Make code clearer

b0e4586

Make code clearer in `Fader::FadermouseDoubleClickEvent`. Only divide if the dialog was accepted with OK.

michaelgregorius merged commit ba4fda7 into LMMS:master Apr 5, 2024
9 checks passed

michaelgregorius deleted the FaderInCodeWithGradients branch April 5, 2024 10:50

Rossmaxx mentioned this pull request Apr 13, 2024

Make equalizer leg design dependent on theme #6261

Open

Rossmaxx mentioned this pull request May 9, 2024

Mixer peak indicators. #1271

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scalable consistent faders with themeable gradients, marker at unity, dbFS by default #7045

Scalable consistent faders with themeable gradients, marker at unity, dbFS by default #7045

michaelgregorius commented Jan 2, 2024 •

edited

Loading

he29-net commented Jan 2, 2024

he29-net commented Jan 2, 2024

michaelgregorius commented Jan 3, 2024

michaelgregorius commented Jan 3, 2024

michaelgregorius commented Jan 3, 2024

michaelgregorius commented Jan 6, 2024

michaelgregorius commented Jan 6, 2024

he29-net commented Jan 6, 2024

michaelgregorius commented Jan 6, 2024

he29-net commented Jan 6, 2024

michaelgregorius commented Feb 22, 2024

curlymorphic commented Feb 22, 2024

michaelgregorius commented Feb 24, 2024

michaelgregorius commented Feb 24, 2024

RoxasKH commented Mar 5, 2024

michaelgregorius commented Mar 10, 2024

michaelgregorius commented Mar 10, 2024

michaelgregorius commented Mar 22, 2024

RoxasKH commented Mar 22, 2024

Scalable consistent faders with themeable gradients, marker at unity, dbFS by default #7045

Scalable consistent faders with themeable gradients, marker at unity, dbFS by default #7045

Conversation

michaelgregorius commented Jan 2, 2024 • edited Loading

How does it look/feel?

Detailed overview of the changes

he29-net commented Jan 2, 2024

he29-net commented Jan 2, 2024

michaelgregorius commented Jan 3, 2024

michaelgregorius commented Jan 3, 2024

michaelgregorius commented Jan 3, 2024

michaelgregorius commented Jan 6, 2024

Analysis of the previous pixmap implementation

michaelgregorius commented Jan 6, 2024

he29-net commented Jan 6, 2024

michaelgregorius commented Jan 6, 2024

he29-net commented Jan 6, 2024

michaelgregorius commented Feb 22, 2024

curlymorphic commented Feb 22, 2024

michaelgregorius commented Feb 24, 2024

michaelgregorius commented Feb 24, 2024

RoxasKH commented Mar 5, 2024

michaelgregorius commented Mar 10, 2024

michaelgregorius commented Mar 10, 2024

michaelgregorius commented Mar 22, 2024

RoxasKH commented Mar 22, 2024

michaelgregorius commented Jan 2, 2024 •

edited

Loading