-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Move MIDI parsing up from ALSA driver to platform independent driver. #90485
Conversation
Sorry for the noisy diff. This one moves a few things around, but hopefully the result is clearer. |
fa1ed95
to
c5daf42
Compare
I've not managed to get hold of a Mac test machine, so very grateful if anyone could give this branch a test on that. A small MIDI test project if needed: |
I tested this PR with chord playing on Mac as per defect #77035 . This fixes the bug :-) . I logged data going in and confirmed that packets with more than one MIDI message work correctly. (I noticed that I was not seeing MIDI running status, but packets with multiple messages including repeated status bytes. So the previous diagnosis of the problem may not have been due to running status, but due to Mac Core MIDI allowing more than 1 MIDI message in 1 packet.) |
@markofjohnson great to hear :) Thanks for that |
b04cc0b
to
8d921c0
Compare
Aims for more consistent MIDI support across Windows, MacOS, Linux and to provide a base for adding MIDI drivers for other platforms. Reworks the MIDIDriverALSAMidi MIDI parsing implementation as a platform independent version in MIDIDriver::Parser. Uses MIDIDriver::Parser to provide running status support in MacOS MIDIDriverCoreMidi. Collects connected input names at open, ensuring devices indices reported in events match names in array returned from get_connected_inputs. Fixes godotengine#77035. Fixes godotengine#79811. With code review changes by: A Thousand Ships (she/her) <96648715+AThousandShips@users.noreply.github.com>
8d921c0
to
607c5ec
Compare
Thanks @AThousandShips :) Is there a full list of the current style rules? |
See this it covers most |
Ah, I missed the Comment Style Guide at the bottom there, sorry about that. |
Thanks! |
Thanks for this. On 4.3.dev6 I was seeing corrupt (pointer?) values for the device on midi events. Now on 4.3.rc3 it's consistently the device index. |
Aims for more consistent MIDI support across Windows, MacOS, Linux and to provide a base for adding MIDI drivers for other platforms. Reworks the MIDIDriverALSAMidi MIDI parsing implementation as a platform independent version in MIDIDriver::Parser.
Uses MIDIDriver::Parser to provide running status support in MacOS MIDIDriverCoreMidi.
Collects connected input names at open, ensuring devices indices reported in events match names in array returned from get_connected_inputs.
Fixes #77035.
Fixes #79811.