AudioKit / Tonic

Swift library for music theory
https://www.audiokit.io/Tonic/
MIT License
154 stars 21 forks source link

If shiftup results in b#, octave is wrong #26

Closed SebastianBoldt closed 9 months ago

SebastianBoldt commented 10 months ago

macOS Version(s) Used to Build

macOS 13 Ventura

Xcode Version(s)

Xcode 14

Description

I tried to create the correct notes from an augmentedTriad starting on E as a root note.

var notes: [Note] = []
for interval in ChordType.augmentedTriad.intervals {
    // Create the next note by using the shiftup function
    if let shifted = Note(.E, accidental: .natural, octave: 0).shiftUp(interval) {
        notes.append(shifted)
    }
}

The first interval will be applied correctly but the last one will result in a B# in octave 1 which results in the wrong note, one octave too high.

 ▿ 3 elements
  ▿ 0 : E0
    ▿ noteClass : E
      - letter : Tonic.Letter.E
      - accidental : 
    - octave : 0
  ▿ 1 : G♯0
    ▿ noteClass : G♯
      - letter : Tonic.Letter.G
      - accidental : ♯
    - octave : 0
  ▿ 2 : B♯0
    ▿ noteClass : B♯
      - letter : Tonic.Letter.B
      - accidental : ♯
    - octave : 1

Midinotenumbers will be:

 ▿ 3 elements
  - 0 : 16
  - 1 : 20
  - 2 : 36

But should be:

 ▿ 3 elements
  - 0 : 16
  - 1 : 20
  - 2 : 24

I did not fully grasp the shiftUp function but my quick fix was to check if the new note is b# and if it matches we just decrease the octave by 1.

public func shiftUp(_ shift: Interval) -> Note? {
    var newNote = Note(.C, accidental: .natural, octave: 0)
    let newLetterIndex = (noteClass.letter.rawValue + (shift.degree - 1))
    let newLetter = Letter(rawValue: newLetterIndex % Letter.allCases.count)!
    let newOctave = (Int(pitch.midiNoteNumber) + shift.semitones) / 12 - 1
    for accidental in Accidental.allCases {
        newNote = Note(newLetter, accidental: accidental, octave: newOctave)
        // special b# handling 
        if newNote.letter == .B && newNote.accidental == .sharp {
            newNote.octave = newNote.octave - 1
        }
        if (newNote.noteNumber % 12) == ((Int(noteNumber) + shift.semitones) % 12) {
            return newNote
        }
    }
    return nil
}

I am also not sure if maybe similar issues arise with the shift down function.

Crash Logs, Screenshots or Other Attachments (if applicable)

No response

aure commented 9 months ago

Hello @SebastianBoldt

I accepted your pull request but then I delved into this some more and I have come to the conclusion that what Tonic had was close to being the correct information, but the accidental was applied after finding the octave. I reverted that change and have developed the following convention that will be deterministic.

So, in general, the octaves on a keyboard will be defined from the one white key that we usually call C, to the next white key that we usually call B. The octaves represent pitches, and not necessarily spellings.

In practice, using your example, if you pitch up a G#0 by a major 3rd, your spelling will be a B# and your octave is 1, so you do want B#1. You will not want to call this B#0 because it is not in octave 0, it is in octave 1, regardless of the spelling.

Basically octaves are numbers that split up the keyboard and they are agnostic to how the notes are spelled.

All this is reflected in v1.3.0. Thanks for shedding light on this issue so we could come to this deterministic solution.

SebastianBoldt commented 9 months ago

Amazing, thanks for taking care ;)