Effect enable fix#1176
Conversation
|
I know it's kinda unrelated to this PR, but I'm on my way to bed and just noticed a bug on master: Echo FX send knob appears to introduce a non-unity gain even when the knob is set to zero. Toggling the enable switch with send set to zero makes a very noticeable change in the volume, which makes the effect basically unusable. |
Please test #1166 |
| m_dMix(0), | ||
| m_pBuffer(SampleUtil::alloc(MAX_BUFFER_LEN)) { | ||
| m_dMix(0) { | ||
| m_pBuffer1 = std::make_unique<SampleBuffer>(MAX_BUFFER_LEN); |
There was a problem hiding this comment.
BTW, there's no need for these to be allocated -- they can be member variables.
rryan
left a comment
rryan
left a comment
There was a problem hiding this comment.
Thank you LGTM, this is a much cleaner implementation on top of the fixes. It would be nice to have a test so we can be sure this won't break again in the future.
|
I have put |
|
DEBUG_ASSERT doesn't make Travis and AppVeyor tell us something broke though. |
| EffectProcessor::EnableState effectiveEnableState = channel_info.enable_state; | ||
|
|
||
| if (channel_info.enable_state == EffectProcessor::DISABLING) { | ||
| channel_info.enable_state = EffectProcessor::DISABLED; |
There was a problem hiding this comment.
This may be outside the scope of this PR, but temporal effects like echo and reverb should continue to output what is left in their buffers after their enable switch is turned off to have a smooth tail out of the effect. This is really helpful for transitioning between tracks that are much different tempos. To do this, I think the effect needs to be able to handle the disabling process itself and tell the engine when it is disabled.
This is currently possible by turning the send knob (which is linked to the metaknob by default) all the way down, but it would be easier if this could be accomplished just by pressing the enable switch of the effect.
There was a problem hiding this comment.
Mmm.. What is done in music composing sometimes is that the effects run all the time, and the input volume to the effect is controlled.
There was a problem hiding this comment.
Effects should only use the CPU if they need to.
There was a problem hiding this comment.
The issue was discussed almost completely here:
https://bugs.launchpad.net/mixxx/+bug/1481170
There was a problem hiding this comment.
I outlined a general idea at the end of the comments on that bug, but there are details that remain unresolved. Anyway, this PR does not introduce the issue, so there's no need for this PR needs to solve it.
|
The Failed build seems to be a Travis issue, so we could merge now. |
5 participants
This fixes th clicking sound when enabling/disabling effects and when turning to fully dry
https://bugs.launchpad.net/mixxx/+bug/1661781