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

InputEvent System Fails to Capture Simultaneous MIDI Events #77035

Closed
Tracked by #76797
ashih42 opened this issue May 13, 2023 · 9 comments · Fixed by #90485
Closed
Tracked by #76797

InputEvent System Fails to Capture Simultaneous MIDI Events #77035

ashih42 opened this issue May 13, 2023 · 9 comments · Fixed by #90485

Comments

@ashih42
Copy link

ashih42 commented May 13, 2023

Godot version

4.0.2.stable.official [7a0977c]

System information

Apple M1 macOS Ventura 13.3.1

Issue description

There are many common situations where multiple simultaneous MIDI events may occur:

  1. Playing a chord (multiple simultaneous Note On messages).
  2. Releasing a chord (multiple simultaneous Note Off messages).
  3. Some MIDI device may always precede a Note On or Note Off message with a Control Change message for High Resolution Velocity Prefix, so that pressing a single note would send 2 simultaneous messages.
  4. Active Sensing and Clock MIDI messages emitted by MIDI device constantly in the background, happening simultaneously with other MIDI messages triggered by the user.

Godot's InputEvent System may fail to capture some of these multiple simultaneous MIDI input events. In the following report, I have demonstrated examples for situations (1) and (3) and observed Godot failed to process some expected MIDI messages. I have not experimentally confirmed situation (4), but in theory it could happen due similar timing-based mechanism if the user is unlucky.

Steps to reproduce

The following 2 examples uses MIDI Monitor on macOS (a tool to observe received MIDI messages before a program handles them) and the following GDScript to print MIDI events handled by Godot:

extends Node2D

func _ready():
	OS.open_midi_inputs()

func _input(event):
	if (
		event is InputEventMIDI and
		event.message not in [MIDI_MESSAGE_ACTIVE_SENSING, MIDI_MESSAGE_TIMING_CLOCK]
	):
		print(event)

Example 1: Godot failed to process 1 of 3 expected Note On messages when playing a C-major triad chord.

chord

What Midi Monitor observed:

  • Note On
  • Note On
  • Note On
  • Note Off
  • Note Off
  • Note Off

What Godot processed:

  • Note On
  • Note On
  • Note On velocity=0
  • Note On velocity=0
  • Note On velocity=0

(Godot converts Note Off messages to Note On with velocity=0, which is odd but still conforms with MIDI Spec.)


Example 2: Godot failed to process single Note On and Note Off messages which were preceded simultaneously with Control Change for High Resolution Velocity Prefix messages.

pair

What MIDI Monitor observed:

  • Control Change for High Resolution Velocity Prefix
  • Note On
  • Control Change for High Resolution Velocity Prefix
  • Note Off

What Godot processed:

  • Control Change for High Resolution Velocity Prefix
  • Control Change for High Resolution Velocity Prefix

Minimal reproduction project

MidiTest.zip

@MJacred
Copy link
Contributor

MJacred commented May 18, 2023

velocity=0 part is related to #76678

@brunodeangelis
Copy link

brunodeangelis commented Feb 15, 2024

This is EXACTLY the issue I'm having, and it's so frustrating! Also on a M1 Mac, Sonoma 14.2.1

I'm making a piano visualiser (similar to PianoVFX for example) and this issue is making it basically impossible to play any piece without a bug slipping in, most commonly a note that was released but still registers as pressed because the message never got through. I don't get this issue in Logic Pro or Ableton. I'm willing to help fix it but I would need directions as to where to look into

See comparison in MIDI Monitor
2024-02-15_19-30-55

I can also confirm that this issue does not happen on Windows, I tried with the same project and it behaves as expected

@joedski
Copy link

joedski commented Feb 29, 2024

This also happens on my Intel Mac, attested to on Ventura 13.3.1. I wanted to animate some pixel toads, but had to offset events to prevent half of them being swallowed up by this issue.

@markofjohnson
Copy link

Confirmed this bug today on M1 Mac using Roland FP90X keyboard over bluetooth midi playing chords (very easy to repro) and Roland A49 over usb (harder to repro). Play a 3 note chord on piano keyboard, Midi on the Mac does receive 3 note ON as seen by eg Midi Monitor app. But only 2 note on messages are passed into gdscript as InputEventMidi. To debug I added printf to MIDIDriver::receive_input_packet, and indeed that code only passes 2 note on into gdscript.

Suspect bug is in midi_driver.cpp MIDIDriver::receive_input_packet. Appears to only processes one midi message from each packet. But midi packet can contain multiple messages, as is common when playing chords. References:
https://cmtext.indiana.edu/MIDI/chapter3_channel_voice_messages.php
https://stackoverflow.com/questions/65448257/ios-samplerunit-stuck-notes

I will comment again if I can hack up a fix to show my diagnosis correct. However I'm new to Midi and many years since I used C++ or even git so it will be quite a stretch for me to get a fix as far as a decent pull request. If someone else submits a proper fix before I do, I will not be sad. Or if someone wants to team up to get this fixed together, reach out. I've not looked at the other Midi drivers for Windows or Linux, it's possible they have a similar bug?

@markofjohnson
Copy link

Confirmed my diagnosis by modifying MIDIDriver::receive_input_packet to track consumed data bytes and recursively call itself again if additional data bytes are in the midi input packet. This fixed the bug and correctly passed all note on messages through Godot. I will improve my bugfix and get a pull request together. As the bug is in midi_driver.cpp I think it will be in all platforms and also back in Godot 3 also?

@markofjohnson
Copy link

Mean time a possible work around for developers... Run a midi utility that consumes and re-transmits the midi data with each Note On or Off in its own midi message instead of cramming multiple into a 'running status' packet. (Chances are that most utilities do the simple thing and retransmit each note separately?) Have Godot consume the midi retransmitted by the utility rather than the original keyboard. This seems to work with 'MidiView' on the Mac.

@brunodeangelis
Copy link

Confirmed my diagnosis by modifying MIDIDriver::receive_input_packet to track consumed data bytes and recursively call itself again if additional data bytes are in the midi input packet. This fixed the bug and correctly passed all note on messages through Godot. I will improve my bugfix and get a pull request together. As the bug is in midi_driver.cpp I think it will be in all platforms and also back in Godot 3 also?

I did not find this issue on Windows, unsure why. But it worked perfectly there. Thanks for looking into a fix!

@markofjohnson
Copy link

I have forked Godot main and committed a fix here: 74f1fbb
Tested with playing chords. Will pull request after I've tested with some additional midi message types.

@markofjohnson
Copy link

PR to fix this issue is ready, hoping it will get merged into 4.3.
#89871

MIDI messages may be sent using 'running status', eg when playing a chord or performing a pitch bend. On Mac, these running status MIDI messages arrive into Godot in running status form, so bug fix is needed for MIDI to work properly on Mac. On Windows, the OS layer 'unrolls' the running status and delivers to Godot as individual messages, so on Windows this bug does not happen. On Linux I don't know.

It would be great if some more people can test this PR. (No bug on Windows, but still needs testing on Windows of course to ensure MIDI still works.) You can get binaries of the Godot Editor built with this bug fix here:
https://github.com/godotengine/godot/actions/runs/8473299568/workflow?pr=89871
Click on the platform you will test, Editor, expand Upload artifact and find a link to download the compiled editor.

@AThousandShips AThousandShips added this to the 4.3 milestone Apr 12, 2024
vgezer pushed a commit to vgezer/godot that referenced this issue Jun 27, 2024
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)
<[email protected]>
sorascode pushed a commit to sorascode/godot-soras-version that referenced this issue Jul 22, 2024
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)
<[email protected]>
Luis-Wong pushed a commit to Luis-Wong/godot that referenced this issue Jul 26, 2024
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)
<[email protected]>
2nafish117 pushed a commit to 2nafish117/godot that referenced this issue Aug 5, 2024
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)
<[email protected]>
chryan pushed a commit to chryan/godot that referenced this issue Aug 6, 2024
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)
<[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment