SinOsc phase argument fails for values outside +-8pi

[danstowell]

The following two lines simply plot sine-waves with advancing phase - note that they're 36 channels so you'll probably need to maximise the plot windows:

{SinOsc.ar(440, (-36..0))}.plot(0.1);
{SinOsc.ar(440, (0..36))}.plot(0.1);

There should be a continuous progression, but you can see that the pattern stops advancing at about 25 and you get locked phase beyond that.

Narrowing down on the exact point of failure, it's starting to look like the phase-lock happens at about 8pi (25.13) - in the following plot I take a diff between adjacent channels, so the failures are now shown by silent channels:

{ var l = SinOsc.ar(440, (25, 25.01..25.3)); (l.size-1).collect{|i|  l[i]-l[i+1]  } }.plot(0.1);

The threshold is the same in the negative phase direction too.

Varying the freq doesn't change the threshold.

I'm not sure what the fix for this should be, because there's some slightly wizardish stuff happening in phase calculation (see PhaseFrac1() for example). It's possible that we should be wrapping floating-point phase arguments at some point in the code, or maybe the integer table lookup stuff can be made robust to this.

((tested on master 38dfdbf, scsynth, ubuntu))

[telephon]

I remember that JMcC limited the range of the phase when he introduced phase modulation as wave shaping. It is certainly an efficiency issue, but perhaps one modulo operation is cheap enough to justify?

[muellmusik]

Would it make sense to simply document the expected range, and direct users to modulo themselves if needed?

On 13 Apr 2013, at 11:46, Julian Rohrhuber wrote:

I remember that JMcC limited the range of the phase when he introduced phase modulation as wave shaping. It is certainly an efficiency issue, but perhaps one modulo operation is cheap enough to justify?

—
Reply to this email directly or view it on GitHub.

[timblechmann]

cannot check the code atm, but the phase wrapping is not the only problem of the current wave table oscillator implementation. it is also not completely accurate, which can cause issues when combining sinosc and fsinosc for FM or pm synthesis. I have a prototype of a more precise implementation, but it is notably slower than sinosc or osc. iirc something like 30%.
so it might be reasonable to have more than one oscillator implementation, depending on the use case.

[danstowell]

Thanks all for the comments. I would suggest that something needs to be done to patch this problem before the next minor release, and it would either be to put a modulo in, or document the limitation. (Tim's alternative implementation: since tim suggests it as an additional ugen I'd suggest it would be good for it to go into sc3-plugins rather than having multiple implementations in core.)

So the immediate question is: do we add a modulo, or do we document the limitation and tell users to do their own modulo? Any further votes from devs?

[muellmusik]

On 17 Apr 2013, at 08:01, danstowell wrote:

So the immediate question is: do we add a modulo, or do we document the limitation and tell users to do their own modulo? Any further votes from devs?

Repeating myself a bit, but I vote for document and tell. SinOsc is used a lot in 'mass production' situations like additive synthesis, so I think it's good to keep it as cheap as possible. Those modulos will add up, and unless you've got a clever implementation there may not be any performance gain from integrating it.

[jamshark70]

I tend to agree with Scott. I'd be skeptical of slowing things down for everybody because some few might shoot themselves in the foot under some rare circumstance. (And, what's a programming language without a way to shoot yourself in the foot?)

Or at least benchmark the CPU consumption with and without a modulo embedded in SinOsc's c++ code. If that proves a negligible effect on CPU usage with a few thousand oscillators running, then I might be persuaded to change my mind. IMO the onus for that testing should be on those who want the change.

[muellmusik]

It's also consistent with the general policy for 'out of range' arguments in UGens. There have been requests for similar safety checks in other UGens, and we've rejected them for similar reasons.