[WIP] Midi import

[gdpelican]

For #25

Alright, I've made some decent progress with this. So far it's able to read some notes from a simple midi file, and store them as data in a Seq which includes info about the pitch, volume, duration, start time, instrument (although it's just a hex instrument atm, need to do some sort of map from MIDI instruments to Alda instruments yet...) etc. Sample output below:

(import-midi "./examples/midi/twotone.mid")
({
  :instrument 0,
  :start 0,
  :channel 0,
  :pitch 72,
  :duration 11520,
  :volume 40
} {
  :instrument 0, 
  :start 1440,
  :channel 0,
  :pitch 74,
  :duration 7200,
  :volume 40
})

Question is, where do I go from here? Do I try to output some data like that outlined in the docs, which could then be transformed into an Alda score somehow?

Also this is my first evar clojure, so code feedback more than welcome too!

[daveyarwood]

Excellent work so far! I'll take a closer look a bit later when I have some time and leave some code review comments/thoughts.

I like your approach where the first step is to translate the MIDI data into data that looks sort of like the score data that results from processing an Alda score. What we want to do from here is to generate a string of Alda code.

I don't know too much about what kind of data is in a MIDI file, but are the note-lengths always expressed in milliseconds instead of standard notation (e.g. quarter, eighth, etc.)? If we have them in standard notation, then we can just reuse the same note lengths when we generate the Alda code. If all we have is the millisecond lengths, then I think we might be able to determine the note lengths (in standard notation) by working backwards from the length in milliseconds, along with the tempo.

For example, if we know the tempo is 120 bpm, and a note is 500 milliseconds long, then we could conclude that the note is one beat long and thus represent it as a quarter note. 60 divided by the tempo bpm gives you the length of 1 beat in seconds, at that tempo. e.g. at 120 bpm, 60/120 = 0.5, which means 1 beat = 0.5 seconds or 500 ms. If the note is 250ms long, then we could compare that to the 500ms = 1 beat rule and conclude that the note is half a beat long, which would be an eighth note.

The trouble is that notes are not going to be an exact number of milliseconds. For example, if a quarter note at 120 bpm is played staccato, it is still conceptually a quarter note, but it might only last 250-400 ms instead of the full 500. I'm not sure how to reverse engineer this from a MIDI file, so this could be a little tricky. 🤔

[gdpelican]

Hm yeah, I think there are other events (other than instrument change, note on, etc) which set and change the tempo, so we'll want to include those as well, and then do some fiddling with the durations to relate them to the standard note lengths, and if they're off we might be able to (quant) them based on how far off they are.

[daveyarwood]

Overall this looks great -- definitely a great start!

[daveyarwood]

I think this file/namespace (alda.import) can be removed, and we can just use the alda.import.midi namespace directly.

I do like the idea of leaving the door open for more alda.import.* namespaces in the future 👍

[daveyarwood]

A general comment about closing parens/brackets in Clojure (as with Lisp in general): the closing parens should go at the end of lines instead of on their own line, as you might be accustomed to doing in C-like languages. For example, this ns declaration would typically be written like this:

(ns alda.import.midi
  (:import [javax.sound.midi MidiSystem]
           [java.io File]))

Another example of typical Clojure closing paren/bracket alignment:

({:instrument 0,
  :start 0,
  :channel 0,
  :pitch 72,
  :duration 11520,
  :volume 40}
 {:instrument 0, 
  :start 1440,
  :channel 0,
  :pitch 74,
  :duration 7200,
  :volume 40})

[daveyarwood]

I can't think of a clojure.core function that does this out of the box, but you could also do this with first and filter:

(defn find-existing [coll crit]
  (first (filter #(= crit (select-keys % (keys crit))) coll)))

You could also use a threading macro to make it a little easier to read and understand at a glance:

(defn find-existing [coll crit]
  (->> coll
       (filter #(= crit (select-keys % (keys crit))))
       first))

[daveyarwood]

I think a cleaner way to express this would be:

(defn- note-on
  "We've just heard a new note beginning, update state partials to include it"
  [{:keys [instrument] :as state} event]
  (let [new-partial {:channel    (-> event .getMessage .getChannel)
                     :pitch      (-> event .getMessage .getData1)
                     :volume     (-> event .getMessage .getData2)
                     :start      (-> event .getTick)
                     :instrument instrument
                     ; duration will be set when the note completes
                     :duration   nil}]
    (update state :partials conj new-partial)))

A couple things I'm doing here:

• When updating one key in a map, you can use update rather than assoc to make your code a bit shorter.
• When working with maps as function arguments (or in a let binding), you can use destructuring to reference individual keys without having to write (get my-map :some-key) each time.
• clojure.core/hash-map isn't really used much in practice; it's usually better to use the map literal (curly braces)

[daveyarwood]

Bear with me, but here is how I would refactor this function:

(defn- note-off
  "We've just heard a note complete; remove the note from partials, add a
   duration, and include it as a completed note"
  [{:keys [partials] :as state} event]
  (if-let [{:keys [start] :as partial-note}
           (find-existing partials
                          {:pitch    (-> event .getMessage .getData1)
                           :channel  (-> event .getMessage .getChannel)
                           :duration nil})]
    ; remove partial note and re-add the completed note
    (let [new-note (assoc partial-note :duration (- (.getTick event) start))]
      (-> state
          (update :partials (partial remove #{partial-note}))
          (update :notes conj new-note)))
    ; couldn't find corresponding note; return state as-is
    state))

• destructuring can help us here too, allowing us to type e.g. partials instead of (get state :partials)

• if-let is useful here; we can check to see if a partial is found (i.e. not nil or false), and if so, we take the first path, if not, we take the second path.

• I rename the local binding partial to partial-note so as not to conflict with clojure.core/partial (see below about how we can use partial here)

• We're updating two keys in state (:partials and :notes). Unlike assoc, you can't use update to update multiple keys at once, so what I usually do is use the threading macro -> and call update multiple times on different lines. This feels clean -- it's like we're saying "take state, update :partials, then update :notes."

• (partial remove #{partial-note}) is equivalent to an anonymous function #(remove #{partial-note} %). The #{partial-note} part is a set containing only partial-note. It's common to see sets used as functions like this -- in this case, #{partial-note} can be used as a function that returns true if its argument is partial-note. (Another way you could write this is (partial remove #(= partial-note %)).)

[daveyarwood]

This isn't a bad approach, IMHO. You could also define a helper function that tries to call .getCommand and if that fails, returns nil, something like this:

(defn get-command
  [x]
  (try
    (.getCommand x)
    (catch IllegalArgumentException e
      nil)))

[daveyarwood]

This is mostly stylistic preference, but I would use a threading macro here too to make it easier to see the steps:

(defn- notes-from-track
  "Get a final state by reading through a series of note events"
  [track]
  (or (->> (map #(.get track %) (range (.size track)))
           (reduce process-note-event {})
           :notes)
      ()))

[daveyarwood]

There's nothing wrong with using the new constructor, but it's a lot more common to see the .-suffix version, like (File. path)

[daveyarwood]

I've noticed that in MIDI, playing the same note on the same note consecutively will cause a sort of implicit "NOTE OFF" for the first note so that the second note can play. Maybe we can adjust the logic for when to add a note to account for this?

[daveyarwood]

Closing - see alda-lang/alda#326 (comment)