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

adding pitch bend support #82

Merged
merged 1 commit into from
Nov 12, 2019
Merged

adding pitch bend support #82

merged 1 commit into from
Nov 12, 2019

Conversation

motifn
Copy link
Contributor

@motifn motifn commented Jul 30, 2019

Adding pitch bend support. In order to compile, I had to alter the midi-file declaration file too.

Notes:

  1. The pitch bend value is passed through with no translation. Perhaps you want to perform some translation, and/or doc-comment on legal range of values.
  2. Obviously the version of the package needs to be updated

@tambien
Copy link
Contributor

tambien commented Aug 2, 2019

@motifn looking through the code. everything is looking good. Is it possible to include a validation test or two to verify the behavior of the new feature? Glancing through the midi i've got on hand, i don't see any that have pitchBend, so can't easily verify this PR.

@motifn
Copy link
Contributor Author

motifn commented Aug 2, 2019

I emailed you a midi file with pitch bends if that helps (the github interface doesn't let me choose a midi file attachment). The file has pitch bends of both up and down, up to two semitones, assuming that the pitch bend range is 2 semitones, so demonstrates the maximum possible range.

Also, I think it would really good to scale the pitch with value: Math.floor(pb.value * 8191.5) as mentioned in the comment here:

#80 (comment)

@stale
Copy link

stale bot commented Nov 12, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Nov 12, 2019
@tambien tambien merged commit 25e4a47 into Tonejs:master Nov 12, 2019
@tambien tambien removed the wontfix label Nov 12, 2019
@tambien
Copy link
Contributor

tambien commented Nov 12, 2019

thanks for the PR! sorry took a while to find time to merge it. should be published soon

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