Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Mixer volume percentage labels are off by a factor of 100 #2340

Closed
Wallacoloo opened this issue Sep 12, 2015 · 6 comments
Closed

Mixer volume percentage labels are off by a factor of 100 #2340

Wallacoloo opened this issue Sep 12, 2015 · 6 comments
Milestone

Comments

@Wallacoloo
Copy link
Member

See the below screenshot. When I right click on the volume slider for a channel in the mixer, the options like "copy value" all display something like "1.25%" when the channel is really at 125% volume:

lmms-volumepercentages

Edit: The channel send knobs are also bugged in the same way.

I've checked the other areas of the program and found some other discrepencies:

  • The master volume slider at the top of the window does not include the "%" unit in the right-click context menu (though it does include it for the tooltip when editing). That is, volume readouts are just "100", or "85", rather than "85%".
  • The master pitch reports no units in the context menu, though it too includes units in the tooltip. In contrast, the instrument-specific pitch knobs do include units.
@Wallacoloo Wallacoloo added this to the 1.2.0 milestone Sep 12, 2015
@Wallacoloo Wallacoloo changed the title Mixer volume percentages labels are off by a factor of 100 Mixer volume percentage labels are off by a factor of 100 Sep 12, 2015
@Wallacoloo Wallacoloo self-assigned this Sep 12, 2015
@Wallacoloo
Copy link
Member Author

Just starting work on a patch for this.

@Wallacoloo
Copy link
Member Author

This doesn't appear to be the trivial fix I thought it would be.
It appears that the context menus are generated in AutomatableModelView::addDefaultActions, which looks like:

void AutomatableModelView::addDefaultActions( QMenu* menu )
{
    AutomatableModel* model = modelUntyped();
    // ...
    menu->addAction( embed::getIconPixmap( "edit_copy" ),
                    AutomatableModel::tr( "&Copy value (%1%2)" ).
                        arg( model->displayValue( model->value<float>() ) ).
                        arg( m_unit ),
                    model, SLOT( copyValue() ) );
// ...

Model::displayValue looks like:

QString AutomatableModel::displayValue( const float val ) const
{
    switch( m_dataType )
    {
        case Float: return QString::number( castValue<float>( scaledValue( val ) ) );
        case Integer: return QString::number( castValue<int>( scaledValue( val ) ) );
        case Bool: return QString::number( castValue<bool>( scaledValue( val ) ) );
    }
    return "0";
}

And finally, Model::scaledValue is as below:

float AutomatableModel::scaledValue( float value ) const
{
    return m_scaleType == Linear
        ? value
        : logToLinearScale<float>( ( value - minValue<float>() ) / m_range );
}

The Fader's volume model stores its volume as a proportion. It doesn't appear that there currently exists any way to have the view to show that value multiplied by 100.

Curiously, the Knob class is also used to report volume, but seems to have found a way around this issue. From the looks of it, it may actually be storing the model itself in units of percent, but I cannot tell.

@Wallacoloo Wallacoloo removed their assignment Sep 13, 2015
@StCyr
Copy link

StCyr commented Feb 26, 2016

Hello,

AutomatableModel::displayValue is only used here:

cyr@opennms-dev:~/lmms/src$ grep -R displayValue *
core/AutomatableModel.cpp:QString AutomatableModel::displayValue( const float val ) const
gui/widgets/Knob.cpp:           s_textFloat->setText( displayValue() );
gui/widgets/Knob.cpp:   s_textFloat->setText( displayValue() );
gui/widgets/Knob.cpp:   s_textFloat->setText( displayValue() );
gui/widgets/Knob.cpp:QString Knob::displayValue() const
gui/editors/AutomationEditor.cpp:                                               ->displayValue( level[i] );
gui/editors/AutomationEditor.cpp:                                                       ->displayValue( level );
gui/AutomatableModelView.cpp:                                                   arg( model->displayValue( model->initValue<float>() ) ).
gui/AutomatableModelView.cpp:                                                   arg( model->displayValue( model->value<float>() ) ).
gui/AutomatableModelView.cpp:                                                   arg( model->displayValue( AutomatableModel::copiedValue() ) ).

Actually the Knob class uses its own displayValue function. So, in the end, AutomatableModel::displayValue is only used within gui/AutomatableModelView.cpp and gui/editors/AutomationEditor.cpp.

I just made a trivial patch which seems to solve the issue.

diff --git a/src/core/AutomatableModel.cpp b/src/core/AutomatableModel.cpp
index bf56285..8b0a5bb 100644
--- a/src/core/AutomatableModel.cpp
+++ b/src/core/AutomatableModel.cpp
@@ -269,8 +269,8 @@ QString AutomatableModel::displayValue( const float val ) const
 {
        switch( m_dataType )
        {
-               case Float: return QString::number( castValue<float>( scaledValue( val ) ) );
-               case Integer: return QString::number( castValue<int>( scaledValue( val ) ) );
+               case Float: return QString::number( castValue<float>( scaledValue( val ) * 100 ) );
+               case Integer: return QString::number( castValue<int>( scaledValue( val ) * 100 ) );
                case Bool: return QString::number( castValue<bool>( scaledValue( val ) ) );
        }
        return "0";
diff --git a/src/gui/editors/AutomationEditor.cpp b/src/gui/editors/AutomationEditor.cpp
index b719063..675af77 100644
--- a/src/gui/editors/AutomationEditor.cpp
+++ b/src/gui/editors/AutomationEditor.cpp
@@ -1044,7 +1044,7 @@ void AutomationEditor::paintEvent(QPaintEvent * pe )
                        for( int i = 0; i < 2; ++i )
                        {
                                const QString & label = m_pattern->firstObject()
-                                               ->displayValue( level[i] );
+                                               ->displayValue( level[i] /100 );
                                p.setPen( QApplication::palette().color( QPalette::Active,
                                                        QPalette::Shadow ) );
                                p.drawText( 1, y[i] - font_height + 1,
@@ -1072,7 +1072,7 @@ void AutomationEditor::paintEvent(QPaintEvent * pe )
                        for( ; level <= m_topLevel; level += printable )
                        {
                                const QString & label = m_pattern->firstObject()
-                                                       ->displayValue( level );
+                                                       ->displayValue( level / 100 );
                                y = yCoordOfLevel( level );
                                p.setPen( QApplication::palette().color( QPalette::Active,
                                                        QPalette::Shadow ) );

@StCyr
Copy link

StCyr commented Feb 26, 2016

Here's the corresponding PR: #2614

@Umcaruje
Copy link
Member

Bumping to 1.3 as this is not a newly introduced bug and has been around for some time.

DigArtRoks added a commit to DigArtRoks/lmms that referenced this issue Aug 24, 2020
…MS#2340)

Patch the default context menu and take into account variable m_displayConversion to multiply/divide by 100.

Reset action: update the text with initValue multiplied by 100 if required.
Copy action: update the text with value multiplied by 100 if required. Replace the default copy slot by one
which also multiplies by 100 if required.
Paste action: Replace the default paste slot which divides by 100 if required.

Faders in FxMixer show now the correct context. Copying and pasting values between faders or even volume knobs
in tracks shows consistent behavior. Other faders (like in Eq) show the old behavior.
DigArtRoks added a commit to DigArtRoks/lmms that referenced this issue Aug 29, 2020
…MMS#2340)

Add m_conversionFactor to the AutomatableModelView. This factor will be applied to the model->value when displaying
it in the contextmenu of the control for the reset and copy actions. The factor will be applied when copying the value to
the clipboard. When pasting from the clipboard, the value will be divided by the factor.

Remove the model->displayValue() calls when updating the reset/copy/paste action text labels as this gives e.g. in the
Equalizer the wrong action text for the Frequency knobs.

In the Fader class, remove the m_displayConversion variable but rather use the new m_conversionFactor variable.
Rewire the setDisplayConversion() function to set the m_conversionFactor to 1.0 or 100.0.

Faders in FxMixer show now the correct context menu. Copying and pasting values between faders or even volume knobs
in tracks shows consistent behavior. Other faders (like in Eq) show the old behavior.
DigArtRoks added a commit to DigArtRoks/lmms that referenced this issue Sep 11, 2020
…MMS#2340)

Add m_conversionFactor to the AutomatableModelView. This factor will be applied to the model->value when displaying
it in the contextmenu of the control for the reset and copy actions. The factor will be applied when copying the value to
the clipboard. When pasting from the clipboard, the value will be divided by the factor.

Remove the model->displayValue() calls when updating the reset/copy/paste action text labels as this gives e.g. in the
Equalizer the wrong action text for the Frequency knobs.

In the Fader class, remove the m_displayConversion variable but rather use the new m_conversionFactor variable.
Rewire the setDisplayConversion() function to set the m_conversionFactor to 1.0 or 100.0.

Faders in FxMixer show now the correct context menu. Copying and pasting values between faders or even volume knobs
in tracks shows consistent behavior. Other faders (like in Eq) show the old behavior.
@zonkmachine
Copy link
Member

Closed in #5661

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

No branches or pull requests

4 participants