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

Initialize peak controller last sample with base value #4382

Merged
merged 2 commits into from Jun 2, 2018

Conversation

@DouglasDGI
Copy link
Contributor

@DouglasDGI DouglasDGI commented May 29, 2018

This change makes it so that when an LMMS project is loaded, each knob connected to a Peak Controller will be set to the Peak Controller's Base value, rather than its minimum possible value.

DouglasDGI added 2 commits May 29, 2018
After the change, Peak Controllers will set any connected knobs to their Base value when an LMMS project is loaded, rather than their lowest possible value.
This change makes it so that when an LMMS project is loaded, each knob connected to a Peak Controller will be set to the Peak Controller's Base value, rather than its minimum possible value.
@DouglasDGI DouglasDGI changed the title Bug fix in peak_controller_effect.cpp Initialize peak controller last sample with base value May 29, 2018
@@ -64,7 +64,7 @@ PeakControllerEffect::PeakControllerEffect(
Effect( &peakcontrollereffect_plugin_descriptor, _parent, _key ),
m_effectId( rand() ),
m_peakControls( this ),
m_lastSample( 0 ),
m_lastSample( m_peakControls.m_baseModel.value() ), //sets the value to the Peak Controller's Base value (rather than 0 like in previous versions)

This comment has been minimized.

@Sawuare

Sawuare May 29, 2018
Member

No need for the comment because the variables names' are descriptive enough.

@Wallacoloo Wallacoloo merged commit 0d7ea27 into LMMS:stable-1.2 Jun 2, 2018
2 checks passed
2 checks passed
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Wallacoloo
Copy link
Member

@Wallacoloo Wallacoloo commented Jun 2, 2018

@DouglasDGI Thanks for the bugfix!

I'm going to go ahead and cherry-pick this onto master as well.

Wallacoloo added a commit that referenced this pull request Jun 2, 2018
* Bug fix in peak_controller_effect.cpp

This change makes it so that when an LMMS project is loaded, each knob connected to a Peak Controller will be set to the Peak Controller's Base value, rather than its minimum possible value.
@DomClark
Copy link
Member

@DomClark DomClark commented Sep 25, 2018

This doesn't work because the state of the controls has not been restored at the point where m_lastSample is initialised. The effect of this PR is that instead of initialising the value to 0, it is initialised to 0.5, the default value of the base model. This isn't a bad change, since the variable now has the correct initial value upon creation of a new peak controller, but it isn't the desired change either.

A working approach would be either to set m_lastSample from PeakControllerEffectControls::loadSettings after the settings for m_baseModel are loaded:

void PeakControllerEffectControls::loadSettings( const QDomElement & _this )
{
	m_baseModel.loadSettings( _this, "base" );
	m_effect->m_lastSample = m_baseModel.value();
...

or to override loadSettings in PeakControllerEffect, something along the lines of

void PeakControllerEffect::loadSettings( const QDomElement & _this )
{
	Effect::loadSettings( _this );
	m_lastSample = m_peakControls.m_baseModel.value();
}
@PhysSong
Copy link
Member

@PhysSong PhysSong commented Nov 5, 2018

@DomClark I believe newly created peak controllers still need the old change.

@DomClark
Copy link
Member

@DomClark DomClark commented Nov 25, 2018

I believe newly created peak controllers still need the old change.

I agree, although it seems less important: changes to a new peak controller's controls don't have an effect on its output until some audio goes through it, so while it would give it a correct initial value, as soon as the user changes the base value it would be incorrect again.

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

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.