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

re: Change encoding of pitch classes #20

Merged
merged 7 commits into from
Aug 24, 2020

Conversation

henryksloan
Copy link
Contributor

Original PR is #15 by @jagen31

On top of #15, this PR:

  • Resolves the merge with master
  • Renames structs in accordance with music theory conventions
  • Added some abstractions to reduce and simplify code

Changes from master

  • Several renames produce this new scheme:
    • NoteLetter: A, B, C, etc. Just the letters of the white keys on a piano
    • Pitch: A note letter and an accidental
      • New related abstraction PitchSymbol, an enum { Cb, C, Cs, Db...} convertible to Pitch for code brevity
    • Note wraps a Pitch with an octave
  • More extensive accidental support in parsing, including double sharps/flats in format

Some notes going forward (pun intended)

  • Cb breaks everything as a result of the current design of integer conversions
    • If C4 is flatted, does it produce Cb4 or Cb3. It's enharmonic to B3 but relative to C. Either solution brings up some dependency questions.
  • Some extra considerations will be necessary for double flats and sharps to work throughout the library
  • The above two can be fixed alongside enharmonic support, which I'll be happy to tackle next

@ozankasikci tests run perfectly, but the actual test code and style consistency could use a good lookover

@jagen31
Copy link
Contributor

jagen31 commented Aug 23, 2020

I came here to finally take care of this just now and it's already been done! Thanks!

src/note/pitch.rs Outdated Show resolved Hide resolved
src/note/pitch.rs Outdated Show resolved Hide resolved
@ozankasikci
Copy link
Owner

ozankasikci commented Aug 23, 2020

That's super cool @henryksloan!

Left some comments for the pitch function and an arbitrary number of accidentals support.

Let me know what you think! 💪

@ozankasikci ozankasikci added the enhancement New feature or request label Aug 23, 2020
@henryksloan
Copy link
Contributor Author

henryksloan commented Aug 23, 2020

@ozankasikci good points, all done! I removed that pitch function. As for multiple accidentals, I added support for an arbitrary number of accidentals of the same type, i.e. C##x returns Some(Pitch {letter: C, accidental: 4}), because sharp+sharp+double sharp = 4. That kind of thing is not uncommon when representing higher-order accidentals. I return None when flats and sharps are combined, like in C#b.

I also extended REGEX_PITCH to allow multiple accidentals. The regex now accepts any number of accidents. I think it would be unnecessarily complex to encode the no-mixing constraint into a regex, especially since Rust's built-in regexes don't support backreferencing.

Copy link
Owner

@ozankasikci ozankasikci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, I'm really happy that we now have accidentals support! 👍

@ozankasikci ozankasikci merged commit 09456c4 into ozankasikci:master Aug 24, 2020
@henryksloan henryksloan deleted the pitch-encoding-refactor branch August 24, 2020 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants