Fix knob linking / refactor linking #7883
Conversation
|
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. |
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. |
|
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. |
|
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: I also observe this on master. |
|
LCD linking to other LCD items isn't possible. Interestingly, you can link LCDs to knobs. |
|
Thanks for testing @regulus79 and @bratpeki |
Yes sounds like LCD widgets do not accept the link for some reason. |
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. |
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. |
In the current system this is a reasonable argument, but in the next few PRs, I will remove controller connection, the controller dialog and others, and "math controllers" will be added to mash together automation and controllers, I will add a model for every automation clip for now to have the ability to record. |
|
Please revert that. Please. You had fixed a crash and improved a bad linking system. It was so close to being ready to merge. But with those major feature changes we are back to square one. Keep this PR simple if you want to ever have it merged. |
I'm sorry I continued to work on this issue. I got confused by your comment "you'll have to think about how to solve this problem, either now or later, and it's better to do it now before breaking features" and decided to implement a solution instead of "thinking" about it. |
szeli1
force-pushed
the
fix_model_returning_nullptr
branch
from
af533de to
40c231d
Compare
| * Y axis can be set to logarithmic, and automation clips store | ||
| * the actual values, and not the invertedScaledValue. | ||
| */ | ||
| model->setValue(model->scaledValue(it.value()), true); |
There was a problem hiding this comment.
Why was this changed to use scaledValue? Is there some separate bug you're fixing by doing this?
There was a problem hiding this comment.
Prior to this change scaledValue() was called inside setAutomatedValue(). That function has been replaced with setValue(value, isAutomated=true) and it has only three usages:
Song::processAutomationsManageVSTEffectView::syncPluginManageVestigeInstrumentView::syncPlugin
The first one is where the scaledValue() conversion needs to happen due to how automation clips store value of logarithmic models. That is why it's been moved here. It puts the code closer to the problem.
The other two usages doesn't need to scale the value... I think. In any case, they proceed to call setInitValue() which overwrites any scaling that might have taken place. So this change does not change that behavior in any way.
| std::copy_n(other->valueBuffer()->data(), | ||
| valueBuffer()->length(), | ||
| valueBuffer()->data()); | ||
| emit dataChanged(); |
There was a problem hiding this comment.
Previously this signal was emitted after this if-statement's body, not inside it.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Previously this signal was emitted after this if-statement's body, not inside it.
setValue() will emit.
There was a problem hiding this comment.
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.
| using namespace lmms; | ||
|
|
||
| BoolModel m1(false), m2(false); | ||
| BoolModel m1(true), m2(false); |
There was a problem hiding this comment.
Lines where this change matters:
m1.linkToModel(&m2);
QVERIFY(m1.value() == m2.value()); // since m1 takes the value of m2I changed the test, before it required m1 to take m2's value, but now it only requires the two linked values to be the same. This makes the test less restrictive, because the directionality of what model takes the other's value isn't checked.
| m_vi->knobFModel[ i ]->setAutomatedValue( f_value ); | ||
| m_vi->knobFModel[ i ]->setInitValue( f_value ); | ||
| m_vi->knobFModel[i]->setValue(f_value, true); | ||
| m_vi->knobFModel[i]->setInitValue(f_value); |
There was a problem hiding this comment.
What's the actual difference between setValue and setAutomatedValue? The latter has been removed in this PR, but I'm curious what it did relative to just setValue?
| if (model()->useControllerValue() | ||
| && model()->controllerConnection() | ||
| && model()->controllerConnection()->getController()->frequentUpdates() | ||
| && Controller::runningFrames() % (256*4) != 0) |
There was a problem hiding this comment.
Noting the change in behavior, quoting @allejok96 as of here:
There was a problem hiding this comment.
This if statement was converted to an early return.
There was a problem hiding this comment.
@bratpeki the only difference here is the additional check of model()->useControllerValue()
| float AutomatableModel::controllerValue( int frameOffset ) const | ||
| { | ||
| if( m_controllerConnection ) | ||
| assert(m_controllerConnection != nullptr); |
There was a problem hiding this comment.
I'm curious why an assert is used here instead of an if/return. Is there any risk that m_controllerConnection could be nullptr?
8 participants
closes #7869
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:
AutomatableModelsstore a list ofAutomatableModelsthat they are linked to.setValue()and other functions make recursive calls to themself resulting in the Linked controls can cause crashes #7869 crashAutomatableModelcan unlink form otherAutomatableModels, unlinking is the inverse operation to linking.After this PR:
AutomatableModelsstore a pointer to an otherAutomatableModelmaking 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.AutomatableModelcan NOT unlink form otherAutomatableModels, unlinking is NOT the inverse operation to linking.AutomatableModelscan unlink themself from the linked list, they can not unlink themself from single models.Issues:
AutomatableModel::value()uses a hack non constsetValue()to update linked widgets if a controller is connected.How to test: