lmms icon indicating copy to clipboard operation
lmms copied to clipboard

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

Open michaelgregorius opened this issue 1 year ago • 17 comments

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

https://github.com/LMMS/lmms/assets/9293269/906307b3-045a-4cc1-b7f9-28243de960b6

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: FaderAdjustmentsLargerFadersExample2

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

Look of the LMMS Compressor: FaderAdjustmentsCompressor

Look of the LMMS Equalizer: FaderAdjustmentsEqualizer

Look of the LMMS Delay: FaderAdjustmentsDelay

Look of the LMMS Crossover EQ: FaderAdjustmentsCrossoverEQ

michaelgregorius avatar Jan 02 '24 19:01 michaelgregorius

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. screen

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 avatar Jan 02 '24 23:01 he29-net

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

he29-net avatar Jan 02 '24 23:01 he29-net

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! :smiley:

michaelgregorius avatar Jan 03 '24 15:01 michaelgregorius

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.

michaelgregorius avatar Jan 03 '24 15:01 michaelgregorius

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

michaelgregorius avatar Jan 03 '24 15:01 michaelgregorius

@he29-net, with commit a644bdcd4f8 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: Minus12ToMinus6

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 avatar Jan 06 '24 14:01 michaelgregorius

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:

https://github.com/LMMS/lmms/assets/9293269/f1ee2caf-9e21-4fe8-91f6-f170c0c6a853

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

https://github.com/LMMS/lmms/assets/9293269/524c573d-efc4-477c-a774-446c37ca88b9

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.

michaelgregorius avatar Jan 06 '24 19:01 michaelgregorius

with commit https://github.com/LMMS/lmms/commit/a644bdcd4f8db2a515a039a15fc15c068b91c6b4 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, https://github.com/LMMS/lmms/commit/045a2cd5c3f9956b912100dfd5aeb0ef2ee5cf87 did not really change anything for me. This is how it looks in the mixer: screen

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. IMG_5748_crop (8 white pixels were added in editor)

he29-net avatar Jan 06 '24 20:01 he29-net

Hi @he29-net,

with commit 99113c3b309 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.

michaelgregorius avatar Jan 06 '24 21:01 michaelgregorius

After 99113c3 the unity line is now pixel-perfect. :+1:

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..)

he29-net avatar Jan 06 '24 21:01 he29-net

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

Ok, great. :slightly_smiling_face: By the way, I think that screenshot was the ultimate (sub) pixel peeping. :smile:

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.

Done with commit a44b654f31a.

(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..)

No problem! Next up then: LMMS on an ESP32. :smile:

michaelgregorius avatar Jan 07 '24 09:01 michaelgregorius

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. One even smaller issue: the unity line overlaps with the borders, resulting in bright pixels where they cross

I fully agree with this: i feel like the full gradient doesn't really fit the current LMMS flat style and it also makes it a lot more difficult to spot clipping. The marker is there to help with it, but it's not really pleasant to look at, as it's a sharp gray line over the volume levels and the fact it's larger than the volume levels doesn't help.

Taking a glance at other daws which uses a full-bar approach at showing volume levels like LMMS (instead of a steps one), they all use a mixed approach, with a gradient on the green/yellow portion, but keeping a straight and sudden passage to red when it clips.

While for the marker, they all have it either outside of the mixer bars, or it just stays quietly behind the levels (as it's more aestethic than anything else at that point considering the red easily marks the clipping levels).

Mixers

In a theme i was working on, which is a recolor that kind of follows the color scheme used in the single window mockup presented in #1911 , i did something similar too, although the mockup didn't show how clipping would have behaved, i had the green gradient but kept the yellow and red sections flat, and also added background markers in a similar way to what FL Studio does. (In this specific example i also made the volume bars larger but that's not something to care about here).

immagine

I'm not saying that's the exact path to follow, as i showed lots of daws have a linear gradient between green and yellow. As said by others, too, i appreciate the work to a better scalable UI, but personally i'd still rather keep at least the clipping to be visible through a clear separation of a red portion of the levels, instead of using a full gradient and overlap an indicator on it.

At least this is my opinion

RoxasKH avatar Jan 08 '24 12:01 RoxasKH

Hi @RoxasKH, thanks for your feedback! I really appreciate it. With commit 5627384833a I have move the unity marker in the background so that it gets overdrawn when the levels surpass it. It looks better indeed.

I think having the markers outside might clash with the option for the users to color the mixer channels. In some cases the markers might be barely visible. One option here might be to render them on top of some slightly transparent black rectangle, so that the colors can shine through but the markers (and later values) would still be readable.

When I find the time I will experiment with drawing the clip color as a solid rectangle which starts right at 0 dbFS.

michaelgregorius avatar Jan 08 '24 17:01 michaelgregorius

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

LevelsWithSolidClipRegion

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

michaelgregorius avatar Jan 08 '24 21:01 michaelgregorius

@PhysSong, do you also want to review this PR? I would greatly appreciate if it would be merged it as it brings several improvements.

michaelgregorius avatar Feb 10 '24 13:02 michaelgregorius

Just a doubt. There seems to be a seperate EqFader class which is inherited from the Fader class and from what I've heard, it's difference is only the fact that EqFader renders in dbFs while the mixer fader renders in linear levels. So my question is, can you use the regular fader for eq and get rid of eq fader after this pr?

Rossmaxx avatar Feb 10 '24 15:02 Rossmaxx

Just a doubt. There seems to be a seperate EqFader class which is inherited from the Fader class and from what I've heard, it's difference is only the fact that EqFader renders in dbFs while the mixer fader renders in linear levels. So my question is, can you use the regular fader for eq and get rid of eq fader after this pr?

Hi @Rossmaxx, I guess the Fader class will then need to be extended with another property which governs if the input data is linear or in dbFS. Currently the Fader class assumes linear input data which might also be a problem if the EqFader (and therefore the Fader) is fed levels which are already in dbFS.

michaelgregorius avatar Feb 11 '24 13:02 michaelgregorius

I have checked the inputs that are used to feed the EqFader with peak data. The EqFader is used in the following LMMS plugins:

  • Compressor: In and Out
  • Delay: Out
  • Equalizer: In, Out and six bands

In all "In" and "Out" cases the peaks are computed from the "linear" signal, e.g. from the data in the buffer that serves as the input and output. This is exactly what the underlying Fader class expects.

This leaves us with the peak values of the six bands in the Equalizer plugin. They might be different but I am not really sure about that. The peak values of the bands are all computed via EqEffect::peakBand which contains a dbFS conversion with regards to the energy that's contained in the bins of the FFT. But it also adds some strange transformation before returning the result.

@curlymorphic Can you please advise with regards to the peaks of the bands? What are their values? What's the transformation before the return about?

michaelgregorius avatar Feb 21 '24 20:02 michaelgregorius

I can sure try, although it was a decade ago I wrote this code. Can you confirm what line you are referring to please?

curlymorphic avatar Feb 21 '24 21:02 curlymorphic

Thanks for the quick response, @curlymorphic!

It's mainly about the lines 299 and 304 in the method EqEffect::peakBand.

michaelgregorius avatar Feb 21 '24 21:02 michaelgregorius

I can sure help, line 299, h = 20 * ( log10( *b / fft->getEnergy() ) ); converts the linear power as return by an fft to dBfs. line 300 simply clamps this value, so any value lower than -60 = -60, so the display does not respond to low level signals. This would have been common on old analog mixing desks from the 1960's to maybe 1990's, as -60dB would have been considered the noise floor.

I am having to think about line 304, and git blame shows this has been changed since my commit, but it looks like it is simply converting the dbfs value to a normalised 0.0f < h < 1.0f range for display.

The conversions are required as dB scales are non linear to represent how the ear responds to the change in power levels.

curlymorphic avatar Feb 21 '24 21:02 curlymorphic

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. :wink: 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.

michaelgregorius avatar Feb 22 '24 19:02 michaelgregorius

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.

curlymorphic avatar Feb 22 '24 20:02 curlymorphic

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 449ab02a 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! :slightly_smiling_face:

michaelgregorius avatar Feb 24 '24 13:02 michaelgregorius

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)

michaelgregorius avatar Feb 24 '24 16:02 michaelgregorius

@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

immagine

Now, aside from the sizes of the mixer buses itself

  1. 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

immagine

  1. 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.

immagine

  1. 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.

immagine

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.

immagine

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

RoxasKH avatar Mar 05 '24 17:03 RoxasKH

Thanks for your feedback, @RoxasKH!

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

  • 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:

FadersBranchRoundLevelsInset

michaelgregorius avatar Mar 10 '24 19:03 michaelgregorius

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

michaelgregorius avatar Mar 10 '24 19:03 michaelgregorius

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

Ready for review. :wink:

michaelgregorius avatar Mar 22 '24 17:03 michaelgregorius

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.

RoxasKH avatar Mar 22 '24 20:03 RoxasKH