Add assignment operators in parameter classes

[chrhaase]

Hi!

I've found it useful to add some assignment operators in the parameter types.
As far as I know, it's the only way to avoid having to manually normalise the new value.

With the current setup I need to do this

auto tempo = processor.getState().params.tempo;
tempo->setValueNotifyingHost (tempo->convertTo0to1 (newBpmValue));

which could just be

processor.getState().params.tempo = newBpmValue;

It seemed straightforward to do for the bool and float params, so I've just started with those.
Is this something you'd consider adding?
Or maybe I've overlooked some simpler solution..

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.02%. Comparing base (c06fa77) to head (84ebcaf).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #562   +/-   ##
=======================================
  Coverage   95.02%   95.02%           
=======================================
  Files         309      309           
  Lines       11675    11676    +1     
  Branches     1084     1084           
=======================================
+ Hits        11094    11095    +1     
  Misses        580      580           
  Partials        1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jatinchowdhury18]

Thanks for bringing this up! It's actually something I was thinking about the other day.

At the moment, I typically use chowdsp::ParameterTypeHelpers::setValue() for this kind of thing, but that's definitely not a very "ergonomic" solution.

My only qualm about the operator overloading approach is that I don't really like when the assignment operator contains some "side-effects" beyond just setting a value. That would be the case here since using the assignment operator also triggers the "notifying host" behaviour. I feel like that's important here, since users might "assign" to a parameter from their audio thread, not expecting all the side-effects that follow (that's something I had trouble with when I started using JUCE).

I'm not sure what the right solution is... maybe making the ParameterTypeHelpers::setValue() easier to access would be a start? Or maybe just having a different "assign" function in the parameter classes that are named and documented in such a way that the side-effects are obvious to the user? I'd be curious to hear your ideas!

[chrhaase]

Yes, I hadn't noticed the ParameterTypeHelpers::setValue. That would actaully be sufficient for me right now - I'm only assigning parameters in a few unit tests at the moment.

I do see your point with the assignment operator having side-effects. Not ideal!
I think a named assign function (setParameterValue?) would be useful - I don't have any better ideas.

Just for the fun of it, I played with some template magic to avoid all the boilerplate that would introduce (sorry if I'm using the mixin-term incorrectly):

template<typename ParamType>
struct SetParameterValueMixin
{
    virtual ~SetParameterValueMixin() = default;

    void setParameterValue (ParameterTypeHelpers::ParameterElementType<ParamType> newValue)
    {
        ParameterTypeHelpers::setValue (newValue, static_cast<ParamType&> (*this));
    }
};

obviously, that would mean moving the type helpers to the parameters module, so it's a bit bigger operation. :)

[jatinchowdhury18]

Yeah, having a named assign function is probably the way to go. setParameterValue() sounds fine to me, and we can have some comments explaining the potential side-effects (and maybe a jassert to confirm that the user is calling that function on the message thread?).

I think I'd prefer to keep chowdsp_parameters independent of chowdsp_plugin_state, so maybe the best way to go would be to just rename your current implementations of the assignment operator? That would also avoid all of the fun with templates (for better or for worse :))

[chrhaase]

I think I'd prefer to keep chowdsp_parameters independent of chowdsp_plugin_state

Definitely.

I'm a little unsure about putting an assertion in there. It seems that sendValueNotifyingHost is actually intended to be used on the audio thread as well. That's what I'm getting that from reading the docs for AudioProcessorParameter::Listener. However, there is a lock in AudioProcessorParameter::sendValueChangedMessageToListeners, so I would, like you, also be hesitant to use it on the audio thread.. I guess it's actually safe if you're not adding/removing listeners during processing. What do you make of this?

[jatinchowdhury18]

I'm a little unsure about putting an assertion in there. It seems that sendValueNotifyingHost is actually intended to be used on the audio thread as well. That's what I'm getting that from reading the docs for AudioProcessorParameter::Listener. However, there is a lock in AudioProcessorParameter::sendValueChangedMessageToListeners, so I would, like you, also be hesitant to use it on the audio thread.. I guess it's actually safe if you're not adding/removing listeners during processing. What do you make of this?

Yeah, I agree with your interpretations of both the docs and code... Maybe an assertion isn't necessary, but there should definitely be a comment to explain the dangers of calling the setParameterValue() method from the audio thread.

[chrhaase]

there should definitely be a comment to explain the dangers of calling the setParameterValue() method from the audio thread.

Yes, I agree. What do you think my latest commit? I added a comment simply referring to the docs for sendValueNotifyingHost.

[jatinchowdhury18]

Yes, I agree. What do you think my latest commit? I added a comment simply referring to the docs for sendValueNotifyingHost.

Ah sorry, I hadn't seen that you had a new commit. That looks good to me! Do you think it also makes sense to have this type of method for ChoiceParameter and/or EnumChoiceParameter?

[chrhaase]

No problem!
Could be handy too yes, but maybe just ChoiceParameter? When adding the function to both, CLion says:

Function 'void chowdsp::EnumChoiceParameter<EnumType>::setParameterValue(EnumType newValue)' hides a non-virtual function from class 'chowdsp::ChoiceParameter'

and I guess it's definitely going to be ambigious if you're using a pure enum as that's implicitly convertible to int?

Adding it only to ChoiceParameter you can still do

myEnumParam.setParameterValue (static_cast<int> (myEnumValue));

which is still pretty good.
I've added that in a new commit.

[jatinchowdhury18]

I think having the setter in EnumChoiceParameter should be fine. I think some linters just have a little trouble with it since they don't know what type EnumType will be ahead of time.

I pushed a change just now... if everything looks okay to you, then we should be good to merge this PR!