FR: beginChangeGesture/endChangeGesture improvement

Feature Requests
1

AudioProcessorParameter::beginChangeGesture is using isPerformingGesture flag to detect (in debug build) if beginChangeGesture has already been called and asserts if it has happened. This works fine in most cases.

I suggest to replace that flag with a counter that is incremented in beginChangeGesture and decremented in endChangeGesture. begin/end messages would only be sent when the counter is one.

This would allow multiple components (or MIDI routing) to control the same parameter at the same time without asserts (and extranous host-plugin messages). This can easily occur ie. when a parameter is adjusted at the same time from eg. a slider and external MIDI.

Example scenario:

Slider mouse down -> beginChangeGesture
MIDI input -> beginChangeGesture -> setValueNotifyingHost -> endChangeGesture
Slider mouse up -> endChangeGesture

With the current JUCE implementation we get an assert when we receive MIDI assigned to the parameter on the second line.

  • Jussi
2 
    diff --git a/modules/juce_audio_processors/processors/juce_AudioProcessor.cpp b/modules/juce_audio_processors/processors/juce_AudioProcessor.cpp
index e6aeae5a4..5bba7fe92 100644
--- a/modules/juce_audio_processors/processors/juce_AudioProcessor.cpp
+++ b/modules/juce_audio_processors/processors/juce_AudioProcessor.cpp
@@ -1452,11 +1452,9 @@ AudioProcessorParameter::AudioProcessorParameter() noexcept {}
 
 AudioProcessorParameter::~AudioProcessorParameter()
 {
-   #if JUCE_DEBUG && ! JUCE_DISABLE_AUDIOPROCESSOR_BEGIN_END_GESTURE_CHECKING
     // This will fail if you've called beginChangeGesture() without having made
     // a corresponding call to endChangeGesture...
-    jassert (! isPerformingGesture);
-   #endif
+    jassert (gestureCount.get() == 0);
 }
 
 void AudioProcessorParameter::setValueNotifyingHost (float newValue)
@@ -1469,14 +1467,13 @@ void AudioProcessorParameter::beginChangeGesture()
 {
     // This method can't be used until the parameter has been attached to a processor!
     jassert (processor != nullptr && parameterIndex >= 0);
+    
+    jassert(gestureCount.get() >= 0);
 
-   #if JUCE_DEBUG && ! JUCE_DISABLE_AUDIOPROCESSOR_BEGIN_END_GESTURE_CHECKING
-    // This means you've called beginChangeGesture twice in succession without
-    // a matching call to endChangeGesture. That might be fine in most hosts,
-    // but it would be better to avoid doing it.
-    jassert (! isPerformingGesture);
-    isPerformingGesture = true;
-   #endif
+    ++gestureCount;
+   
+    if (gestureCount.get() > 1)
+        return;
 
     ScopedLock lock (listenerLock);
 
@@ -1499,13 +1496,12 @@ void AudioProcessorParameter::endChangeGesture()
     // This method can't be used until the parameter has been attached to a processor!
     jassert (processor != nullptr && parameterIndex >= 0);
 
-   #if JUCE_DEBUG && ! JUCE_DISABLE_AUDIOPROCESSOR_BEGIN_END_GESTURE_CHECKING
-    // This means you've called endChangeGesture without having previously
-    // called beginChangeGesture. That might be fine in most hosts, but it
-    // would be better to keep the calls matched correctly.
-    jassert (isPerformingGesture);
-    isPerformingGesture = false;
-   #endif
+    jassert (gestureCount.get() > 0);
+
+    --gestureCount;
+
+    if (gestureCount.get() > 0)
+        return;
 
     ScopedLock lock (listenerLock);
 
diff --git a/modules/juce_audio_processors/processors/juce_AudioProcessorParameter.h b/modules/juce_audio_processors/processors/juce_AudioProcessorParameter.h
index f661dcb19..c373da7c1 100644
--- a/modules/juce_audio_processors/processors/juce_AudioProcessorParameter.h
+++ b/modules/juce_audio_processors/processors/juce_AudioProcessorParameter.h
@@ -292,10 +292,7 @@ private:
     CriticalSection listenerLock;
     Array<Listener*> listeners;
     mutable StringArray valueStrings;
-
-   #if JUCE_DEBUG
-    bool isPerformingGesture = false;
-   #endif
+    juce::Atomic<int> gestureCount { 0 };
 
     JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (AudioProcessorParameter)
 };
1 Like
3

What’s with all those CTRL+M at the end of each line?

4

Thanks! Fixed now.

5

Interesting, I just ran into this as well, why hasn’t @JussiNeuralDSP 's suggestion made it into JUCE? It seems reasonable to me.

6

I’ve got two concerns with this suggestion

  1. If I understand correctly the assertion will no longer be able to catch the main issue it is trying to catch at the earliest opportunity (during beginGesture). This will likely mean it’s harder to debug the issue at hand.

  2. The more concerning issue however, is the change in behaviour for listeners. This change proposes that listeners only receive the first begin gesture and last end gesture callback but that could be a breaking change for some people and I don’t think there would be anyway to restore that behaviour with this change.

Another way to deal with this is to enable JUCE_DISABLE_AUDIOPROCESSOR_BEGIN_END_GESTURE_CHECKING and create a custom Listener class that can wrap up other listeners to add the required asserts and only call the actual listener when the first begin / last end gesture is called. For example maybe something like this? (untested)


class ParameterListener final : public juce::AudioProcessorParameter::Listener 
{
public:
    ParameterListener (juce::AudioProcessorParameter::Listener& l) 
        : listener (&l) 
    {}

    ~ParameterListener() final
    {
        jassert (numGestures == 0);
    }

    void parameterGestureChanged (int parameterIndex, bool gestureIsStarting) final
    {
        numGestures += gestureIsStarting ? 1 : -1;
        jassert (numGestures >= 0);

        if (numGestures == 0 || numGestures == 1)
            listener->parameterGestureChanged (parameterIndex, gestureIsStarting);
    }

    void parameterValueChanged (int parameterIndex, float newValue) final
    {
        listener->parameterValueChanged (parameterIndex, newValue);
    }

private:
    juce::AudioProcessorParameter::Listener* listener;
    std::atomic<int> numGestures;
}

Would something like this work?

7

IMHO I still think the initial solution is worth a breaking change.

Suggestion #1: Yes you are right, but I think its the lesser problem
Suggestion #2: Yes, but semantically parameterGestureChanged implies that the gesture has “changed”, so if do beginGesture twice, the gesture doesn’t “change”, or?

It think its a common problem (I have it too) and it should be “solved” in the main juce source tree.

8

Maybe but it’s the whole point of the assertions and there is already a way to switch them off. Is it common to hit these asserts when debugging a typical plugin? if so why? Doesn’t it require the parameter to be manipulated by two sources at once? That seems to me something that probably doesn’t happen often?

I don’t completely follow. I agree parameterGestureChanged implies that the gesture has “changed” and I agree that beginGesture being called twice doesn’t imply a change. But it does tell you that something else now wants to potentially change the parameter.

When you say it’s a common problem, what do you mean? to encounter the assertion? or that you have to add some logic in to ignore multiple begin/end gesture calls? If the later what problem is the multiple begin/end gestures causing?

I’m not asking these questions to be a pain I just want to understand what the issue is.

At the moment I’m 50/50 on moving the assertion but I’m not yet convinced that we should change anything about the listeners, particularly because if we do there is then no way to replicate the old behaviour for someone that wants/needs it.

9

In the situation where you are dragging a parameter value with the mouse, and the DAW is simultaneously sending MIDI automation to that same parameter - it is very counter-intuitive and frustrating to have the experience of ‘fighting’ the automation. i.e. the slider is jittering back and forward between two competing sources of events.

IMHO the correct behaviour here is that when you ‘grab’ a parameter via the mouse-down event, it overrides other sources of automation (the slider refuses to move unless the mouse moves it) until the mouse is released again.

It would be a mistake to sanction undesirable behaviour (jittering back and forth) by modifying JUCE to accept it as ‘normal’.

2 Likes