search

micdah / RtMidi.Core

RtMidi for .Net Standard
https://rtmidicore.micdah.dk/
Other
36 stars 12 forks source link

WIP: Timestamp & additional messages support #20

Closed Drachenkaetzchen closed 3 years ago

Drachenkaetzchen commented 5 years ago

This is a WIP PR related to #13

I have added timestamp support to all events. I think the PR is backwards compatible, as it does not change the exposed callbacks. All unit tests are currently passing on Windows.

I have decided against BPM calculation in RtMidi.Core, as I think separation of concerns is appropriate here. Since the timestamp is generated by rtmidi, it doesn't matter that much if there are delays due to the event handler.

Drachenkaetzchen commented 5 years ago

Please don't merge this PR yet; it is still work in progress and I most likely will add more messages. Reason for already creating the PR is to allow others to test-drive the changes.

Also I think unit tests are missing for the new messages.

micdah commented 5 years ago

Excellent work - thanks for contributing to the library! Just give me a heads up when you feel it is ready to be reviewed and merged. 😄

I notice you introduce the iMessage interface and add this to all the struct types - but as far as I can see you don't use it at any point?

I think it would be best to remove this interface, as handling a struct as an interface type will incur boxing/unboxing - so having this interface might make users of the library create generalised code which just works with the iMessage interface type and incur unwanted boxing which might affect performance.

My consideration for using structs was performance, and using in variables we ensure they are passed by reference rather than copying the struct around.

In use there will be a lot of midi messages, so it can easily put pressure on the garbage collector by allocating a lot of objects - so I'm trying to keep allocation to a minimum.

Optimally I was considering readonly ref structs, which can only live on the stack - but that was too restrictive in use though.

Drachenkaetzchen commented 5 years ago

The main reason for the IMessage interface is to allow me and other developers to make development easier. Imagine a DAW where a control can be bound to a CC or Midi On/Off messages. In that case, the developer can abstract it down to the bare IMessage interface.

However, I was not aware that this could be a performance problem - I might solve it at another place and not in RtMidi.Core.

Drachenkaetzchen commented 3 years ago

Closed because I was asked to do so in #13