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

LV Mix EQ: Fix pops when enabling #13073

Merged
merged 1 commit into from
May 1, 2024
Merged

LV Mix EQ: Fix pops when enabling #13073

merged 1 commit into from
May 1, 2024

Conversation

daschuer
Copy link
Member

@daschuer daschuer commented Apr 7, 2024

This fixes #13055

@ronso0
Copy link
Member

ronso0 commented Apr 8, 2024

I have almost no clue what's going on here.
Who of @mixxxdj/developers can review this?
Also, some comments would be helpful.

Can this huge commented code block be removed?

@ronso0
Copy link
Member

ronso0 commented Apr 8, 2024

I can not reproduce the bug anymore so it can't even test if this fixes it, sorry 🤷

@daschuer
Copy link
Member Author

daschuer commented Apr 9, 2024

Ok, I can confirm that the pop can only be heared by a chance. I happens only after a toggle cycle of enabling and disabling the EQ as effect.
After the patch the seep slope shown in the bug with mid kill enabled iwhile enabling the effect is ramped.

m_oldLow(1.0),
m_oldMid(1.0),
m_oldLow(0.0),
m_oldMid(0.0),
Copy link
Member Author

Choose a reason for hiding this comment

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

In our LV mix, the high part is carrying the dry stream.
So this is an obvious fix to ramp from dry after initializing.

m_oldLow = 0.0;
m_oldMid = 0.0;
m_oldHigh = 1.0;
m_rampHoldOff = LVMixEQEffectGroupStateConstants::kRampDone;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is that actual fix:

This fixes the missing tracking if the old values after ramping to unity. After the fix it will ramp from unity when enabling again, before it rams from the old knob positions before disabling which may create the pop.

@ronso0
Copy link
Member

ronso0 commented May 1, 2024

I assume it's correct that this is assigned to 2.4.1, please rebase so we can merge it.

@daschuer daschuer changed the base branch from main to 2.4 May 1, 2024 06:57
@daschuer
Copy link
Member Author

daschuer commented May 1, 2024

Done.

@ronso0
Copy link
Member

ronso0 commented May 1, 2024

Thanks for the quick fix!

@ronso0 ronso0 merged commit 1f804ba into mixxxdj:2.4 May 1, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pop when enabling Bessel8 ISO in effect unit
3 participants