Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
3212bd8
ModelView_adding_error_message_for_model_nullptr
szeli1 Apr 9, 2025
247e348
ModelView_adding_assert_to_model_also
szeli1 Apr 10, 2025
4ba7cbb
AutomatableModel_refactoring_linking_part_1
szeli1 May 9, 2025
74500d8
LapsdaControl_updating_functions
szeli1 May 9, 2025
dba2a5a
Song_updating_automation_functions
szeli1 May 9, 2025
57c24b8
AutomatableModel_refactoring_part_2
szeli1 May 9, 2025
eff1d2a
AutomatableModel_moving_funcion_to_private
szeli1 May 9, 2025
ffaddb1
ModelView_reveting_changes
szeli1 May 9, 2025
f4678e9
AutomatableModel_removing_unused_code
szeli1 May 9, 2025
368d8bf
Song_removing_unused_code
szeli1 May 9, 2025
23c99df
Vestige_replace_setAutomatedValue
szeli1 May 9, 2025
6bb8982
VstEffectControls_replace_setAutomatedValue
szeli1 May 9, 2025
c5f9a04
AutomatableModel_fixing_issues
szeli1 May 9, 2025
e008275
AutomatableModelTest_updating_test
szeli1 May 9, 2025
db3b11a
AutomatatbleModel adding comments and assert
szeli1 May 23, 2025
675993a
edit_unlink adding new svg
szeli1 May 24, 2025
f1f0d04
AutomatableModelView fixing unlink picture
szeli1 May 24, 2025
8ab1bd1
AutomatableModel removing unused function
szeli1 Jun 4, 2025
37400d4
edit_unlink replacing with better image
szeli1 Jun 9, 2025
33fa252
MULTIPLE FILES applying style suggestions
szeli1 Jun 9, 2025
a8a5b3e
AutomatableModel updating while loops
szeli1 Jun 27, 2025
40c231d
AutomatableModel fixing value buffers causing crash
szeli1 Jun 29, 2025
f7c1d79
Remove const_cast and simiplify linking functions
allejok96 Jul 7, 2025
761b385
Clarify for, add assert, revert setUseControllerValue, remove comments
allejok96 Jul 13, 2025
e89ba7d
Add comments about design flaws
allejok96 Jul 18, 2025
71fe1a7
Remove const_cast and simiplify linking functions
szeli1 Aug 5, 2025
3e396ab
Merge branch 'LMMS:master' into fix_model_returning_nullptr
szeli1 Oct 25, 2025
2680d4c
AutomatableModel style fixes
szeli1 Oct 25, 2025
3f2e78e
FloatModelEditorBase fixing format
szeli1 Dec 6, 2025
0558a6b
LinkedModelGroups fixing link order
szeli1 Dec 6, 2025
c53f982
LinkedModelGroups fixing bug
szeli1 Dec 6, 2025
6cfa40c
LinkedModelGroups fixing bug 2
szeli1 Dec 6, 2025
6789da4
AutomatableModel removing emit
szeli1 Dec 21, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Remove const_cast and simiplify linking functions
  • Loading branch information
allejok96 committed Jul 7, 2025
commit f7c1d79e2815ace854223ec34c1c0d352e723e20
43 changes: 26 additions & 17 deletions include/AutomatableModel.h
Original file line number Diff line number Diff line change
Expand Up @@ -149,12 +149,21 @@ class LMMS_EXPORT AutomatableModel : public Model, public JournallingObject
template<class T>
inline T value( int frameOffset = 0 ) const
{
if (m_controllerConnection && m_useControllerValue)
if (m_useControllerValue)
{
// workaround to update linked models
AutomatableModel* thisModel = const_cast<AutomatableModel*>(this);
thisModel->setValue(controllerValue(frameOffset), true);
if (m_controllerConnection)
{
return castValue<T>(controllerValue(frameOffset));
}
for (auto next = m_nextLink; next != this; next = next->m_nextLink)
{
if (next->controllerConnection() && next->useControllerValue())
{
return castValue<T>(fittedValue(next->controllerValue(frameOffset)));
}
Comment thread
szeli1 marked this conversation as resolved.
}
}

return castValue<T>( m_value );
}

Expand Down Expand Up @@ -200,7 +209,7 @@ class LMMS_EXPORT AutomatableModel : public Model, public JournallingObject

void setInitValue( const float value );

void setValue(float value, bool isAutomated = false);
void setValue(const float value, const bool isAutomated = false);

void incValue( int steps )
{
Expand Down Expand Up @@ -237,10 +246,9 @@ class LMMS_EXPORT AutomatableModel : public Model, public JournallingObject
m_centerValue = centerVal;
}

//! link @p m1 and @p m2, let @p m1 take the values of @p m2
static void linkModels( AutomatableModel* m1, AutomatableModel* m2 );
void unlinkAllModels();
//! @return 0 if not connected, never 1, 2 if connected to 1 model
//! link this to @p model, copying the value from @p model
void linkToModel(AutomatableModel* model);
//! @return number of other models linked to this
size_t countLinks() const;

/**
Expand All @@ -264,9 +272,9 @@ class LMMS_EXPORT AutomatableModel : public Model, public JournallingObject

virtual QString displayValue( const float val ) const = 0;

bool hasLinkedModels() const
bool isLinked() const
{
return m_nextLink != nullptr;
return m_nextLink != this;
}

// a way to track changed values in the model and avoid using signals/slots - useful for speed-critical code.
Expand Down Expand Up @@ -306,6 +314,7 @@ class LMMS_EXPORT AutomatableModel : public Model, public JournallingObject

public slots:
virtual void reset();
void unlink();
void unlinkControllerConnection();
void setUseControllerValue(bool b = true);

Expand All @@ -327,7 +336,6 @@ public slots:


private:
void setValueInternal(float value, bool isAutomated);
// dynamicCast implementation
template<class Target>
struct DCastVisitor : public ModelVisitor
Expand Down Expand Up @@ -356,13 +364,14 @@ public slots:
loadSettings( element, "value" );
}

void linkModel(AutomatableModel* model);
void unlinkModel();
void setValueInternal(const float value);

//! linking is stored in a linked list ring
//! @return the model that's `m_nextLink` is `this`
//! @return the model whose `m_nextLink` is `this`,
//! or `this` if there are no linked models
AutomatableModel* getLastLinkedModel() const;
//! @return true if the `model` is in the linked list
bool isModelLinked(AutomatableModel* model) const;
bool isLinkedToModel(AutomatableModel* model) const;

//! @brief Scales @value from linear to logarithmic.
//! Value should be within [0,1]
Expand All @@ -383,7 +392,7 @@ public slots:
float m_centerValue;

bool m_valueChanged;
float m_oldValue; //!< used for interpolation
float m_oldValue; //!< used by valueBuffer for interpolation

// used to determine if step size should be applied strictly (ie. always)
// or only when value set from gui (default)
Expand Down
1 change: 0 additions & 1 deletion include/AutomatableModelView.h
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,6 @@ public slots:
void execConnectionDialog();
void removeConnection();
void editSongGlobalAutomation();
void unlinkAllModels();
void removeSongGlobalAutomation();

private slots:
Expand Down
158 changes: 55 additions & 103 deletions src/core/AutomatableModel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ AutomatableModel::AutomatableModel(
m_valueChanged( false ),
Comment on lines 53 to 55

This comment was marked as off-topic.

m_oldValue(val),
m_hasStrictStepSize( false ),
m_nextLink(nullptr),
m_nextLink(this),
m_controllerConnection( nullptr ),
m_valueBuffer( static_cast<int>( Engine::audioEngine()->framesPerPeriod() ) ),
m_lastUpdatedPeriod( -1 ),
Comment on lines 57 to 61

This comment was marked as off-topic.

Expand All @@ -70,8 +70,7 @@ AutomatableModel::AutomatableModel(

AutomatableModel::~AutomatableModel()
{
// unlink this from anything else
unlinkModel();
unlink();

if (m_controllerConnection)
{
Expand Down Expand Up @@ -290,33 +289,34 @@ void AutomatableModel::loadSettings( const QDomElement& element, const QString&



void AutomatableModel::setValue(float value, bool isAutomated)
void AutomatableModel::setValue(const float value, const bool isAutomated)
{
float newValue = fittedValue(value);
if (newValue == m_value) { emit dataUnchanged(); return; }
if (fittedValue(value) == m_value)
{
// TODO check why we need this signal, is there a better solution?
if (!isAutomated) { emit dataUnchanged(); }
return;
}

if (isAutomated == false) { addJournalCheckPoint(); }
if (!isAutomated) { addJournalCheckPoint(); }

setValueInternal(value, isAutomated);
// set value for this and the other linked widgets
if (hasLinkedModels())
// set value for this and the other linked models
setValueInternal(value);
for (auto model = m_nextLink; model != this; model = model->m_nextLink)
{
AutomatableModel* next = m_nextLink;
while (next != this)
{
next->setValueInternal(value, isAutomated);
next = next->m_nextLink;
}
model->setValueInternal(value);
}
}

void AutomatableModel::setValueInternal(float value, bool isAutomated)



void AutomatableModel::setValueInternal(const float value)
Comment thread
szeli1 marked this conversation as resolved.
{
float oldValue = m_value;
if (isAutomated) { m_value = fittedValue(scaledValue(value)); }
else { m_value = fittedValue(value); }
m_oldValue = m_value;
m_value = fittedValue(value);
Comment thread
messmerd marked this conversation as resolved.

if (oldValue != m_value)
if (m_oldValue != m_value)
{
m_valueChanged = true;
emit dataChanged();
Expand Down Expand Up @@ -427,103 +427,67 @@ float AutomatableModel::fittedValue( float value ) const




void AutomatableModel::linkModel(AutomatableModel* model)
AutomatableModel* AutomatableModel::getLastLinkedModel() const
{
if (model == nullptr || model == this || isModelLinked(model)) { return; }

AutomatableModel* otherEnd = model->getLastLinkedModel();
AutomatableModel* next = m_nextLink == nullptr ? this : m_nextLink;
if (otherEnd == nullptr)
for (auto model = m_nextLink; ; model = model->m_nextLink)
{
// linking other start to our next
model->m_nextLink = next;
// The last model in the circual reference links back to this
Comment thread
szeli1 marked this conversation as resolved.
Outdated
if (model->m_nextLink == this) { return model; }
}
else
{
// linking other end to our next
otherEnd->m_nextLink = next;
}
// linking this to other start
m_nextLink = model;
assert(m_nextLink != this); // if you change the code, be careful to not link to this
}

void AutomatableModel::unlinkModel()
{
if (hasLinkedModels() == false) { return; }

AutomatableModel* end = getLastLinkedModel();
assert(end != nullptr);
end->m_nextLink = end == m_nextLink ? nullptr : m_nextLink;
m_nextLink = nullptr;
}

AutomatableModel* AutomatableModel::getLastLinkedModel() const
{
if (hasLinkedModels() == false) { return nullptr; }
AutomatableModel* output = m_nextLink;
while (output->m_nextLink != this)
{
output = output->m_nextLink;
}
return output;
}

bool AutomatableModel::isModelLinked(AutomatableModel* model) const
bool AutomatableModel::isLinkedToModel(AutomatableModel* model) const
{
if (hasLinkedModels() == false || model == nullptr || model == this) { return false; }
AutomatableModel* next = m_nextLink;
while (next != this)
for (auto next = m_nextLink; ; next = next->m_nextLink)
{
if (next == model) { return true; }
next = next->m_nextLink;
if (next == this) { return false; }
}
return false;
}

size_t AutomatableModel::countLinks() const
{
if (hasLinkedModels() == false) { return 0; }
size_t output = 1;
AutomatableModel* next = m_nextLink;
while (next != this)
size_t output = 0;
for (auto model = m_nextLink; model != this; model = model->m_nextLink)
{
output++;
next = next->m_nextLink;
}
return output;
}





void AutomatableModel::linkModels( AutomatableModel* model1, AutomatableModel* model2 )
void AutomatableModel::linkToModel(AutomatableModel* other)
{
// copy data
model1->setValue(model2->m_value);
if (model1->valueBuffer() && model2->valueBuffer())
if (isLinkedToModel(other)) { return; }

// copy data from other to this
setValue(other->m_value);
if (valueBuffer() && other->valueBuffer())
{
std::copy_n(model2->valueBuffer()->data(),
model1->valueBuffer()->length(),
model1->valueBuffer()->data());
emit model1->dataChanged();
std::copy_n(other->valueBuffer()->data(),
valueBuffer()->length(),
valueBuffer()->data());
emit dataChanged();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Previously this signal was emitted after this if-statement's body, not inside it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The call to setValue() above will emit dataChanged(). I think it will may be safe to remove the emit inside the if-statement since valueBuffer() is only used in the audio processing and that happens frame by frame, not using signals and slots.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Previously this signal was emitted after this if-statement's body, not inside it.

setValue() will emit.

Copy link
Copy Markdown
Member

@messmerd messmerd Dec 10, 2025

Choose a reason for hiding this comment

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

Ok, I see. If the value changes, setValue() will emit a dataChanged() signal for the value change, then if valueBuffer() changes, this line will emit another dataChanged() for the valueBuffer change.

Correct me if I'm wrong, but I believe that's still different from the previous behavior, since now 0, 1, or even 2 signals are emitted rather than just 1 signal.

Here's an idea for how to solve it (assuming it's worth solving):

First, make setValueInternal() return a bool indicating whether the value changed:

-void AutomatableModel::setValueInternal(const float value)
+bool AutomatableModel::setValueInternal(const float value)
{
	m_oldValue = m_value;
	m_value = fittedValue(value);

	if (m_oldValue != m_value)
	{
		m_valueChanged = true;
-		emit dataChanged();
+		return true;
	}
+
+	return false;
}

Then emit the signal from setValue() instead:

	// set value for this and the other linked models
-	setValueInternal(value);
+	const bool valueChanged = setValueInternal(value);
	for (auto model = m_nextLink; model != this; model = model->m_nextLink)
	{
-		model->setValueInternal(value);
+		if (model->setValueInternal(value)) { emit model->dataChanged(); }
	}
+
+	// emit signal *after* all values have been updated (just in case)
+	if (valueChanged) { emit dataChanged(); }

(If there's no point in emitting the signal after the linked models have been updated, you could remove the valueChanged variable and just put an if statement there to emit the signal)

Lastly, this method (linkToModel()) would become:

void AutomatableModel::linkToModel(AutomatableModel* other)
{
	if (isLinkedToModel(other)) { return; }

	addJournalCheckPoint();

	// copy data from other to this
	setValueInternal(other->m_value);
	for (auto model = m_nextLink; model != this; model = model->m_nextLink)
	{
		if (model->setValueInternal(other->m_value)) { emit model->dataChanged(); }
	}

	if (valueBuffer() && other->valueBuffer())
	{
		std::copy_n(other->valueBuffer()->data(),
			valueBuffer()->length(),
			valueBuffer()->data());
	}

	// send dataChanged() before linking (because linking will
	// connect the two dataChanged() signals)
	emit dataChanged();

	// link the models
	AutomatableModel* thisEnd = getLastLinkedModel();
	AutomatableModel* otherEnd = other->getLastLinkedModel();
	thisEnd->m_nextLink = other;
	otherEnd->m_nextLink = this;
}

As you can see, this method is now like a version of setValue() that copies both the value and the valueBuffer, and only emits a single dataChanged() signal after everything (including the valueBuffer) has been copied.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

^^^ @szeli1 What do you think about this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@messmerd while your suggestion seems technically correct, I believe emitting dataChanged for valueBuffer() is useless. Take this very simplified use of valueBuffer()

void AmplifierEffect::processImpl(SampleFrame* outputFrameArray, int frameCount)
{
	auto amplificationBuffer = amplificationModel.valueBuffer();
	for (int f = 0; f < frameCount; ++f)
	{
		outputFrameArray[f] *= amplificationBuffer->value(f);
	}
}

Since the valueBuffer() is a pointer to an array that is updated in-place, if it changes the effect will be instant. There is no need to emit a signal. The signal is used for GUI or other thing that updates once and stays that way. The value buffer is used for real-time things so it doesn't need to signal a change.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you're right. The best option is to just remove the emit dataChanged(); from from this method and rely on setValue() to emit the signal when needed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Done with 6789da4.

}
// link the models
model1->linkModel(model2);
AutomatableModel* thisEnd = getLastLinkedModel();
AutomatableModel* otherEnd = other->getLastLinkedModel();
thisEnd->m_nextLink = other;
otherEnd->m_nextLink = this;
}








void AutomatableModel::unlinkAllModels()
void AutomatableModel::unlink()
{
unlinkModel();
getLastLinkedModel()->m_nextLink = m_nextLink;
m_nextLink = this;
}


Expand All @@ -547,8 +511,6 @@ void AutomatableModel::setControllerConnection( ControllerConnection* c )

float AutomatableModel::controllerValue( int frameOffset ) const
{
if (m_controllerConnection == nullptr) { return m_value; }

float v = 0;
switch (m_scaleType)
{
Expand Down Expand Up @@ -584,6 +546,7 @@ ValueBuffer * AutomatableModel::valueBuffer()

float val = m_value; // make sure our m_value doesn't change midway

// Get value buffer from our controller
if (m_controllerConnection && m_useControllerValue && m_controllerConnection->getController()->isSampleExact())
{
auto vb = m_controllerConnection->valueBuffer();
Expand Down Expand Up @@ -616,13 +579,11 @@ ValueBuffer * AutomatableModel::valueBuffer()
}
}

if (!m_controllerConnection)
// Get value buffer from one of the linked models' controller
if (m_useControllerValue)
{
if (hasLinkedModels())
for (auto next = m_nextLink; next != this; next = next->m_nextLink)
{
AutomatableModel* next = this;
while (true)
{
if (next->controllerConnection() && next->useControllerValue() &&
next->controllerConnection()->getController()->isSampleExact())
{
Expand All @@ -637,12 +598,10 @@ ValueBuffer * AutomatableModel::valueBuffer()
m_hasSampleExactData = true;
return &m_valueBuffer;
}
next = next->m_nextLink;
if (next == this) { break; }
}
}
}

// Populate value buffer by interpolatating between the old and new value
if (m_oldValue != val)
{
m_valueBuffer.interpolate(m_oldValue, val);
Expand Down Expand Up @@ -754,16 +713,9 @@ float AutomatableModel::globalAutomationValueAt( const TimePos& time )

void AutomatableModel::setUseControllerValue(bool b)
{
if (b)
{
m_useControllerValue = true;
emit dataChanged();
}
else if (m_controllerConnection && m_useControllerValue)
{
m_useControllerValue = false;
emit dataChanged();
}
if (m_useControllerValue == b) { return; }
m_useControllerValue = b;
if (m_controllerConnection) { emit dataChanged(); }
}

float FloatModel::getRoundedValue() const
Expand Down
Loading