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

Fix issue #3339 by not clearing filter history on control change #3343

Merged
merged 1 commit into from Mar 8, 2017

Conversation

Projects
None yet
2 participants
@michaelgregorius
Contributor

michaelgregorius commented Feb 10, 2017

Do not clear the filter histories when the crossover control has changed,
e.g. via automation.

Add a new method CrossoverEQEffect::clearFilterHistories that's called
whenever the filter histories need to be cleared, e.g. after loading a
crossover EQ. It would be beneficial to also call this method when the
effect is enabled again after being disabled but it seems there is no
was to find out that this event has happened. One could implement it in
the process method by storing the current state in a member and
comparing it to the state at the time of the last process call but this
is something that should be provided by the framework.

Fix issue #3339 by not clearing filter history on control change
Do not clear the filter histories when the crossover control has changed,
e.g. via automation.

Add a new method CrossoverEQEffect::clearFilterHistories that's called
whenever the filter histories need to be cleared, e.g. after loading a
crossover EQ. It would be beneficial to also call this method when the
effect is enabled again after being disabled but it seems there is no
was to find out that this event has happened. One could implement it in
the process method by storing the current state in a member and
comparing it to the state at the time of the last process call but this
is something that should be provided by the framework.
@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Feb 21, 2017

Member

Testing this with an Automation Track. It solves the problem when I use Linear and Cubic Hermite progression but creates new glitches on Discrete progression.

Member

zonkmachine commented Feb 21, 2017

Testing this with an Automation Track. It solves the problem when I use Linear and Cubic Hermite progression but creates new glitches on Discrete progression.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Feb 21, 2017

Member

Interestingly it's only on a positive flank, when the automation jumps from a lower to a higher value, that I get a glitch.

Member

zonkmachine commented Feb 21, 2017

Interestingly it's only on a positive flank, when the automation jumps from a lower to a higher value, that I get a glitch.

@michaelgregorius

This comment has been minimized.

Show comment
Hide comment
@michaelgregorius

michaelgregorius Feb 21, 2017

Contributor

I think that the pops are created because the parameter changes are not smoothed at all. So instead of interpreting the incoming parameter value directly they'd need to go through a simple 1 pole low pass like the one described here:
http://www.musicdsp.org/archive.php?classid=3#257

However, that might be a more general problem and is something for another issue. So I propose to merge my changes as a first improvement and perhaps add a new issue that describes the problem and that might also be used to investigate whether similar problems show up in other areas as well.

Contributor

michaelgregorius commented Feb 21, 2017

I think that the pops are created because the parameter changes are not smoothed at all. So instead of interpreting the incoming parameter value directly they'd need to go through a simple 1 pole low pass like the one described here:
http://www.musicdsp.org/archive.php?classid=3#257

However, that might be a more general problem and is something for another issue. So I propose to merge my changes as a first improvement and perhaps add a new issue that describes the problem and that might also be used to investigate whether similar problems show up in other areas as well.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Feb 21, 2017

Member

So I propose to merge my changes as a first improvement and perhaps add a new issue that describes the problem and that might also be used to investigate whether similar problems show up in other areas as well.

Sounds good to me. 👍

Any guess as to why I only see this on a rising signal? I've seen pops before but have never noticed this behaviour. It could simply be that I just haven't thought about it but since the behaviour is systematic it could be that we do something wrong here. It works the same way with an LFO square wave.

Member

zonkmachine commented Feb 21, 2017

So I propose to merge my changes as a first improvement and perhaps add a new issue that describes the problem and that might also be used to investigate whether similar problems show up in other areas as well.

Sounds good to me. 👍

Any guess as to why I only see this on a rising signal? I've seen pops before but have never noticed this behaviour. It could simply be that I just haven't thought about it but since the behaviour is systematic it could be that we do something wrong here. It works the same way with an LFO square wave.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Mar 6, 2017

Member

Merge?

Member

zonkmachine commented Mar 6, 2017

Merge?

@michaelgregorius

This comment has been minimized.

Show comment
Hide comment
@michaelgregorius

michaelgregorius Mar 8, 2017

Contributor

@zonkmachine, I'd say let's merge it. It's not perfect but already better than before.

Contributor

michaelgregorius commented Mar 8, 2017

@zonkmachine, I'd say let's merge it. It's not perfect but already better than before.

@zonkmachine zonkmachine merged commit fe881de into LMMS:master Mar 8, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Mar 8, 2017

Member

However, that might be a more general problem and is something for another issue. So I propose to merge my changes as a first improvement and perhaps add a new issue that describes the problem and that might also be used to investigate whether similar problems show up in other areas as well.

Will you open this new issue? Close #3339 ?

Member

zonkmachine commented Mar 8, 2017

However, that might be a more general problem and is something for another issue. So I propose to merge my changes as a first improvement and perhaps add a new issue that describes the problem and that might also be used to investigate whether similar problems show up in other areas as well.

Will you open this new issue? Close #3339 ?

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Mar 13, 2017

Member

Will you open this new issue? Close #3339 ?

Fixed! New issue here: #3421

Member

zonkmachine commented Mar 13, 2017

Will you open this new issue? Close #3339 ?

Fixed! New issue here: #3421

@michaelgregorius

This comment has been minimized.

Show comment
Hide comment
@michaelgregorius

michaelgregorius Mar 17, 2017

Contributor

Thanks for the merge and the creation of the new issue, @zonkmachine!

Contributor

michaelgregorius commented Mar 17, 2017

Thanks for the merge and the creation of the new issue, @zonkmachine!

@michaelgregorius michaelgregorius deleted the michaelgregorius:3339-Crossover-EQ-Pops branch Mar 17, 2017

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