Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
yamadapc opened this issue Sep 8, 2023 · 4 comments · May be fixed by #4
Open

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

yamadapc opened this issue Sep 8, 2023 · 4 comments · May be fixed by #4
Assignees

Comments

@yamadapc
Copy link

yamadapc commented Sep 8, 2023

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:

  • the JUCE parameter value (absent from this reproduction code)
  • the RNBO state returned by getStateInformation

But it's not restored into:

  • rnbo.getParameterValue

This might be breaking preset handling for me.

@yamadapc
Copy link
Author

yamadapc commented Sep 8, 2023

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

I believe the issue is the added "notifying" flag - 75fddac#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 (yamadapc/rnbo-preset-issues@21bbbef)

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.

yamadapc added a commit to yamadapc/rnbo.adapter.juce that referenced this issue Sep 8, 2023
I'm not sure if preset handling will be totally correct since the
parameters would not be notifying the hosts depending on how they are
called.

`RNBO::JuceAudioProcessor::setCurrentProgram` does not set
`_isSettingPresetAsync` as far as I can tell.

This fixes the test suite referenced in Cycling74#2.

Closes Cycling74#2.

- https://github.com/Cycling74/rnbo.adapter.juce/blob/dd233609adc495828fa9efcf756ab2a155a3bd1d/RNBO_JuceAudioProcessor.cpp#L412-L421
@yamadapc yamadapc linked a pull request Sep 8, 2023 that will close this issue
@x37v x37v self-assigned this Sep 12, 2023
@x37v
Copy link
Contributor

x37v commented Sep 12, 2023

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 added a commit to yamadapc/rnbo-preset-issues that referenced this issue Feb 8, 2024
@yamadapc
Copy link
Author

yamadapc commented Feb 8, 2024

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
Copy link
Contributor

x37v commented Feb 8, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants