Individual knob labels rendered using the widget font #7525
Conversation
Enable the knob to render the label correctly at arbitrary sizes if it's configured to do so. Otherwise it will render like before. The used mode is determined when a label is set for the knob because as long as the label is not set a knob does not have one anyway. The painting code now always renders the label with the font that's set for the widget. The are now two methods to set the label text. The new method `setLabelLegacy` renders the label as before albeit in a slightly adjusted implementation. It now sets the widget font to a fixed pixel size font and then calculates the new widget size as before, i.e. not really taking the size of the font into account. This might lead to overlaps if the font of the knob is large. The method `setLabel` now has an additional (temporary) parameter called `legacyMode`. It is by default set to `true` so that all knobs still render like they did before. This is implemented by delegating to `setLabelLegacy` if it's set to `true`. Otherwise the method calculates the new size of the widget by taking the pixmap and the label with the current font into account. Please note that as of now you must set the knob font before calling any of the methods that sets the label. This is because the new size is only calculated via these code paths. However, this is already much better than only being able to use one hard-coded label size for all knobs.
Switch all callers of `setLabel` to `setLabelLegacy` so that it becomes obvious in which places the old knob implementation is used.
Remove the parameter `legacyMode` from `setLabel`. Add the member `m_legacyMode` as it is needed in `Knob::changeEvent` so that we do not switch the behavior when the knob is enabled/disabled.
Extract `setLegacyMode` and `updateFixedSize`. Also add the getter `legacyMode`.
Introduce legacy knob builders and apply them to the plugins. There are three new methods which encapsulate how to create a corresponding legacy knob: * `Knob::buildLegacyKnob` * `CustomTextKnob::buildLegacyKnob` * `TempoSyncKnob::buildLegacyKnob` These methods set the knob they build to legacy mode and also set a label in legacy mode. The idea is to concentrate the relevant legacy code in these methods. They will later also be useful to quickly find all the places in the application where legacy knobs are used. The three methods are applied to the plugins directory. Most plugins use the build methods to build their knobs which also enables the removal of the explicit calls to `setLabelLegacy` from their code. For some plugins their implementations were adjusted so that they can deal with showing the labels in the applicaiton font, i.e. in the font size of the widget their are contained in. Most of the times this involved removing the fixed size and putting the elements in a layout (while also removing move calls). The following LMMS plugins use the application font now and are thus better readable: * Amplifier * BassBooster * Dispersion * Flanger * Peak Controller * ReverbSC * StereoEnhancer Effect The Vectorscope now shows the "Persist." label in the same size as the label of the check boxes. Setting an empty label was removed for Lb302.
Apply the legacy knob builders in the GUI components. Most components use the legacy knobs until they can be redesigned:
* Effect view ("W/D", "DECAY", "GATE")
* LFO Controller
* Instrument window
Everything related to the instrument window is for now kept to use the legacy knobs and should be adjusted at a later point to be more flexible:
* Envelope and LFO
* Functions
* Sound Shaping
The Instrument and sample track both use the legacy knobs for the "VOL" and "PAN" knobs. This might be adjusted later.
The following components now render the labels of their knobs with the application font size:
* MIDI CC Rack
* The class `LadspaControlView`, which is not in used anymore
Some vertical spacing was added to the MIDI CC Rack for slightly improved separation of the elements. The knobs are center aligned in the layout so that the transition between element under and over "CC 100" is cleaner. Setting the models in an explicit loop was removed and is now done when the knobs are created.
## Technical details
Extend `Knob::buildLegacyKnob` with the option to also set the name of the knob. This is needed for some changes in this PR.
The method `KnobControl::setText` now needs to distinguish between legacy mode and non-legacy mode.
Remove `Knob::setLabelLegacy`. Instead make sure that the `Knob` updates its size in the following situations: * The label is set. * The font changes. * Legacy mode is set or unset (already implemented). The handling of font changes has been added to `Knob::changeEvent`. The update in case of a changed label is added to `Knob::setLabel`. Every client that called `setLabelLegacy` now also sets the legacy font in size `SMALL_FONT_SIZE` as this was previously done in `setLabelLegacy`. The label is set via `setLabel` now. Both actions should result in an up-to-date size. The method `KnobControl::setText` now only sets the label via `setLabel`, assuming that the wrapped knob was already configured correctly to either be a legacy knob or not.
Use the descent of the font to calculate the distance of the base line from the bottom of the knob widget if we are not in legacy mode. In legacy mode we still assume the descent to have a value of 2, i.e. the base line will always have a distance of 2 from the bottom of the knob widget regardless of the used font. Also refactor the code a bit to make it more manageable.
Extract the method `Knob::drawLabel` which draws the label. It is called from `paintEvent`.
Use non-legacy knobs for the "VOL" and "PAN" knobs of the instrument and sample track. This gives a bit more separation between the knob and the label but to make this work the font size had to be decreased by one pixel.
Introduce the builder method `buildKnobWithSmallPixelFont` in `Knob` and `TempoSyncKnob`. It creates a non-legacy knob with a small pixel sized font, i.e. it still uses the small font but with a corrected size computation and corrected space between the knob and the label. It is mostly used in places with manual layouts where there's enough space to have the bit of extra space between the knob and the label. The following plugins use these knobs: * Bitcrush * Crossover EQ * Dual Filter * Dynamics Processor * Multitap Echo * Spectrum analyzer * Mallets * Waveshaper * ZynAddSubFx The "IN" and "OUT" label of the Bitcrush plugin use the default fixed font size now because the plugin uses a pixel based layout. Using the point based application font looked off. They are also used in the following component: * Effect view, i.e. the "W/D", "DECAY", "GATE" knobs of an effect * LFO Controller
Use non-legacy knobs with small pixel fonts for the parameter lists of VST instruments and effects. This is accomplished by renaming `CustomTextKnob::buildLegacyKnob` to `buildKnobWithSmallPixelFont` and removing the call to `setLegacyMode`.
Fix the handling of styled knobs which are created in non-legacy mode. Styled knobs do not use pixmaps and have no labels. Their size is set from the outside and they are painted within these limits. Hence we should not compute a new size from a pixmap and/or label in `Knob::updateFixedSize`. This fixes the following plugins: * FreeBoy * Kicker * Monstro * Nescaline * Opulenz * Organic * Sf2 Player * sfxr * SID * SlicerT * Triple * Watsyn * Xpressive The functionality broke with commit defa8c0180e. An alternative would have been to check for a styled knob in the contructor or `initUI` method and to set the legacy flag for these. The best solution would likely be to create an own class for styled knobs and to pull that functionality out of `Knob` because they somewhat clash in their handling.
|
@Rossmaxx, @sakertooth, don't be intimidated by the long blurb. 😅 You will find that most of the changes in the code are making calls to There are only very few places left that use the legacy knob. These could be addressed in further pull requests. It should also be considered to extract the "styled" knob into its own class because it is used and behaves quite differently from the pixmap knob. |
|
IIRC, @LostRobotMusic too has a knob refactor planned for his upcoming Disintegrator/Limiter plugins. So in that case, extraction of styled knobs into it's own class would be a better first step over this PR. I don't wanna understate your efforts. You did put a lot of effort but this knob refactor started from a wrong angle from my perspective. We should have started with a new dynamic knob and work our way towards replacing the existing knob. |
The styled knobs are only used in the context of plugins while the other knobs are used throughout the application. Therefore many places will benefit from these changes.
Let's first improve the existing stuff before thinking about other dynamic knobs which are "in the sky". They can still be implemented once this is merged. I am all for incremental improvements. |
I don't see how my plans to eventually clean up the code in a certain file is even remotely relevant to this PR. Please stop speaking for me. |
In this case, I'm trying to avoid duplication of efforts and also to make the knobs handle new upcoming changes (in this case your limiter/disintegrator) better. I'm not "speaking for you" this time. You did plan a clean up right? Which is what this PR is about.
I missed this point when I typed the previous comment. I don't really understand what the knob changes are even with the verbose description, but I'll try. |
Ok, let's clarify this for good. @LostRobotMusic, did you already start to make large changes to the
Do you understand the "TL;DR" at the top of the description, @Rossmaxx? To give you a specific example. Let's say that during your changes in #7493 someone would have said: "Everything looks fine except the knob labels in plugin 'XYZ' are too large now." In that case your only option would have been to globally decrease the font size in the I'd greatly appreciate if this could be reviewed as I have spent a large amount of the last weekend on this. It makes the GUI more flexible and I doubt that the I already wondered if the description might be intimidating while typing it. However, please just check the code. Much of it consists of repeated adjustments. The main things that need to be understood are the changes in the |
Parameter whitespaces in the builder methods of `Knob`. Use `adjustedToPixelSize` in `InstrumentTrackView` and `SampleTrackView`.
|
Thanks for the code review so far, @Rossmaxx!
Yes, some plugins have been switched to layouts to make them more flexible, i.e. to allow rendering their knob labels in the application font. The plugin windows do not really need to be resizable but their implementation is more flexible now. If we for example decided to change the font sizes later this would be the only thing that's necessary and there would be no need to manually move around widgets. |
No, not in any way. All I said on Discord was that Knob.cpp is a mess and that I'll have to clean it up at some point to add a new styled knob type for two specific plugins. Those plans aren't at all relevant to this PR and will not conflict in any way or shape or form. |
Thanks for the heads up, @LostRobotMusic! By the way, I have also experimented with extracting a |
|
Can we merge this @Rossmaxx? |
|
I can't say anything without testing, and I can't test for 2 more days. I feel like me approving stuff looking at the diff is hurting the codebase more than helping. I would like to invite @messmerd as a second reviewer. |
|
I can see a lot of technical talk. How does one test this? |
|
@bratpeki, at the end of the PR description there is a details element which is labeled "Click for (much) more details (including screenshots)". If you open it you will find a description of the things that have changed. So testing more or less consist of checking in a running application if the changes are okay. It's more or less checking all the place in the application where knobs are used and to see if they look okay. |
Conflicts: * plugins/Vectorscope/VecControlsDialog.cpp
|
Starting work on this! |
|
Two things I've noticed off the bat:
Example of 2:
|
|
Thanks for checking @bratpeki! Here's how the elements look when the font size is decreased to 8 pixels:
Shall I push these changes? By the way, this case perfectly demonstrates the intention of this pull request. With the changes of this PR it is possible to set the font size of the knob's label individually. In |
|
Ah, I see! I think those look pretty good, push that and I'll check it on my system! 😎 |
Decrease the size of the following knob labels to 8 pixels: * "VOL" and "PAN" in the instrument and sample track views * "W/D", "DECAY" and "GATE" in the effect view Technically this is accomplished by introducing `Knob::buildKnobWithFixedPixelFont` and `TempoSyncKnob::buildKnobWithFixedPixelFont`. Both versions of `buildKnobWithSmallPixelFont` now also delegate to the new methods.
|
Testing again! Does the "Delay" plugin feature these changes? I'm mainly asking because the DELAY and FDBK (and maybe even the other two) labels are really close to the knobs, whereas other knobs in this PR have some space between the knob and the label. |
@bratpeki, the "Delay" plugin does not use the new implementation because it is implemented with hard-coded knob positions. Therefore it uses the old rendering and should look unchanged to master. The old implementation is selected by using the If one wanted to use the new flexible knob implementation which can use the system/application font size then the "Delay" plugin would have to be implemented with layouts. A list of elements and place which use the old implementation can be found in the section "Legacy Knobs" in the details fold-out in the PR description. In the code one should be able to find them by searching for the usage of the |
|
Here's how the "Delay" plugin would look with the new implementation, i.e. when the calls to
If I remember correctly I did not go with that option because the "RATE" label now looks closer to the "AMNT" knob than its own knob. However, if wanted I can push the changes so that it looks like above. |
|
Given how all of them feature relevant text below the knob, I'd think this is okay. I'd like to ask @sakertooth and @tresf for opinions on this, though. |
Merge `master` so that conflicts can be resolved. Conflicts: * plugins/Amplifier/AmplifierControlDialog.cpp
Conflicts: * plugins/CrossoverEQ/CrossoverEQControlDialog.cpp
Commit the adjustments that were done to `CrossoverEQControlDialog` which I had forgotten to add after fixing the merge.
Fix the formatting of `CrossoverEQControlDialog` which got messed up after copying the code from the current version on GitHub.
|
@bratpeki, I propose to merge this as is. If something needs to be changed it is simple to do. The gist of it is (applies to
There are two other convenience methods which build a knob with the improved rendering and set its font as follows:
Can you please approve this PR so that it can get merged? |
|
Sure, I'll give it another look soon and approve. |
|
This looks pretty good to me, but I invited @szeli1 to review, just as a sanity check! |
Yes:
The current nightly is:
Looking at it now, it might be nice to make them a bit bigger, although I feel like this is perfectly readable. Asking |
Use `std::max` instead of `qMax`. Remove some unnecessary whitespace.
Make `legacyMode` and `setLegacyMode` protected to ensure that legacy knobs can only be built using the factory method `buildLegacyKnob`. In the long term legacy mode should be removed.
If necessary I can adjust this to the previous implementation by using Please note that there is also another PR though which is intended to adjust the rending of the effect view anyway: #7438. It seems to make use of layouts instead of hard coded positions which is good because the current implementation uses hard coded positions and sizes which is the reason why there is so little space for the knob labels. Side note: as of writing the title "mini fx rewrite without merge conflicts" of the other PR is a bit ironic as the PR has... merge conflicts. 😅 |
Merge upstream/master in hopes to fix the Linux build on GitHub.
|
In that case, it'd be good to stick to the new implementation. Is the space between the knob and the text fixed? Maybe a bigger text and smaller padding would look good, since now, when comparing, I've noticed we've made the text much smaller, maybe causing issues for users with vision problems, at least in terms of the effect stack. |
The space between the knob itself and the label is more or less a function of the font definition. The height of the whole knob widget is calculated as the sum of the height that's needed to draw the knob and the height needed for the text (as of writing see line 226 of It's likely safer to keep it that way because some fonts (especially ones made by amateurs and downloaded from the web) might not report the correct values. So if we decided to put the knob and the text slightly on top of each other they might overlap and "bleed" into each other for "bad" fonts. |
What is this PR about?
Up until now it was only possible to change the size of the knob labels in the actual implementation of
Knob. This means that the changes affected all places where knobs are used and it was not possible to fix a specific instance of a knob. Also, if the font size was increase the label started to "bleed" into the knob because the size of the font was not taken into account when determining the size of theKnobinstance.This pull request enables the
Knobclass to render its label correctly at arbitrary sizes using the font that's set for the widget. This means that it's now also possible to render the labels using the application font and size. The knob can also be set into a legacy mode so that it will render itself like before, i.e. as "buggy" as described above.The left knob in the following image shows how the label rendering looked before this PR if the font size was increased (and it still looks like that in legacy mode):
The middle knob shows a knob with a "regular" font size and the right knob shows rendering at increased font size.
Click for (much) more details (including screenshots)
Knobs using the application font
Several elements now make use of the fact that the knob label can be rendered using the application font.
Plugins
The following plugins were adjusted so that they now use the application font to render their labels:
Other components
The following components now render the labels of their knobs with the application font size:
LadspaControlView(which is not in used anymore though)Non-legacy knobs with small font sizes
There are several places where the implementation of the knobs was switched to the non-legacy mode which gives better separation between the knob and the label. The font size was kept at a small size though. This was mostly done in places with manual layouts where there's enough space to have the bit of extra space between the knob and the label.
In most places this was accomplished by using the builder method
buildKnobWithSmallPixelFontwhich can be found forKnobandTempoSyncKnob.Plugins
The following plugins use these knobs:
Other components
They are also used in the following component:
buildKnobWithSmallPixelFont.Styled knobs
Styled knobs do not use pixmaps and have no labels. Their size is set from the outside and they are painted within these limits. Hence we should not compute a new size from a pixmap and/or label in
Knob::updateFixedSize.This mostly applies to the following plugins: FreeBoy, Kicker, Monstro, Nescaline, Opulenz, Organic, Sf2 Player, sfxr, SID, SlicerT, TripleOscillator, Watsyn, Xpressive.
The best solution would likely be to create an own class for styled knobs and to pull that functionality out of Knob because they somewhat clash in their handling.
Legacy knobs
Everything related to the instrument window for now uses the legacy knobs.
The Carla plugin also uses the legacy knobs as I was not able to test it due to crashes that existed before. The LMMS Delay also uses the legacy knobs because it is rather "crammed" already.
You can find the code that uses legacy knobs if you search for calls to the method
buildLegacyKnob. It's provided byKnob,CustomTextKnobandTempoSyncKnob.Other changes
The Vectorscope now shows the "Persist." label in the same size as the label of the check boxes.
Some vertical spacing was added to the MIDI CC Rack for slightly improved separation of the elements. The knobs are center aligned in the layout so that the transition between element under and over "CC 100" is cleaner. See the screenshot above. Setting the models in an explicit loop was removed and is now done when the knobs are created.
The "IN" and "OUT" label of the Bitcrush plugin use the default fixed font size now because the plugin uses a pixel based layout. Using the point based application font looked off. See the screenshot above for the updated view.
Technical details
Changes in the Knob class
Legacy mode
Add the member
m_legacyModeas it is needed in several places to do the right thing, e.g. in the update of the fixed size or the paining of the label. Add the getterlegacyModeand the settersetLegacyMode.Size updates
Make sure that the Knob updates its size in the following situations:
The handling of font changes has been added to Knob::changeEvent. The update in case of a changed label is added to Knob::setLabel.
Extract the method
updateFixedSize.Paining code
The painting code now always renders the label with the font that's set for the widget.
Extract the method
Knob::drawLabelwhich draws the label. It is called frompaintEvent.Use descent to calculate base line
Use the descent of the font to calculate the distance of the base line from the bottom of the knob widget if we are not in legacy mode. In legacy mode we still assume the descent to have a value of 2, i.e. the base line will always have a distance of 2 from the bottom of the knob widget regardless of the used font.
Other
Setting an empty label was removed for Lb302.