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

Context aware MIDI event printing #68820

Merged
merged 1 commit into from
Aug 28, 2023

Conversation

alcomposer
Copy link
Contributor

@alcomposer alcomposer commented Nov 18, 2022

What this adds

Adds Human readable MIDI event context aware printing, typical use case in GDScript:
print(midi_event)
output for note-on message:
InputEventMIDI - Note On: channel=0, pitch=1, velocity=127

I also added missing instrument printing. However keep in mind outputting a valid program-change is a multi-step process.

Whats the current problem

Currently printing a MIDI event results in all InputMIDIEvent variables being listed:
InputEventMIDI: channel=0, message=9, pitch=1, velocity=127, pressure=0, controller_number=0, controller_value=0
However, the problem is that each MIDI event only contains specific variables. There is no controller_number in a MIDI-Note-On message.

This makes it confusing to understand the MIDI events, and clutters the output console. We are also listing the MIDI message raw number, as opposed to the enum, which is already present in Godot.

Discussion

I have chosen to omit the message= as we now know what the message is, and Godot already has helpful enums to deal with messages anyway:
if event.message == MIDI_MESSAGE_NOTE_ON etc.

Improvement in the future can occur with SYSEX printing added, along with other event types. However this PR provides the vast majority of use cases.

NOTE: Many MIDI devices use NOTE-ON with velocity 0 to signify NOTE-OFF. As Godot's MIDI system is intended to allow developers to use MIDI in their own software we shouldn't deal with this. Rather provide the data as it comes from the MIDI input, and allow the end developer to deal with it how they choose.

MIDI events printed in this PR:

NOTE_ON
NOTE_OFF
PITCH_BEND
CHANNEL_PRESSURE
CONTROL_CHANGE

For all other message types this PR defaults to the old way of displaying MIDI events. This can be the topic of future PRs.

NOTE: Pitch Bend event is currently being saved in pitch, whereas it should be saved into it's own variable: pitch_bend for consistency. This will break current API, so will be subject to another PR for 4.x

See godotengine/godot-proposals#5805 for more info

@alcomposer alcomposer requested a review from a team as a code owner November 18, 2022 08:12
@alcomposer alcomposer force-pushed the midi_print_context_aware branch 2 times, most recently from e50f677 to 504abea Compare November 18, 2022 08:44
@Calinou Calinou added this to the 4.0 milestone Nov 18, 2022
@alcomposer alcomposer force-pushed the midi_print_context_aware branch 2 times, most recently from 504abea to c8d2d2b Compare November 29, 2022 03:51
@YuriSizov YuriSizov modified the milestones: 4.0, 4.1 Feb 10, 2023
@YuriSizov YuriSizov modified the milestones: 4.1, 4.2 Jun 14, 2023
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. This should make troubleshooting easier.

@akien-mga akien-mga changed the title context aware MIDI event printing Context aware MIDI event printing Aug 8, 2023
@YuriSizov YuriSizov requested a review from a team August 9, 2023 16:21
@akien-mga akien-mga force-pushed the midi_print_context_aware branch from c8d2d2b to a415533 Compare August 28, 2023 10:41
@akien-mga akien-mga force-pushed the midi_print_context_aware branch from a415533 to deaf6c3 Compare August 28, 2023 10:42
@akien-mga akien-mga merged commit a8e93f3 into godotengine:master Aug 28, 2023
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants