Fix knob linking / refactor linking #7883

szeli1 · 2025-05-09T15:39:25Z

This PR aims to fix linking bugs and it aims to make linking code faster. In the future I would like to replace controller and automation code with linked models, so it is essential for this feature to work as efficiently as possible.

Before this PR:

AutomatableModels store a list of AutomatableModels that they are linked to.
setValue() and other functions make recursive calls to themself resulting in the Linked controls can cause crashes #7869 crash
Each AutomatableModel can unlink form other AutomatableModels, unlinking is the inverse operation to linking.

After this PR:

AutomatableModels store a pointer to an other AutomatableModel making a linked list. The end is connected to the first element resulting in a "ring"
setValue() and others are now recursion free, the code runs for every linked model, more efficiently than before.
Each AutomatableModel can NOT unlink form other AutomatableModels, unlinking is NOT the inverse operation to linking. AutomatableModels can unlink themself from the linked list, they can not unlink themself from single models.

Issues:

Const AutomatableModel::value() uses a hack non const setValue() to update linked widgets if a controller is connected.
Linked widgets do not clamp each other unlike before this PR.

How to test:

Press ctrl and drag a Knob onto an other Knob. They should now be linked.
Change the first knob's value, the second knob should change.
Click on "remove all linked controls" for the first knob, if done, the knobs should be unlinked.
Repeat these steps and link together combo boxes, mute buttons, "lcd" displays, and anything that can be automated.
Make sure to link different kinds of widgets together.
See if Linked controls can cause crashes #7869 is fixed.

firiox · 2025-05-09T23:00:15Z

Hey Bro! Nice job I was just testint It and I have a problem I think. When you link mixer channels to the volume fader It changes them so fast that It was jumping for o to 4 really fast thought I think a better way to do this would be to divide the volume range by the number of mixers channels so It is more responsive I guess I don't know if thats challenging but I guess so anyway I got ninja and could test it that for each volume step mixer channels got update right so this is very subjective right but I think It'll feel alot better.
Another thing following up the testing I added the mute toggle to the volume and mixer channel link and It got gimmick like the mute toggle gets toggled before the mixer gets it's first value It is kinda strange like it's not the same method for changing 1 value for the mute toggl and the mixer channel kinda feels off Do you understand me? anyway It's a step forward and this is just final tunning I guess. So to be more clear you can do a half step and the mute toggle gets triggered but the mixer channel is still in 0 and then another half step and the mixer channels finally reaches his next value. Weird right? (It is kinda a ninja movement thought but puts me off)

szeli1 · 2025-05-10T09:33:58Z

Another thing following up the testing I added the mute toggle to the volume and mixer channel link and It got gimmick like the mute toggle gets toggled before the mixer gets it's first value It is kinda strange like it's not the same method for changing 1 value for the mute toggl and the mixer channel kinda feels off Do you understand me?

This is because internally everything is stored as a float even the mute buttons, but these internal values do not range from 0 to 1, but from a custom min value to a custom max value. Each widget receives the same value and interprets it with their own min-max. For example the mixer channel lcd widget gets "2" as input and sends it over to the mute button, the mute button ranges from 0 to 1 so it clamps the "2" to "1" and turns on. If you connect a mixer channel lcd widget to a volume knob, the volume knob ranges from 0 to 200 but the lcd widget ranges from 0 to the number of mixer channels, for example 4. This leads to the behavior you are experiencing.

Its feels badly designed, I will see what I can do.

src/core/AutomatableModel.cpp

regulus79 · 2025-05-21T02:49:29Z

I tested this, and it definitely seems to fix the bug.

Also it seems like you also addressed #7621, somehow.

One thing I noticed is that now if two knobs with different ranges are linked (like volume and panning), if you move one knob out of the range of the other knob (like moving panning to negative), it lets you do that and just sets the value of the other knob to within its own bounds (volume doesn't go below 0). However, on master, if you try to turn one knob past the end of a linked knob, it won't let you, it will stop you from turning it. If you try to turn panning below 0, it won't let you. Well, sometimes. Sometimes it would still let you do it if you did some complicated linking between multiple knobs.

Maybe that's a good thing in this PR? It feels like it probably gives users more freedom. Though of course it means if you move the other knob, the first knob will snap back, which might not be desirable idk. But if it's also in master sometimes... idk, this PR feels like a much cleaner system.

bratpeki · 2025-05-22T22:42:08Z

Tried to cause the crash described in the corresponding issue, and couldn't! 🎉

It seems the image for the "Remove all linked controls" is missing and I get this issue:

Error loading icon pixmap "edit-delete": "File not found"

I also observe this on master.

bratpeki · 2025-05-22T22:44:41Z

LCD linking to other LCD items isn't possible. Interestingly, you can link LCDs to knobs.

szeli1 · 2025-05-23T15:48:12Z

Thanks for testing @regulus79 and @bratpeki

szeli1 · 2025-05-23T15:49:16Z

LCD linking to other LCD items isn't possible. Interestingly, you can link LCDs to knobs.

Yes sounds like LCD widgets do not accept the link for some reason.

szeli1 · 2025-05-23T15:52:22Z

Maybe that's a good thing in this PR? It feels like it probably gives users more freedom

I can make it work like in old lmms, but currently linking isn't saved so this new behavior won't change anyone's project and It will add an additional loop for setValue which would make setValue more computationally expensive. This isn't ideal for the next step: removing controller connection and replacing it with linking.

bratpeki · 2025-05-23T16:30:41Z

Error loading icon pixmap "edit-delete": "File not found"
I also observe this on master.

I find this very strange, TBH. Can someome check they're also getting this in the terminal when removing linking in the context menu?

szeli1 · 2025-05-23T17:11:55Z

I find this very strange, TBH. Can someome check they're also getting this in the terminal when removing linking in the context menu?

I got this message also, there is no "edit-delete" file in the theme. I could fix it if you want it, but I'm unsure what kind of picture to use / make.

regulus79 · 2025-05-23T19:08:10Z

I also get that message. edit-delete.png doesn't appear to exist in the default or classic theme, and doing git log -- data/themes/default/edit-delete.png gives nothing, so I'm not sure it existed in the past either.

Also, 1.2.2 doesn't appear to have the icon either, although I don't get any warning in the terminal.

szeli1 · 2025-05-24T17:16:50Z

I added a new svg for unlinking @bratpeki

bratpeki · 2025-05-24T17:28:00Z

Cool! Any idea why the old one was missing? Or maybe it was never there?

bratpeki · 2025-05-24T17:28:22Z

I also get that message. edit-delete.png doesn't appear to exist in the default or classic theme, and doing git log -- data/themes/default/edit-delete.png gives nothing, so I'm not sure it existed in the past either.

Also, 1.2.2 doesn't appear to have the icon either, although I don't get any warning in the terminal.

I see!

AW1534

Tested this quite rigorously, didn't find any issues. Just have a few style nitpicks.

AW1534 · 2025-05-24T18:01:53Z

plugins/Vestige/Vestige.cpp

+			m_vi->knobFModel[ i ]->setValue(f_value, true);
 			m_vi->knobFModel[ i ]->setInitValue( f_value );


Suggested change

m_vi->knobFModel[ i ]->setValue(f_value, true);

m_vi->knobFModel[ i ]->setInitValue( f_value );

m_vi->knobFModel[i]->setValue(f_value, true);

m_vi->knobFModel[i]->setInitValue(f_value);

AW1534 · 2025-05-24T18:02:18Z

plugins/VstEffect/VstEffectControls.cpp

+			m_vi2->knobFModel[ i ]->setValue(f_value, true);
 			m_vi2->knobFModel[ i ]->setInitValue( f_value );


Suggested change

m_vi2->knobFModel[ i ]->setValue(f_value, true);

m_vi2->knobFModel[ i ]->setInitValue( f_value );

m_vi2->knobFModel[i]->setValue(f_value, true);

m_vi2->knobFModel[i]->setInitValue(f_value);

src/core/AutomatableModel.cpp

 	m_range( max - min ),
 	m_centerValue( m_minValue ),
 	m_valueChanged( false ),


src/core/AutomatableModel.cpp

 	m_hasStrictStepSize( false ),
+	m_nextLink(nullptr),
 	m_controllerConnection( nullptr ),
 	m_valueBuffer( static_cast<int>( Engine::audioEngine()->framesPerPeriod() ) ),
 	m_lastUpdatedPeriod( -1 ),


AW1534 · 2025-05-24T18:06:28Z

src/core/AutomatableModel.cpp

-		m_linkedModels.erase( m_linkedModels.end() - 1 );
-	}
+	// unlink this from anything else
+	unlinkModel();

 	if( m_controllerConnection )


Suggested change

if( m_controllerConnection )

if(m_controllerConnection)

src/core/AutomatableModel.cpp

 template<class T> T AutomatableModel::logToLinearScale( T value ) const
 {
 	return castValue<T>( lmms::logToLinearScale( minValue<float>(), maxValue<float>(), static_cast<float>( value ) ) );


AW1534 · 2025-05-24T18:08:40Z

src/gui/AutomatableModelView.cpp

+		menu->addAction( embed::getIconPixmap( "edit_unlink" ),
 							AutomatableModel::tr( "Remove all linked controls" ),


Suggested change

menu->addAction( embed::getIconPixmap( "edit_unlink" ),

AutomatableModel::tr( "Remove all linked controls" ),

menu->addAction(embed::getIconPixmap("edit_unlink"),

AutomatableModel::tr("Remove all linked controls"),

AW1534 · 2025-05-24T18:12:18Z

src/core/AutomatableModel.cpp

 		}
 	}

-	if( m_oldValue != val )
+	if (m_oldValue != val)
 	{
 		m_valueBuffer.interpolate( m_oldValue, val );


Suggested change

m_valueBuffer.interpolate( m_oldValue, val );

m_valueBuffer.interpolate(m_oldValue, val);

AW1534 · 2025-05-24T18:12:51Z

src/core/LadspaControl.cpp

@@ -342,16 +342,18 @@ void LadspaControl::unlinkControls( LadspaControl * _control )
 	switch( m_port->data_type )


Suggested change

switch( m_port->data_type )

switch(m_port->data_type)

regulus79 · 2025-05-25T23:45:48Z

include/AutomatableModel.h

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


What's the difference between setting the automated value via setAutomatedValue() and setValue(..., true)?

szeli1 added 12 commits April 9, 2025 22:18

ModelView_adding_error_message_for_model_nullptr

3212bd8

ModelView_adding_assert_to_model_also

247e348

AutomatableModel_refactoring_linking_part_1

4ba7cbb

LapsdaControl_updating_functions

74500d8

Song_updating_automation_functions

dba2a5a

AutomatableModel_refactoring_part_2

57c24b8

AutomatableModel_moving_funcion_to_private

eff1d2a

ModelView_reveting_changes

ffaddb1

AutomatableModel_removing_unused_code

f4678e9

Song_removing_unused_code

368d8bf

Vestige_replace_setAutomatedValue

23c99df

VstEffectControls_replace_setAutomatedValue

6bb8982

szeli1 changed the title ~~Fix knob linking~~ Fix knob linking / refactor linking May 9, 2025

szeli1 marked this pull request as draft May 9, 2025 18:55

szeli1 added 2 commits May 9, 2025 21:54

AutomatableModel_fixing_issues

c5f9a04

AutomatableModelTest_updating_test

e008275

szeli1 marked this pull request as ready for review May 9, 2025 21:28

regulus79 reviewed May 21, 2025

View reviewed changes

src/core/AutomatableModel.cpp Outdated Show resolved Hide resolved

bratpeki self-assigned this May 22, 2025

AutomatatbleModel adding comments and assert

db3b11a

szeli1 added 2 commits May 24, 2025 19:14

edit_unlink adding new svg

675993a

AutomatableModelView fixing unlink picture

f1f0d04

AW1534 approved these changes May 24, 2025

View reviewed changes

regulus79 reviewed May 25, 2025

View reviewed changes

		m_vi->knobFModel[ i ]->setValue(f_value, true);
		m_vi->knobFModel[ i ]->setInitValue( f_value );

		m_vi2->knobFModel[ i ]->setValue(f_value, true);
		m_vi2->knobFModel[ i ]->setInitValue( f_value );

		menu->addAction( embed::getIconPixmap( "edit_unlink" ),
		AutomatableModel::tr( "Remove all linked controls" ),

	m_valueBuffer.interpolate( m_oldValue, val );
	m_valueBuffer.interpolate(m_oldValue, val);

		@@ -342,16 +342,18 @@ void LadspaControl::unlinkControls( LadspaControl * _control )
		switch( m_port->data_type )

Uh oh!

Fix knob linking / refactor linking #7883

Are you sure you want to change the base?

Fix knob linking / refactor linking #7883

Uh oh!

Conversation

szeli1 commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

firiox commented May 9, 2025

Uh oh!

szeli1 commented May 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

regulus79 commented May 21, 2025

Uh oh!

bratpeki commented May 22, 2025

Uh oh!

bratpeki commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szeli1 commented May 23, 2025

Uh oh!

szeli1 commented May 23, 2025

Uh oh!

szeli1 commented May 23, 2025

Uh oh!

bratpeki commented May 23, 2025

Uh oh!

szeli1 commented May 23, 2025

Uh oh!

regulus79 commented May 23, 2025

Uh oh!

szeli1 commented May 24, 2025

Uh oh!

bratpeki commented May 24, 2025

Uh oh!

bratpeki commented May 24, 2025

Uh oh!

AW1534 left a comment

Choose a reason for hiding this comment

Uh oh!

AW1534 May 24, 2025

Choose a reason for hiding this comment

Uh oh!

AW1534 May 24, 2025

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

AW1534 May 24, 2025

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

AW1534 May 24, 2025

Choose a reason for hiding this comment

Uh oh!

AW1534 May 24, 2025

Choose a reason for hiding this comment

Uh oh!

AW1534 May 24, 2025

Choose a reason for hiding this comment

Uh oh!

regulus79 May 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

szeli1 commented May 9, 2025 •

edited

Loading

szeli1 commented May 10, 2025 •

edited

Loading

bratpeki commented May 22, 2025 •

edited

Loading