Mar 022009
 

I stumbled across something I consider to be a bug while creating some custom controls for IPlug. This is not a bug that is going to cause any strange behavior or crashes but rather a slight inefficiency that does not seem to match the intent of the original coder. Let’s take a look.

Here is the original code as found in IControl.cpp from WDL v2009.01.18

void IControl::SetValueFromPlug(double value)
{
    if (mDefaultValue < 0.0) {
        mDefaultValue = mValue = value;
        SetDirty(false);
        Redraw();
    }
    else
        if (mValue != value) {
            mValue = value;
            SetDirty(false);
            Redraw();
        }
}

A first glance over this code looks fairly reasonable. Line 9 takes special action on the default value if it is negative (not sure why that matters). Line 15 is a slight optimization which only sets the new value if it doesn’t match the old value. The double assignment on line 10 can be broken up into two statements leaving our code looking like this (after a bit of additional formatting):

    if (mDefaultValue < 0.0) {
        mDefaultValue = value;
        mValue = value;
        SetDirty(false);
        Redraw();
    }
    else if (mValue != value) {
        mValue = value;
        SetDirty(false);
        Redraw();
    }

Now we can clearly see the similarity between the two cases and maybe the bug is a bit more clear to you. Spotted it yet?

  • Hint 1: We only need to redraw the control if the value has changed.
  • Hint 2: We only need to set the value if the value has changed.

That’s right, even though we only need to do those two things if the value has changed we are doing both of them every time, regardless of whether the value has changed. With that in mind, let’s refactor.

void IControl::SetValueFromPlug(double value)
{
    if (mDefaultValue < 0.0) {
        mDefaultValue = value;
    }

    if (mValue != value) {
        mValue = value;
        SetDirty(false);
        Redraw();
    }
}

By using two discrete if statements we are able to keep our redraws to a minimum and maintain our default value. As mentioned at the beginning this is not a huge bug. It isn't a behavior or crash but it is a performance bug. An attempted performance improvement resulted in a failed optimization which actually worsens performance by forcing an extra redraw when the default is negative but the value is being set to its old value.

Finding and fixing small bugs like these is not a glamorous task, but VST plugin development is enough fun to make it worth slogging through!

  3 Responses to “IPlug: Bug in SetValueFromPlug”

  1. SetValueFromUserInput is

    if (mValue != value) {
    mValue = value;
    SetDirty();
    Redraw();
    }

    It’s redundancy… optimation? …For not putting SetValueFromUserInput inline?
    (Qurius)

  2. I consider this to be a conceptual issue. “SetFromPlug” and “SetFromGui” are two bits of functionality that should be kept separate. I would not consider calling one from the other. The “proper” solution in my mind if I was worried about this duplication of code would be to extract it into a commonly used inline function or macro. You get into even more trouble when you consider the virtual nature of the function and the fact it may be overridden by any derived control. Calling “fromGui” from “fromPlug” sets up an unnatural dependency between these two functions that could not only be broken by derived controls but also by future updates to the core api which you do not directly control and makes no promise of continued dependence between them.

  3. Oh my!! Should check the function names before jumping into premature conclusions. :)
    Fanx allot, your answer is total clear!

    Hmmm. Can call the specific IControl function (e.g. IControl::SetValueFromUserInput(…) ) inside SetValueFromUserInput to avoid virtual calls.

    Anyway, that does not change anything. Still gives unnatural/unstable dependencies like you said.

Sorry, the comment form is closed at this time.