Tonejs / Tone.js

A Web Audio framework for making interactive music in the browser.
https://tonejs.github.io
MIT License
13.56k stars 990 forks source link

Signal doesn't work with PitchShifter pitch #513

Closed Foaly closed 5 years ago

Foaly commented 5 years ago

I think I discovered a bug in Tone.js. I was trying to get some help on the forum, but I didn't get a response, so I will try again here.

I want to change the pitch of the Pitchshifter over time. Unfortunatelly I am unable to acchive this. It seems to me as if the pitch member of the PitchShifter is not an AudioParam and can thus not be used with Signal. To me this seems like a bug. But I'm not quite sure what to do about it. Is there another way to interpolate the pitch smoothly? Thank you so much in advance!

Here is my code:

  var shifter = new Tone.PitchShift(0).toMaster();

  var pitchShiftSignal = new Tone.Signal(0, Tone.Type.Interval);
  pitchShiftSignal.connect(shifter.pitch);

  var player = new Tone.Player("./sounds/cello/cello-a2.wav").connect(shifter);
  player.loop = true;

  $("#playButton").click(function() {
    player.start();

    pitchShiftSignal.linearRampTo(2, 2);
  });

I'm getting this error:

Error: error connecting to node: 0
TypeError: Argument 1 is not valid for any of the 2-argument overloads of AudioNode.connect.
tambien commented 5 years ago

This is not necessarily a bug, but related to how the PitchShifter is implemented. The pitch attribute is a number and not an AudioParam so it can't be scheduled and connected to in the same way as an AudioParam. I described more fully the reason for this limitation here.

There is currently no easy way around that other than to incrementally update the pitch attribute on a setInterval.

It seems possible to reimplement Tone.Scale with a Tone.Add and Tone.Multiply instead of a WaveShaper which could propagate all the way up to the PitchShifter enabling it to have a signal-rate pitch attribute.

Foaly commented 5 years ago

Thank you for the fast answer and explanation! My javascript is a bit rusty so maybe I am missing something but looking at Scale.js I can only see Add and Multiply being used. So maybe it is possible to change pitch into an AudioParam?

tambien commented 5 years ago

you're right! it's currently implemented with a Multiply and Add, i guess the part that would need to be updated is making max/min signal-rate instead of just numbers.

Foaly commented 5 years ago

Ah thats fantastic news! Do you think it's worthwhile to reopen this ticket? I have done a bit of reading in the code, but I am still not 100% sure I got everything. You refering to the min/max in Scale.js right? If I understand it correctly they influence scale->lfo->pitchshifter. So would it be a simple matter of changing this:

// Scale.js
// line 34
    this._outputMin = Tone.Signal(Tone.defaultArg(outputMin, 0));
// ...
// line 41
    this._outputMax = Tone.Signal(Tone.defaultArg(outputMax, 1));

Would that be the whole fix? Because from what I can see the pitch attribute in PitchShifter (_frequency) is already a Signal. If thats the case I can send a PR if wanted :)

tambien commented 5 years ago

It wouldn't be quite that simple. To get it to be signal-rate, every part of the scaling equation would need to be signal rate:

output = input * (max - min) + min

Currently just the multiply and add are signal rate, and the subtraction is computed only when min/max are adjusted. this line

so the proper way to do this would probably be something like:

max = Tone.Signal()
min = Tone.Signal()
sub = Tone.Subtract()
multiply = Tone.Multiply()
add = Tone.Add()

max.connect(sub)
min.connect(sub, 0, 1)
min.connect(add)
sub.connect(multiply)
input.connect(multiply)
sub.connect(add)

Something like that... it may not be 100% right.

The most straightforward implementation would be probably use the new AudioWorklet, but i'm reticent to include that since support for the new node still isn't there on many browsers and adds latency in browsers that don't support it.

Foaly commented 5 years ago

Cool! Thanks for the detailed explanation again! I agree AudioWorklets would probably be the best way to go, but given their current state of implementation it's too early. So would you say anything speaks against implementing this? Would there be any negative consequences on the rest of the library that depends on Scale? I couldn't find any build instructions on how to package the library into one file. If you could point me to a resource that describes the process I could try to make a PR!

tambien commented 5 years ago

There's some documentation on building, testing and contributing. Hope that helps!