Add optional normalization parameter to createPeriodicWave #545
Conversation
This fixes WebAudio#91 by adding the optional normalization parameter to createPeriodicWave. We also describe the basic waveform generation and the normalization process.
…icWave o Add optional normalization parameter to createPeriodicWave. o Define how normalization is done. o Define the waveform generation better.
|
This seems like it exhibits two antipatterns:
May I suggest |
|
That's a good point. However, we might have to consider the different name than Perhaps we should do |
|
Yeah, I thought denormalized might not be right. And yeah, it would be cleanest to change the default. Otherwise you have to come up with an inverted name, which can be awkward for words without natural antonyms. |
|
I am opposed to changing the default normalization. |
|
Right, I didn't meant to imply you should actually make a backward-incompatible change, just that that would be "easiest" from a naming perspective. Coming up with an inverted name seems most straightforward. |
|
Ok. I don't have any strong opinions either way on the other suggestions. I'd even be ok with createUnnormalizedPeriodicWave(r, i). |
I like it when this is the case, but does it matter if the default is true in cases where the inverted name would be awkward? |
Yes, I think it's better to have a slightly-awkward name than to violate the expectation that a boolean defaults to false. |
|
FWIW, it wouldn't be the first dictionary member to default to true, and the existing cases in canvas don't seem terrible to me: Of course, it would depend on what the actual suggestion is, and if there are actual usability problems with boolean dictionary members defaulting to true, not just a style issue. |
|
If we write the Fourier series in complex form, I think this basically works out to be the same as solving for the roots of an order N polynomial with complex coefficients. Doing the inverse transform and finding the max is much easier to do and doesn't require finding and writing a good polynomial root solver, especially since the order of the polynomial could be as high as 2048 or more. |
|
If there is no reason why a PeriodicWave must be used with a
particular AudioContext, then would it make sense to instead
provide a PeriodicWave constructor?
The new constructor could then have the desired
default-no-normalization behaviour, and the createPeriodicWave()
factory method could be retained as long as legacy support is
required.
Or is a factory method preferred to reduce pollution of the global
namespace?
|
|
I can second the idea of having PeriodicWave detached from the context, but: var p1 = new PeriodicWave({ normalization: false });
var p2 = context.createPeriodicWave({ normalization: true });This doesn't look great in my opinion. Probably we really need to try hard to come up with a clever name. |
|
How about we just name the parameter disableNormalization? |
|
+1 A bit verbose, but I can live with that. |
|
The key of the dict, you mean ? Or just the parameter in the prototype ? |
|
Just the parameter. I don't foresee other items we would want to add to the dict, so a dict seems like overkill. |
|
No. Use a dict. The Boolean trap is real. |
|
Ugh. Ok. |
|
Dealing with the normalization problem on PeriodicWave is good if
we can assume that normalization of built-in oscillator types has
no effect because they are already normalized.
However, it wouldn't solve the problem with built-in oscillator
types if they are going to be based on a similar normalization
using a magnitude from an arbitrary Fourier truncation of the
specified waveform. If that is the intention, then a solution is
required on OscillatorNode instead of PeriodicWave.
#447
Normalization of built-in waves is not required because they are
already normalized, so I hope we don't need to solve that problem.
|
|
I don't quite follow. But in any case, the waveforms for oscillators are always normalized, and to be consistent with PeriodicWaves, the normalization must be consistent with the normalization of PeriodicWaves. We are not allowing oscillators the option of not being normalized. If a user wants that, they can use a periodic wave without normalization. |
|
Please take another look. I've updated the spec to use a dictionary. |
| the <code>real</code> and <code>imag</code> parameters represent | ||
| <em>relative</em> values. | ||
| <em>relative</em> values. If normalization is disabled via the <code>normalize</code> |
There was a problem hiding this comment.
s/normalize/disableNormalization?
|
LGTM, thank you so much for taking the time to make this API ergonomic :) |
Add optional normalization parameter to createPeriodicWave
|
Yep, looks good. |
|
@domenic Is there any method in the other Web APIs that uses a JavaScript object (dict) as an optional descriptor? What if How should we handle the third argument? |
|
Yes, I suggest searching for the keyword "dictionary" in your favorite spec (e.g. DOM or HTML) and you will find plenty of examples. From: Hongchan Choi notifications@github.com @domenichttps://github.com/domenic Is there any method in the other Web APIs that uses a JavaScript object (dict) as an optional descriptor? What if new Float32Array(1) is passed as an argument? For example: context.createPeriodicWave(new Float32Array(5), new Float32Array(5), new Float32Array(5)); How should we handle the third argument? — |
|
@domenic Thanks! It helped. |
#2046 is reconsidering this. |
Fix #91
This adds the optional normalization parameter to createPeriodicWave, defines how normalization is done and how the time-domain waveform can be created from the Fourier coefficients.