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. |
include/CustomTextKnob.h
Outdated
| @@ -42,6 +42,8 @@ class LMMS_EXPORT CustomTextKnob : public Knob | |||
|
|
|||
| CustomTextKnob( const Knob& other ) = delete; | |||
|
|
|||
| static CustomTextKnob* buildKnobWithSmallPixelFont(KnobType knob_num, QWidget* parent, const QString& description, const QString& label); | |||
There was a problem hiding this comment.
You don't need to mention "Knob" in the function name. createWithSmallFont might be better.
Or just make this a constructor.
| return; | ||
| } | ||
|
|
||
| QSize pixmapSize = m_knobPixmap ? m_knobPixmap->size() : QSize(0, 0); |
There was a problem hiding this comment.
| QSize pixmapSize = m_knobPixmap ? m_knobPixmap->size() : QSize(0, 0); | |
| assert(m_knobPixmap != nullptr); | |
| const QSize pixmapSize = m_knobPixmap->size(); |
If m_knobPixmap should always be non-null here, it's better to state that assumption rather than hiding a potential logic error by setting the size to (0, 0).
There was a problem hiding this comment.
If it should always be non-null then it should not be a pointer in the first place. Changing this is not part of this PR though. Therefore I deal with this potential case by not using any space for the knob pixmap, i.e. by only taking space for a label into account.
| const auto & description = s_dumpValues.at(1); | ||
|
|
||
| auto knob = CustomTextKnob::buildKnobWithSmallPixelFont(KnobType::Bright26, this, description, description.left(15)); | ||
| knob->setDescription(description + ":" ); |
There was a problem hiding this comment.
| const auto & description = s_dumpValues.at(1); | |
| auto knob = CustomTextKnob::buildKnobWithSmallPixelFont(KnobType::Bright26, this, description, description.left(15)); | |
| knob->setDescription(description + ":" ); | |
| const auto& description = s_dumpValues.at(1); | |
| auto knob = CustomTextKnob::buildKnobWithSmallPixelFont(KnobType::Bright26, this, description, description.left(15)); | |
| knob->setDescription(description + ':'); |
There was a problem hiding this comment.
I don't really see what would be improved by that change.
| const auto & description = s_dumpValues.at(1); | ||
|
|
||
| auto knob = CustomTextKnob::buildKnobWithSmallPixelFont(KnobType::Bright26, widget, description, description.left(15)); | ||
| knob->setDescription(description + ":" ); |
There was a problem hiding this comment.
| const auto & description = s_dumpValues.at(1); | |
| auto knob = CustomTextKnob::buildKnobWithSmallPixelFont(KnobType::Bright26, widget, description, description.left(15)); | |
| knob->setDescription(description + ":" ); | |
| const auto& description = s_dumpValues.at(1); | |
| auto knob = CustomTextKnob::buildKnobWithSmallPixelFont(KnobType::Bright26, widget, description, description.left(15)); | |
| knob->setDescription(description + ':'); |
There was a problem hiding this comment.
I don't really see what would be improved by that change.
Make the code that computes the new fixed size in legacy more readable even if it is just legacy code that's was not touched. Add some code documentation. Other cosmetic changes: * Whitespace adjustments * Remove unused parameter in `paintEvent` * Rename `knob_num` to `knobNum`
Add some documentation which explains what the effects of legacy mode are.
Remove unnecessary dereference. Also remove unncessary code repetition by introducing `currentParamModel`.
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.