Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should all tables be made "symetrical"? #280

Closed
tomcombriat opened this issue Oct 11, 2024 · 8 comments · Fixed by #284
Closed

Should all tables be made "symetrical"? #280

tomcombriat opened this issue Oct 11, 2024 · 8 comments · Fixed by #284

Comments

@tomcombriat
Copy link
Collaborator

tomcombriat commented Oct 11, 2024

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:

  • they would be naturally centered around 0, and not around -0.5
  • negating them (ie, inverting the phase) would be easier (as of now, negating of these poses problems as -(-128) cannot be represented in int8_t, leading to an overflow.

Cons:

  • slightly less range
  • might have a very slight effect on existing sketch (this is probably very mild though).

Note that all the band_limited tables are actually already symmetric.

Any thoughts?

@eclab
Copy link

eclab commented Oct 11, 2024

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
Copy link
Owner

Sounds like a good thing to try...

@tomcombriat
Copy link
Collaborator Author

tomcombriat commented Oct 21, 2024

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?

  • let it be like it is as of now, knowing that uint8_t and int8_t refer to the same thing. They can still be rescaled.
  • changing them all, deprecating the old one with a warning that they might be deleted at some point and pointing to the correct one.

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
Copy link

eclab commented Oct 21, 2024

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
Copy link
Collaborator Author

tomcombriat commented Oct 21, 2024

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
Copy link
Collaborator Author

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

@tomcombriat tomcombriat mentioned this issue Oct 21, 2024
@tomcombriat tomcombriat linked a pull request Oct 21, 2024 that will close this issue
@tomcombriat
Copy link
Collaborator Author

@eclab Any opinion on #284 ?

@eclab
Copy link

eclab commented Nov 11, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants