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 high-level midi datatypes #36

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Cyclic3
Copy link

@Cyclic3 Cyclic3 commented Jul 11, 2017

I spent a day writing enums and functions to handle the midi data, and then realised that they could be shared with everyone.

Do they work?

@Boscop
Copy link
Contributor

Boscop commented Jul 11, 2017

Good idea, a lot of VSTs deal with MIDI but:
I think it would make more sense to have MIDI types in a separate crate, because it would be useful outside of VST development, too. And it would 'separate the concerns' of VST and MIDI which would be more orthogonal / elegant.

Also, maybe you'd want to use the MIDI Status type from the rimd crate and re-export that (I use that in my own VSTs).
Btw, I'd also use enum_from_primitive for your other enums.
And I'd use STATUS_MASK and CHANNEL_MASK from rimd (I think they should be made public in rimd).
Btw, what is the purpose of your Data type? It seems like you intend it to store the info of a general MIDI message in a higher abstracted form but you are only storing the status information, not the channel and actual data (only for SysEx msgs). And what is the purpose of this Data::Voice(i, b[1]), you're ignoring b[2] and the channel. I think what you intended (and what would make sense) is to have a datatype that represents a MIDI message on a higher level. But then it should also store channel info and b[2].
And if b[0] ^ 0b11111000 should probably be if b[0] & 0b11111000. And I would give all masks names (constants). Also I would just return Options instead of Result<Self, bool>.

But that's just my two cents..

Btw, I was actually considering using such a higher-level type and rewriting rimd to use that type instead of the low-level Vec<u8> for all messages. Or at least not using a Vec for short messages (which are all except sysex messages and are the majority in a normal MIDI file). This would speed up parsing MIDI files because it wouldn't have to allocate a Vec for each small message during reading.
So it could be enum MidiMessage { Short(ShortMessage), Long(Vec<u8>) } struct ShortMessage(u8, u8, u8); for rimd to reduce allocations (MidiMessage would be able to be constructed from a &[u8]) and then a new crate would provide a more discriminating higher level message type that can be parsed from a MidiMessage.

@Cyclic3
Copy link
Author

Cyclic3 commented Jul 12, 2017

Thanks for the detailed response!

Putting the midi handling in a separate crate sounds like a much better idea than dumping it in the main library.

The Data type is supposed to be a type for a MIDI message, but as you have correctly pointed out, I may have messed up some of the cases (a key was pressed, but we're not going to tell you which one =])

The reason why some return a Result<_, bool> is because I wanted the from_bytes function to do all of the checking. However, if I returned an option. I would not be able to distinguish between the midi message being the wrong type, and the midi message being malformed. In the former case, I want to try the next type, and in the latter, I want to return a None. There are ways to avoid this, but the ones I can see involve either a) doing the check twice b) not doing the check in the from_bytes function.

I had not found the rimd crate when I started writing this (I should have spent longer looking). On first glance, it looks significantly more detailed (and better written) than the module I wrote. I will move my VST to using that crate.

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.

None yet

2 participants