sensorium / Mozzi

sound synthesis library for Arduino
https://sensorium.github.io/Mozzi/
GNU Lesser General Public License v2.1
1.09k stars 187 forks source link

Should all tables be made "symetrical"? #280

Closed tomcombriat closed 1 week ago

tomcombriat commented 1 month ago

I have be wondering about that for some time and #279 re-triggered this question in my mind, and this issue is to have others' opinions

Most of legacy tables of Mozzi, which are of int8_t have a maximum value of 127 and a minimal value of -128, taking the full range of the type.

Wouldn't it preferable to have them ranging from -127 to 127?

Pros:

Cons:

Note that all the band_limited tables are actually already symmetric.

Any thoughts?

eclab commented 1 month ago

Not a terrible idea. The primary advantage would be if you need to invert the table. I have written about 50 sketches using Mozzi and I can't think of any that would be significantly impacted by this change.

sensorium commented 1 month ago

Sounds like a good thing to try...

tomcombriat commented 1 month ago

Hi again, I am just starting to dive into this and I am actually interested in an external opinion. ~Looking more closely at the tables, a fairly good chunck of them are marked as int8_t type, even though the internal data are uint8_t. Note that Oscil only supports uint8_t as of now… Note also that even the band limited tables I generated for non-aliased sound are also wrongly marked…~ ~I am a bit tempted to rename all of that to make it coherent, but that might have big consequences for users… So what is the best strategy in your opinion?~

~I am probably for option 1, but would like to have other opinions.~

Edit: int8_t is the good type. As point by eclab. My mind completely shift, sorry for that.


Also, I realized that cos tables are mostly describing -cos. Not a very big deal IMO but while at it I could put them back at the correct phase. What do you think?

eclab commented 1 month ago

I am confused by this comment: as far as I know, oscil accepts int8_t, not uint8_t in its constructor. See https://sensorium.github.io/Mozzi/doc/html/class_oscil.html

tomcombriat commented 1 month ago

That's a typo, this should read

only supports uint8_t as of now…

Edited in the OP.

Indeed, that does not concern a lot of these…

tomcombriat commented 1 month ago

@eclab Indeed, I do not know my mind shifted to: "These tables should be uint8_t. "

tomcombriat commented 1 week ago

@eclab Any opinion on #284 ?

eclab commented 1 week ago

I haven't pulled yet but it appears to be correct changes. I'm not sure if doing -127 rather than -128 will matter given that they're 8-bit, but I suppose it's not unreasonable.