search

BlackPhlox / bevy_midi

Send and receive MIDI data to and from bevy using DAWS or MIDI Controllers
Apache License 2.0
56 stars 10 forks source link

`midly` integration for comprehensive message parsing #35

Open masonium opened 11 months ago

masonium commented 11 months ago

Currently, bevy_midi only parses Note On and Note Off messages, leaving it to the user to parse most messages. Because messages are consistently presented as 3 bytes, some messages are bound to be incorrectly parsed as well (this is the root cause of #29 , I think).

What do you think about using an external library, like midly, to do the messaging parsing? It has MidiStream specifically meant for parsing undelimitted byte streams of MIDI data into discrete events.

It does add an external dependency, and it would be a breaking change to the interface if bevy_midi directly uses its LiveEvent as the output. (midly itself has only one optional dependency in rayon, which bevy_midi wouldn't need for streaming data).

If it sounds reasonable, I'd be happy to take a crack at it.

BlackPhlox commented 11 months ago

It does sound very reasonable! midly have been on my radar, in regards to midi file playback (See here and here), but it is a bit of work that I myself don't have capacity right now. LiveEvent would also be a better place to start from, so you have my full support on this! 🚀

midly dosn't mention anything about WASM support as mentioned in #1, but that might be an non-issue and is just a question about implementing the integration via js/wasm bindgen

While we are talking about parsing underlying Midi messages, I wonder if it also would resolve #33, as it looks like some midi controllers have different ways of handling, in this case, the NOTE_OFF event. Looking at midly at a glance, it dosn't look like it handles it. I was thinking to create a lookup table for certain midi-devices and their name, to then handle odd cases as 33 and otherwise use the default, I would like to hear if you have any opinion on this in regards to midly.

Regarding #29, you might be right but I think it has more to do with it being Ableton Push 2 Live Port which doesn't follow the midi spec.

masonium commented 11 months ago

Some updates:

midly itself seems to build fine with WASM, so I don't foresee an issue there.

The main thing I've run into is that LiveEvents are parameterized by lifetime, between they share storage with the buffer used in MidiStream. There is an to_static function which converts any LiveEvent<'a> to a LiveEvent<'static>, but this drops the data from SysEx messages. One approach is to accept that limitation; another approach is to create an OwnedLiveEvent and OwnedSystemCommand that mirrors the respective midly implementations but uses Vec<u8> (or a stack-allocated version) to make the events usable.

It could be nice to add some convenience functions for certain transformations (e.g. NOTE_ON with velocity 0 is converted to NOTE_OFF), or a comprehensive MessageTranslator that has a bunch of options for transformations. Personally I would lean against the lookup table approach; it strikes me as a maintenance headache (user reports with requests for changes but no real way of testing, etc.) but I don't have a strong opinion.

Anyway, I'm happy to proceed with the mirrored OwnedLiveEvent , etc. if you don't have a strong preference otherwise.

BlackPhlox commented 11 months ago

Nice, that's good news! I'm all for the mirrored OwnedLiveEvent. Yeah, lookup will be painful, I think some DAWs have midi read certain transformations if they are non-destructive to other midi controllers, such as NOTE_ON with vel 0. I'll take a look at an impl. for that