rodio icon indicating copy to clipboard operation
rodio copied to clipboard

`SineWave::new` taking u32 instead of f32

Open AntonHermann opened this issue 6 years ago • 3 comments

Hey there :) is there a particular reason why SineWave::new() takes an u32 when it stores a f32 internally? I'm not that familiar with sound and stuff so I can't estimate, if this minor accuracy loss is even noticeable, but to me there is no particular reason to limit it to u32

AntonHermann avatar Sep 21 '18 12:09 AntonHermann

This is mainly a quirk in SineWave that should eventually be fixed. Unfortunately, fixing it is a breaking change and requires a major version, which is why it isn't straight-forward to do so.

tomaka avatar Oct 11 '18 14:10 tomaka

I'm working on several changes to SineWave right now and will push up a pull request. Including fixing a float overflow issue that made it practically unusable.

mlindner avatar Nov 05 '18 11:11 mlindner

@mlindner Any news on this? I'm writing a program that output music notes for tuning so it would be convenient for me to drop either a table of decimal frequencies or a formula without having to round.

The freq member is private and when used, it's multiplied by PI, so the fact that the value is integer shouldn't affect too much. If the issue is about range/overflow as you said, maybe some clamping and/or warning can at least notify the user when passing crazy values (for negative values, you could silently take the opposite, or clamp to 0).

This conversion:

let value = 2.0 * 3.14159265 * self.freq * self.num_sample as f32 / 48000.0;

makes me wonder if self.freq was meant to be something else as well (it seems the product is already f32 and shouldn't need conversion).

hsandt avatar Dec 12 '20 21:12 hsandt