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

make mic ducking use strength the same way in Auto & Man mode #2750

Merged
merged 1 commit into from May 6, 2020

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented May 5, 2020

Previously, engine/enginetalkoverducking returned opposite gain values for 'strength' in Auto and Manual mode.

This commit removes the inversion from Auto mode, thus the Strength knob is like a Music volume knob with both modes now, nicely illustrated with reversed knob arc:
image

  • fully left: music muted completely
  • fully right: music volume unchanged

@ronso0 ronso0 added this to the 2.3.0 milestone May 5, 2020
@ronso0 ronso0 requested a review from ywwg May 5, 2020 06:31
@ronso0 ronso0 added this to In Review in 2.3 release May 5, 2020
@ronso0
Copy link
Member Author

ronso0 commented May 5, 2020

My manual tests went well but the engine tests are failing.
Will have to wrap my head around those, or can someone more familiar with those help?

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good to me.
I will have a look into the tests.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, after the suggested changes the tests will probably work again.

src/engine/enginesidechaincompressor.cpp Outdated Show resolved Hide resolved
// If we overshot, clamp.
m_compressRatio = 0;
m_compressRatio = 1;
}
} else if (m_compressRatio < 0) {
// Complain loudly.
qWarning() << "Programming error, below-zero compression detected.";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERIFY_OR_DEBUG_ASSERT and before the condition above.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully I got your suggestion right here

} else {
    VERIFY_OR_DEBUG_ASSERT(m_compressRatio >= 0) {
        qWarning() << "Programming error, below-zero compression detected.";
    }
    if (m_compressRatio < 1) {
        m_compressRatio += m_decayPerFrame * frames;
        if (m_compressRatio > 1) {
            // If we overshot, clamp.
            m_compressRatio = 1;
        }
    }
}

(sorry, I force-pushed)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

local engine tests are all passing now.

src/engine/enginesidechaincompressor.cpp Outdated Show resolved Hide resolved
Previously, engine/enginetalkoverducking returned opposite values
for 'strength' in Auto and Manual mode. This commit removes the
inversion from Auto mode, thus the Strength knob always affects the
music volume in the same way with both modes, much like a Muisc Volume
knob: * fully left: music muted completely
      * fully right: music volume unchanged
@daschuer
Copy link
Member

daschuer commented May 6, 2020

LGTM, Thank you.
However during testing I found some other issues.
I will target them in separate PRs.

@daschuer daschuer merged commit e2b0fc5 into mixxxdj:master May 6, 2020
2.3 release automation moved this from In Review to Done May 6, 2020
@ronso0 ronso0 deleted the mic-ducking-strength branch May 6, 2020 21:43
@mixxxbot mixxxbot mentioned this pull request Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2.3 release
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants