Cycling74 / rnbo.adapter.juce

RNBO JUCE Adapter code
https://rnbo.cycling74.com/
MIT License
2 stars 2 forks source link

Seems there is a regression in parameter/state handling on version 1.2 #2

Open yamadapc opened 10 months ago

yamadapc commented 10 months ago

I have managed to isolate an issue I have with the new changes on https://github.com/yamadapc/rnbo-preset-issues

The test suite on TestProcessorPresets.cpp reproduces the problem and this test-suite passes on v.1.1.2, which I have pushed to a branch.

Everything should be in this repository. You can build/run the test-suite with cmake.

The issue is here:

nlohmann::json stateToRestore = {{"__presetid", "rnbo"},
                                     {"gain", {{"value", 0.8}}}};
REQUIRE(gainAudioProcessor->getCurrentProgram() == -1);

auto initialState = getJSONState(*gainAudioProcessor);
REQUIRE(static_cast<float>(initialState["gain"]["value"]) - 1.0 < 0.001);
REQUIRE(static_cast<float>(rnbo.getParameterValue(gainIndex)) - 1.0 < 0.001);

auto stateToRestoreStr = nlohmann::to_string(stateToRestore);
gainAudioProcessor->setStateInformation(stateToRestoreStr.c_str(), stateToRestoreStr.length());
gainAudioProcessor->processBlock(audioBuffer, midiBuffer);

auto newState = getJSONState(*gainAudioProcessor);
REQUIRE(static_cast<float>(newState["gain"]["value"]) - 0.8 < 0.001); // <- Everything is still fine here

// Now ----->
REQUIRE(static_cast<float>(rnbo.getParameterValue(gainIndex)) - 0.8 < 0.001); // <----- this assertion fails

When restoring state (and in some cases setting presets, but I still have to isolate this issue into this reproduction repository), then the state is properly restored onto:

But it's not restored into:

This might be breaking preset handling for me.

yamadapc commented 10 months ago

After comparing - https://github.com/yamadapc/rnbo-preset-issues/compare/v1.1#diff-844f7832a380add738588b2ea8fcde656fde8b46770b649d010b24a02086bd09L241

I believe the issue is the added "notifying" flag - https://github.com/Cycling74/rnbo.adapter.juce/commit/75fddacfa47d3de0da9d0de8e81daf37e968bd62#diff-dc0af4662f43b55e1484928b2ba6f5c8d1c74818e3bd5a26617d9c2c62415d5f

Defining RNBO_JUCE_PARAM_DEFAULT_NOTIFY=1 fixes the tests on v1.2.0.

I have also confirmed that this bug breaks preset handling (https://github.com/yamadapc/rnbo-preset-issues/commit/21bbbef7bcb2010ed7c97e3f87a68d0129553cf7)

Perhaps the fix would be:

diff --git a/export/rnbo/adapters/juce/RNBO_JuceAudioProcessor.cpp b/export/rnbo/adapters/juce/RNBO_JuceAudioProcessor.cpp
index 0357fc4..6d110f8 100644
--- a/export/rnbo/adapters/juce/RNBO_JuceAudioProcessor.cpp
+++ b/export/rnbo/adapters/juce/RNBO_JuceAudioProcessor.cpp
@@ -243,6 +243,9 @@ void JuceAudioProcessor::handleParameterEvent(const ParameterEvent& event)
            param->setValueNotifyingHost((float)normalizedValue);
            param->endChangeGesture();
        }
+        else {
+            param->setValue((float)normalizedValue);
+        }
    }
 }

When I make this change ^, the test-suite on my repository works.

x37v commented 10 months ago

I think you found a bug but this "fix" shouldn't be needed, there is a bug in the preset handling within rnbo (I have a unit test in the rnbo repo that shows it, not yet in the released code though).

If you look at how parameters work in the JUCE adapter, you'll see that the rnbo parameters get values from the underlying rnbo object, and if presets work correctly, you should be able to query the value of the parameter without doing this round trip update and get the correct value out.

I'll link your issue in our ticket and try to update when we have this resolved. Thanks a ton for reporting it!

yamadapc commented 5 months ago

Please find a reproduction on v8.6.0 attached. Two new tests in that test-suite are failing on assertions over the nº of presets.

x37v commented 5 months ago

the main branch adds an initial preset that resets parameters to their initial value. It is sort of a work around because hosts seem to want to set the initial MIDI program to 0 if it isn't already 0 so I set up 0 to represent the initial state. There is some work in our develop branch of RNBO restoring host saved state on startup.. we're working on a release for that but if you're in the beta program maybe you could test the latest dev RNBO.