Jan/fix forcing lfo change

[JanWilczek]

No description provided.

[reuk]

The inline comments are all fairly minor, the code looks generally good.

One thing I noticed while testing is that the LFO seems to "remember" its phase during bypass, so when the bypass is disabled the LFO resumes from its last position. I'm not sure if that's desirable behaviour - normally I'd expect bypass to behave as though the plugin is still active in the background, i.e. the LFO phase would continue to update.

[reuk]

Although this matches the JUCE API, I don't think we've done a very good job here. Bool parameters like this (auxiliary flags that modify the "main" behaviour of the function) aren't very readable at the call site. Consider:

lfo.setModulationRate (5.0f, true);

Unless we go and look at the docs, it's not immediately clear what this function will do. Using an enum class ApplySmoothing { no, yes }; might improve readability.

[JanWilczek]

I was not convinced of this change, but after implementing it, I agree 🙂

What I would do in a plugin is to create a separate struct Tremolo::Parameters with all plugin parameters, and possibly a force field. Then there could be just a single setParameters() function and using designated initializers would get the message across, i.e.,

tremolo.setParameters({
  //...
  .force = bypassedAndNotTransitioning
});

This would allow us avoiding the SerializableParameters struct as well.

[reuk]

Looking at this again, maybe this should be named setModulationRateHz so that the units are slightly more obvious

[JanWilczek]

Done.

For more clarity, I'd use the ClosedRangeValue<float> or Frequency classes from my wolfsound_dsp_utils library. Then, you could write

tremolo.setModulationRate(Frequency{p.rate});

or even

tremolo.setModulationRate(10_Hz);

I don't like using floats as parameters in general!

[reuk]

It would be good to describe exactly the kind of behaviours that we're trying to avoid here. If this comment is here to act as a warning ("something subtle is happening here") then, as a reader/maintainer, I'd want to know what behaviours to test when making changes in this area. The same goes for the comment later on in the file.

[JanWilczek]

I've tried to clarify, but it's difficult to explain waveform shapes in words! 😄

[reuk]

Consider instead introducing a new variable with a different name (maybe just offsetPhase), so that phase always refers to the initial parameter value. I expect this will be marginally clearer

[JanWilczek]

Done.

[JanWilczek]

The inline comments are all fairly minor, the code looks generally good.

One thing I noticed while testing is that the LFO seems to "remember" its phase during bypass, so when the bypass is disabled the LFO resumes from its last position. I'm not sure if that's desirable behaviour - normally I'd expect bypass to behave as though the plugin is still active in the background, i.e. the LFO phase would continue to update.

I see three options for the LFO phase on toggling bypass ON:

1. Phase of the LFO “freezes” and after toggling bypass OFF, the LFO picks up exactly where it left off when bypass was turned on
2. Phase resets
3. Phase is continuously updated even if bypass is ON (what you suggested)

Attached is the image of these approaches.

TBH, I asked a fellow sound engineer, because I have no intuition on which approach is right.

[JanWilczek]

The inline comments are all fairly minor, the code looks generally good.

One thing I noticed while testing is that the LFO seems to "remember" its phase during bypass, so when the bypass is disabled the LFO resumes from its last position. I'm not sure if that's desirable behaviour - normally I'd expect bypass to behave as though the plugin is still active in the background, i.e. the LFO phase would continue to update.

I believe this is a minor thing that requires major changes. I have put that on my TODO list. Let's deal with that once all lessons are published.