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

Fix/pitch shift #166

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Fix/pitch shift #166

wants to merge 4 commits into from

Conversation

stephenhensley
Copy link
Collaborator

This should end up fixing #162

The negative transposition was almost 2x the ratio that it was supposed to. So that none of the semitones were accurate when set below 0. This has been fixed.

Making the phasor accept negative frequency let's us remove any of the weird "is it going up or down" logic, and just have the phasor flip when the ratio is derived from a negative transposition.

This does remove the pre-generated lookup for the semitones. We can replace it with a more robust lut at some point to get this to be a bit more efficient.

Also moved the one global define into the class as a member constexpr

And added a secondary argument (defaulting to false) to lock transpositions to semitones.

@stephenhensley stephenhensley marked this pull request as draft October 1, 2021 23:13
@stephenhensley
Copy link
Collaborator Author

stephenhensley commented Oct 1, 2021

This isn't quite ready. Checked the "fun" parameter, and it is quite fun when tranpsosing up, but when transposing down it just adds a bunch of noise. So we probably need to flip the direction the modulation happens in when transposing below 0

It also does nothing when set to original pitch,

…added some auto-recalc for transposition at/near zero.
@xconverge
Copy link

This would also fix #201

@xconverge
Copy link

@stephenhensley is there more work to be done for this? I wanted to open a PR to templatize or parameterize the buffer size but want it based off of this PR. I have run this PR a decent amount and it seems pretty solid

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

Successfully merging this pull request may close these issues.

2 participants