Fix MIDI note-on events being converted to note-off events

[voidshine]

This PR fixes a bug in Godot's MIDI processing logic that destroys information as it flows from device to application. The engine converts note-on events with velocity 0 to note-off for programmer convenience, which is sometimes helpful and sometimes confounding. In fact, MIDI device implementations vary and some MIDI devices intend for note-on to mean note-on regardless of velocity. So the only way to support such devices is to faithfully pass MIDI information to the application exactly as it is received from the device.

It's up to application developers to decide how to handle received MIDI data and it's a very minor inconvenience to convert if needed, but having the engine interfere and mutate event data before it reaches the application makes some use cases impossible.

I commented on this before but the issues were closed, so now I'm submitting a fix directly:

• MIDI Note Off message never fires (on some devices) #27199 (comment)
• Fix MIDI Note Off missing on some devices #27214 (comment)

[akien-mga]

The documentation should likely be improved to advise users that they have to do this check themselves if they rely on this common convention.

Not suitable for a 3.5 cherrypick as it breaks compat, so removing that label.

[voidshine]

Good idea, thanks -- I added a second commit that updates the documentation.

[reduz]

AFAIK I am not sure what the point of this is, NOTE ON with velocity 0 is always expected to be note off. Any synth will react to this.

[voidshine]

Hi Juan, first of all, I want to say thanks for all your work on Godot -- awesome engine!

The point of this change is to make it possible for Godot applications to work correctly with all kinds of MIDI input devices. To do that, Godot must report MIDI input events accurately. Currently, Godot is tampering with events in a way that makes some applications impossible. The mutation is often harmless, but not always, and when NOTE ON is transformed by Godot into a NOTE OFF, the application has no way of knowing.

Some MIDI keyboard implementations are very simple; they use the full velocity range from 0 to 127 with NOTE ON for key presses, and then they use NOTE OFF for key releases. For such keyboards, a gentle press of the key will play a quiet note in other apps, but will be ignored as a NOTE OFF in Godot apps. There's no way for a Godot script to know the truth that the device reported a NOTE ON, because Godot mutated it. The mutation was well-intended, for the sake of programmer convenience, but it's inaccurate. So this PR is a bug fix, improving Godot's correctness and capability.

[reduz]

@voidshine makes sense, I guess if you want to use a device as a controller for a specific situation, Godot should simply pass what comes in, as there is no reason to alter it.

[akien-mga]

For the record, the email you used to author this commit isn't valid, so it's not attributed to your GitHub account (see the header of https://patch-diff.githubusercontent.com/raw/godotengine/godot/pull/66003.patch). If you want to change it, you can do so by editing your local Git identity and then git commit --amend --reset-author).

[voidshine]

Thanks for accepting the change! Excited to make my first little contribution here. :)

[akien-mga]

Could you squash the 2 commits? See PR workflow for instructions.

(For the record that's why I suggested amending the original commit with git commit --amend, instead of making a new commit.)

That would also give you the possibility to fix the email on that first commit, which is still the original invalid one.

And finally, the new email you used is valid, but not tied to your GitHub account. That's not a problem, but if you want the commit to appear as authored by @voidshine, you can add that email as secondary email in your GitHub settings.

[voidshine]

@akien-mga I actually did use --amend, I entered the git commands as written. So there should only be my original two commits, no extras. Are you saying I should not have created the second documentation-update commit in the first place? Having two seemed sensible at the time and doesn't seem to violate the article's principle about keeping Godot in a functional state. Happy to conform to Godot's PR workflow here, I just thought what I did was sensible.

Would squashing the code and documentation commits together invalidate the approval? I'd like to avoid spending more code approver time on a history change if possible. If that's allowed without approval invalidation, I can try my hand at rewriting history...

[voidshine]

Also, thanks again for the tip about email -- I am not too concerned about credit here, but will go ahead and validate the email with GitHub so it will link things up.

[akien-mga]

Ah I see. Well the code change and the documentation are related, so it's better to have them in a single commit. The documentation change is only valid because of the code change so they can be kept together as a unit.

[voidshine]

Makes sense, I just did the squash, thanks for the guidance here!

[akien-mga]

Thanks! And congrats for your first merged Godot contribution 🎉

[voidshine]

Thanks! 🎉 Hoping to make some more in years to come. 💟 Godot!

[akien-mga]

Cherry-picked for 3.6.