server plugins: Unify panning behavior of granular ugens #2136

Merged
merged 6 commits into from Aug 23, 2016

Conversation

Projects
None yet
5 participants
@snappizz
Member

snappizz commented May 28, 2016

The pan argument to TGrains has two problems, originally reported for TGrains2 in sc3-plugins. One of them is a documentation issue, which says that it behaves like PanAz. The intended behavior is more like GrainBuf, which behaves like Pan2 when the number of channels is 2, and like PanAz for 3 or more channels.

Furthermore, the math of the pan argument is not a correct emulation of Pan2. Here is a crude way to visualize the problem, by plotting a pan from -2 to 2:

b = Buffer.read(s, Platform.resourceDir +/+ "sounds/a11wlk01.wav");
{ Pan2.ar(SinOsc.ar(440), Line.kr(-2, 2, 1)) }.plot(1.0)
{ TGrains.ar(2, Impulse.ar(100), b, dur: 0.01, pan: Line.kr(-2, 2, 1)) }.plot(1.0)

screen shot 2016-08-23 at 15 07 18

TGrains wraps around at +-1. Pan2 clips.

This fixes both problems. I also couldn't resist fixing a typo in GrainBuf.

server/plugins/DelayUGens.cpp
@@ -6210,7 +6210,7 @@ void TGrains_next(TGrains *unit, int inNumSamples)
if (grain->chan >= (int)numOutputs) grain->chan -= numOutputs;
} else {
grain->chan = 0;
- pan = sc_wrap(pan * 0.5f + 0.5f, 0.f, 1.f);
+ pan = sc_clip(pan * 0.5f + 0.5f, 0.f, 1.f);

This comment has been minimized.

@timblechmann

timblechmann May 28, 2016

Contributor

2-channel should clip, but more than 2 channel should wrap (line 6204)?

it is both inconsistent and may break existing pieces, relying on the behavior. maybe the documentation should reflect the wrap-around?

@timblechmann

timblechmann May 28, 2016

Contributor

2-channel should clip, but more than 2 channel should wrap (line 6204)?

it is both inconsistent and may break existing pieces, relying on the behavior. maybe the documentation should reflect the wrap-around?

This comment has been minimized.

@snappizz

snappizz May 28, 2016

Member
@snappizz

snappizz via email May 28, 2016

Member
@snappizz

This comment has been minimized.

Show comment
Hide comment
@snappizz

snappizz May 28, 2016

Member

The inconsistency is deeper than I thought -- the GrainUGens (GrainIn, GrainFM, GrainSin, GrainBuf) also do not correctly emulate Pan2, since they fold around +-1 instead of wrapping or clipping.

Member

snappizz commented May 28, 2016

The inconsistency is deeper than I thought -- the GrainUGens (GrainIn, GrainFM, GrainSin, GrainBuf) also do not correctly emulate Pan2, since they fold around +-1 instead of wrapping or clipping.

@telephon

This comment has been minimized.

Show comment
Hide comment
@telephon

telephon May 28, 2016

Member

In general, the two models of linear and circular panning are hard to resolve. Maybe someone has an idea what would be the clearest solution?

Member

telephon commented May 28, 2016

In general, the two models of linear and circular panning are hard to resolve. Maybe someone has an idea what would be the clearest solution?

@timblechmann

This comment has been minimized.

Show comment
Hide comment
@timblechmann

timblechmann May 28, 2016

Contributor

in general it is probably a good idea to unify the implementation, but the change should be communicated reasonably well.

cmake uses 'policies', so that the users can explicitly switch between old and new semantics in case of a breaking change. maybe supercollider could print a one-time warning from the class library (which could be silenced via a startup.scd file) for a few versions.

Contributor

timblechmann commented May 28, 2016

in general it is probably a good idea to unify the implementation, but the change should be communicated reasonably well.

cmake uses 'policies', so that the users can explicitly switch between old and new semantics in case of a breaking change. maybe supercollider could print a one-time warning from the class library (which could be silenced via a startup.scd file) for a few versions.

@snappizz

This comment has been minimized.

Show comment
Hide comment
@snappizz

snappizz May 28, 2016

Member

Here's the behavior advertised in the GrainBuf helpfile:

  • pan argument is ignored for 1 channel
  • pan argument works exactly like Pan2 for 2 channels
  • pan argument works exactly like PanAz for 3 or more channels

Even if it's not implemented correctly in any of the ugens in question, I think this is a good convention. (And while we're on the topic of mono, why does TGrains require at least 2 channels?)

Member

snappizz commented May 28, 2016

Here's the behavior advertised in the GrainBuf helpfile:

  • pan argument is ignored for 1 channel
  • pan argument works exactly like Pan2 for 2 channels
  • pan argument works exactly like PanAz for 3 or more channels

Even if it's not implemented correctly in any of the ugens in question, I think this is a good convention. (And while we're on the topic of mono, why does TGrains require at least 2 channels?)

@snappizz

This comment has been minimized.

Show comment
Hide comment
@snappizz

snappizz May 29, 2016

Member
Member

snappizz commented May 29, 2016

@timblechmann

This comment has been minimized.

Show comment
Hide comment
@timblechmann

timblechmann May 29, 2016

Contributor

argument order will break a lot of code. might be better to introduce new classes, which only reorder the arguments. that allows a deprecation/transition period ... fwiw, a few years back there was a discussion about sc4, which could break compatibility and therefore allow fixing bugs/inconsistencies in the API of the class library ...

Contributor

timblechmann commented May 29, 2016

argument order will break a lot of code. might be better to introduce new classes, which only reorder the arguments. that allows a deprecation/transition period ... fwiw, a few years back there was a discussion about sc4, which could break compatibility and therefore allow fixing bugs/inconsistencies in the API of the class library ...

@snappizz

This comment has been minimized.

Show comment
Hide comment
@snappizz

snappizz Jun 27, 2016

Member

I'm going to abandon this and work on another fix that unifies all the granular ugens.

Member

snappizz commented Jun 27, 2016

I'm going to abandon this and work on another fix that unifies all the granular ugens.

@snappizz

This comment has been minimized.

Show comment
Hide comment
@snappizz

snappizz Jul 14, 2016

Member

On second thought, I think this can be salvaged

Member

snappizz commented Jul 14, 2016

On second thought, I think this can be salvaged

@snappizz snappizz reopened this Jul 14, 2016

@snappizz snappizz changed the title from Fix documentation and behavior of TGrains when numChannels = 2 to Unify panning behavior of granular ugens Jul 14, 2016

@snappizz snappizz changed the title from Unify panning behavior of granular ugens to server plugins: Unify panning behavior of granular ugens Jul 14, 2016

@snappizz

This comment has been minimized.

Show comment
Hide comment
@snappizz

snappizz Aug 7, 2016

Member

OK, this is ready to merge (after review of course). All the following ugens should now be working as described in #2230:

  • GrainSin
  • GrainBuf
  • GrainIn
  • GrainFM
  • TGrains

and the docs have been updated as necessary.

Member

snappizz commented Aug 7, 2016

OK, this is ready to merge (after review of course). All the following ugens should now be working as described in #2230:

  • GrainSin
  • GrainBuf
  • GrainIn
  • GrainFM
  • TGrains

and the docs have been updated as necessary.

@crucialfelix

This comment has been minimized.

Show comment
Hide comment
@crucialfelix

crucialfelix Aug 23, 2016

Member

Before:
screen shot 2016-08-23 at 15 07 18

With this fix:
pan2-vs-tgrain

grainfm:
grainfm

grainbuf:
grainbuf

grainsin:
grainsin

Member

crucialfelix commented Aug 23, 2016

Before:
screen shot 2016-08-23 at 15 07 18

With this fix:
pan2-vs-tgrain

grainfm:
grainfm

grainbuf:
grainbuf

grainsin:
grainsin

@crucialfelix

This comment has been minimized.

Show comment
Hide comment
@crucialfelix

crucialfelix Aug 23, 2016

Member

I agree with everybody else here that this is more correct.

This will break somebody's glitch masterpiece. I'm the kind of guy that would've had some noise that I had no idea how it got into that state, and then the update made it normal and I would get fussed about that.

We will make sure it's announced in the breaking changes.

Member

crucialfelix commented Aug 23, 2016

I agree with everybody else here that this is more correct.

This will break somebody's glitch masterpiece. I'm the kind of guy that would've had some noise that I had no idea how it got into that state, and then the update made it normal and I would get fussed about that.

We will make sure it's announced in the breaking changes.

@crucialfelix crucialfelix merged commit aa77ac7 into supercollider:master Aug 23, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@snappizz

This comment has been minimized.

Show comment
Hide comment
@snappizz

snappizz Aug 23, 2016

Member

Thanks for taking the time to check my work. Where are we currently keeping track of all the breaking changes?

Member

snappizz commented Aug 23, 2016

Thanks for taking the time to check my work. Where are we currently keeping track of all the breaking changes?

@snappizz snappizz referenced this pull request in supercollider/sc3-plugins Aug 23, 2016

Merged

BhobUGens: use standard panning behavior for TGrains2, TGrains3 #111

@vivid-synth

This comment has been minimized.

Show comment
Hide comment
@vivid-synth

vivid-synth Sep 12, 2016

Member

@snappizz I don't know but I think that's an important question. We could start a file in this repo, a la Changelog

Member

vivid-synth commented Sep 12, 2016

@snappizz I don't know but I think that's an important question. We could start a file in this repo, a la Changelog

@snappizz

This comment has been minimized.

Show comment
Hide comment
Member

snappizz commented Sep 12, 2016

This link is probably sufficient for tabulating the API changes: https://github.com/supercollider/supercollider/pulls?q=is%3Apr+label%3A%22API+change%22+is%3Aclosed

brianlheim added a commit to brianlheim/supercollider that referenced this pull request Mar 21, 2017

Fix broken TGrains `amp` handling
\#2136 missed a use of TGrains' `amp` parameter that left it unused
in the graph function. This commit adds it back based on where it
previously resided.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment