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

Add Web MIDI support #95928

Merged
merged 1 commit into from
Dec 16, 2024
Merged

Conversation

ryanbraganza
Copy link
Contributor

@ryanbraganza ryanbraganza commented Aug 22, 2024

This PR adds support for web exports by using webmidi.

Demo: (code)

Note: webmidi is not supported on all browsers

image

@ryanbraganza ryanbraganza requested a review from a team as a code owner August 22, 2024 01:18
@ryanbraganza ryanbraganza changed the title Adds webmidi support Add webmidi support Aug 22, 2024
@Calinou Calinou added this to the 4.x milestone Aug 22, 2024
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.

Since there is a permission request involved, this should be documented in doc/classes/InputEventMIDI.xml's class description. Something like this:

[b]Note:[/b] On the Web platform, using MIDI input requires a browser permission to be granted first. This permission request is performed when calling [method OS.open_midi_inputs]. MIDI input will not work until the user accepts the permission request.

I would copy the same note at the end of OS' open_midi_inputs description.

doc/classes/OS.xml should have its open_midi_inputs and close_midi_inputs method descriptions modified to mention Web platform support.

PS: It seems OS.get_connected_midi_inputs() will not work on the Web platform if I'm reading the code right. It might be worth documenting this explicitly in its description as well.

const input_names = inputs.map((input) => input.name);

const c_ptr = GodotRuntime.allocStringArray(input_names);
set_input_names_cb(input_names.length, c_ptr);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is where the connected_midi_inputs will be populated.

@ryanbraganza
Copy link
Contributor Author

PS: It seems OS.get_connected_midi_inputs() will not work on the Web platform if I'm reading the code right. It might be worth documenting this explicitly in its description as well.

Kind of. OS.get_connected_midi_inputs() will be populated asynchronously. https://github.com/godotengine/godot/pull/95928/files#r1726105863

@ryanbraganza ryanbraganza requested a review from a team as a code owner August 22, 2024 06:42
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

The extra note is very good, particularly in line with what I would've written myself.

@@ -242,7 +242,8 @@
<return type="PackedStringArray" />
<description>
Returns an array of connected MIDI device names, if they exist. Returns an empty array if the system MIDI driver has not previously been initialized with [method open_midi_inputs]. See also [method close_midi_inputs].
[b]Note:[/b] This method is implemented on Linux, macOS and Windows.
[b]Note:[/b] This method is implemented on Linux, macOS, Web, and Windows.
[b]Note:[/b] On the Web platform, using MIDI input requires a browser permission to be granted first. This permission request is performed when calling [method open_midi_inputs]. MIDI input will not work until the user accepts the permission request.
Copy link
Contributor

@Mickeon Mickeon Aug 22, 2024

Choose a reason for hiding this comment

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

I don't feel like it's necessary to note it here, because the prior description already says that you need to call open_midi_inputs() (whose description contains the details) for this method to work.

If the driver has not been initialized on Web due to lack of permissions, it won't work. The logic feels straightforward.

What isn't straightforward is the asynchronous nature of it, I assume? On Web, are there any chances that the array may "falsely" return empty? If yes, that's worth noting down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What isn't straightforward is the asynchronous nature of it, I assume?

Yes, exactly.

On Web, are there any chances that the array may "falsely" return empty? If yes, that's worth noting down.

Apart from the async nature, and the permissions request being denied, the other "false" cases would be:

  1. The midi device must be connected at the time of calling open_midi_inputs the first time - since the callbacks are cached after that. I could start listening to the statechange event, and then the array of connected inputs would change over time, but it doesn't look like any of the other MIDI drivers (alsamidi/coremidi/winmidi) do that? For example, every time you receive an InputEventMIDI, the device would need to be checked against the current OS.get_connected_midi_inputs() if you wanted to know which device it came from.
    • (re: "the first time") As a "workaround", you can "refresh" the inputs by calling close_midi_inputs() and then open_midi_inputs() again.
  2. Apart from the permissions request being denied, these are the other limitations called out on MDN:
    1. The method must be called in a secure context.
      • But I don't think we need to call this out as being a special requirement for MIDI on web.
    2. Access may be gated by the midi HTTP Permission Policy.
      • Maybe this would be too obscure to note here? The default permissions policy is that midi would be allowed, so whoever is hosting the godot web app would have specifically blocked it for you to run into it.
  3. The user's browser not supporting WebMIDI.

@adamscott adamscott self-assigned this Aug 22, 2024
@adamscott adamscott self-requested a review August 22, 2024 14:12
Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

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

Good work! I have a few comments, but it's on the right track.

@@ -242,7 +242,8 @@
<return type="PackedStringArray" />
<description>
Returns an array of connected MIDI device names, if they exist. Returns an empty array if the system MIDI driver has not previously been initialized with [method open_midi_inputs]. See also [method close_midi_inputs].
[b]Note:[/b] This method is implemented on Linux, macOS and Windows.
[b]Note:[/b] This method is implemented on Linux, macOS, Web, and Windows.
[b]Note:[/b] On the Web platform, using MIDI input requires a browser permission to be granted first. This permission request is performed when calling [method open_midi_inputs]. MIDI input will not work until the user accepts the permission request.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[b]Note:[/b] On the Web platform, using MIDI input requires a browser permission to be granted first. This permission request is performed when calling [method open_midi_inputs]. MIDI input will not work until the user accepts the permission request.
[b]Note:[/b] On the Web platform, using MIDI input requires a browser permission to be granted first. This permission request is performed when calling [method open_midi_inputs]. The browser will restrain from processing MIDI input until the user accepts the permission request.

That's closer to reality. Because if the user declines the permission or takes time to accept, Godot will show no connected devices, but will never know that the permission has been declined.

@ryanbraganza
Copy link
Contributor Author

Happy to rename any variables, or make any other changes. Just let me know!

@ryanbraganza ryanbraganza requested a review from a team as a code owner November 22, 2024 09:48
@ryanbraganza
Copy link
Contributor Author

It's been a while, but I am still interested in getting this merged!
Just rebased to fix merge conflicts.

@adamscott
Copy link
Member

@ryanbraganza Sorry for the delay! I'm reviewing this PR right now!

@adamscott
Copy link
Member

I'm currently investigating why it fails on the proxy_to_thread=yes build.

Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

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

So there's few issues, but overall, it's really nice. Once the changes are made, I'll approve and we'll put this in the merge queue.

WASM_EXPORT static void set_input_names_callback(int p_size, const char **p_input_names);
static void _set_input_names_callback(const Vector<String> &p_input_names);

WASM_EXPORT static void on_midi_message(int p_device_index, uint8_t p_status, const uint8_t *p_data, size_t p_data_len);
Copy link
Member

Choose a reason for hiding this comment

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

As said before, we need to change the signature of on_midi_message() to remove highly detailed types, and we need to add _on_midi_message()

Suggested change
WASM_EXPORT static void on_midi_message(int p_device_index, uint8_t p_status, const uint8_t *p_data, size_t p_data_len);
WASM_EXPORT static void on_midi_message(int p_device_index, int p_status, const uint8_t *p_data, int p_data_len);
static void _on_midi_message(int p_device_index, int p_status, const PackedByteArray &p_data, int p_data_len);

@adamscott
Copy link
Member

PROXY_TO_PTHREAD_ENABLED is essentially telling emscripten to put most of the logic not on the main thread. But as Godot expects some events to happen on the main thread, we essentially use callable_mp_static() to call deferred on the main thread if the event isn't received on it.

@adamscott adamscott changed the title Add webmidi support Add Web MIDI support Nov 22, 2024
@ryanbraganza
Copy link
Contributor Author

applied the changes and rebased

Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

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

Nice work! I have a few suggestions for the docs, but otherwise, all good!

@adamscott
Copy link
Member

cc. @AThousandShips and @Mickeon for my suggested doc changes.

Co-authored-by: Adam Scott <ascott.ca@gmail.com>
Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
@ryanbraganza
Copy link
Contributor Author

Applied all suggested changes, squashed, & rebased

@Repiteo Repiteo modified the milestones: 4.x, 4.4 Dec 16, 2024
@Repiteo Repiteo merged commit 3bc4936 into godotengine:master Dec 16, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 16, 2024

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.

7 participants