c++ wrapper set_calc_function also calculates a sample of output

[esluyter]

I just got down to debugging random server hangs in a plugin I built off the c++ wrapper examples, and I discovered that set_calc_function also calculates a sample of output, so calling it first before initializing state variables sometimes results in problems. I feel that either the wrapper should be changed to mimic the behavior of SETCALC, or the examples should be changed to put calls to set_calc_function after initializing state variables, and getting rid of step 3, "calculate one sample of output".

[nhthn]

you're right. i was oblivious to this when i was writing these examples but ran into this problem in this C++ wrapper. i ended up overriding the set_calc_function method and replacing the function call with something that sets all the outputs to 0 (to clear any garbage memory):

    template <typename UnitType, void (UnitType::*PointerToMember)(int)>
    void set_calc_function(void)
    {
        mCalcFunc = make_calc_function<UnitType, PointerToMember>();
        ClearUnitOutputs(this, 1);
    }

my idea would be to overload this method so that it takes an argument, which can be one of the enum values SCUnit::DoNothing, SCUnit::Calc1Sample, SCUnit::ClearOutputs. if no argument is provided, the behavior is identical to SCUnit::Calc1Sample, since we need to maintain backward compatibility.

[michaeldzjap]

Yep. I also just ran into an issue because of this. Happy enough I figured out how it is solvable in my case; i.e. simply not calling the calculation myself in the constructor and making sure set_calc_function() is called after defining all my state variables. Only thing I am not certain about now, is it fine doing the set_calc_function() call last in my constructor or could this cause some other unforeseen errors possibly? So far it seems perfectly fine.

[mtmccrea]

Hi @michaeldzjap

is it fine doing the set_calc_function() call last in my constructor or could this cause some other unforeseen errors possibly?

I raised this issue here: https://github.com/supercollider/supercollider/issues/4075#issuecomment-429496146

It seems OK to use set_calc_function() at the end of your constructor if you're sure you've already initialized your member variables, and your calculation function doesn't change any member variables itself which will be expected to the be the same when the actual first sample is calculated again. The name of the function is misleading, in that it's not explicit about calculating the first sample. There are certain situations where it may be convenient to use your calc function to calculate the first sample, but other situations where you want to set member variable and the first output sample directly without using the calc function.

For this reason I'm in agreement:

I feel that either the wrapper should be changed to mimic the behavior of SETCALC

This would be a breaking change, but TBH I haven't seen signs that this wrapper has been used much at all (@snappizz's example above being the exception), so maybe this could be acceptable after polling users and devs to see if anyone depends on this currently. (I haven't received any responses to my post to the user list a week ago.)

@snappizz, how do you feel about strictly maintaining backwards compatibility on this? This may be a good opportunity to revisit the design more generally of some of these SCUnit wrappers?

[nhthn]

hey, sorry it took me so ridiculously long to get back on this. i was taking a break from SC development. really appreciate your efforts in investigating the problems with this interface, this is stuff that i've thought about for a while and i'm glad to see it discussed.

currently, in core and sc3-plugins, these are all the uses of SCUnit i could find:

• MulAdd in core (which inherits from SIMD_Unit, which inherits from SCUnit)
• K2A in core (also inherits from SIMD_Unit)
• NHHall in sc3-plugins
• NovaDiskIO in sc3-plugins

there could be other third-party ugens using this C++ wrapper that we don't know about, since this repository has helped bring the wrapper to attention. fortunately i recommended the oldschool C wrapper as example 1a and C++ as example 1b, so i don't think i've necessarily spread it to many people aside from present company.

overall, i agree that the amount of damage possible by breaking compatibility is probably not to worry about. removing next(1) gets a thumbs up from me.

i don't think this needs a plugin API version change in core, since SC_PlugIn.hpp is a header only wrapper.

we should try to make this change quickly because we're on the verge of releasing 3.10. i don't want to be in a situation where the SC_PlugIn.hpp in the 3.10 branch has an additional next(1) but the SC_PlugIn.hpp in the develop branch doesn't, which creates hell for anyone developing or compiling a third party plugin. i will run this by other devs this weekend to see what people think.