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

Send and receive note and pedal events to/from connected devices via WebMIDI #112

Merged
merged 35 commits into from Jan 25, 2022

Conversation

broadwell
Copy link
Contributor

@broadwell broadwell commented Oct 4, 2021

The Web MIDI functionality, when available (currently only on Chrome/Edge including Android, though for some others there's this sketchy polyfill/shim) is pretty tightly coupled to the SamplePlayer's operations, but it's tidier to have a distinct WebMidi component that can persist for the lifetime of the app.*

It seemed preferable to avoid needing an actual MIDI connections config menu, so to this end the component connects automatically to all available MIDI devices, listening to and sending messages to all of them. It thus was necessary to complicate the note and pedal on/off logic slightly so that the app would be a good citizen of the MIDI device network and not trigger any MIDI through feedback loops. The component also makes what I think is pretty successful use of the nifty Notification functionality to keep the user updated about MIDI device comings and goings (and also the overall availability of Web MIDI or lack thereof from the browser) in a fairly unobtrusive way.

Note that this functionality can be tested without needing any special MIDI hardware; on Mac OS, the combination of the Inter-Application Communication (IAC) driver and MidiKeys works quite well. This page lists similar combos for other OSes.

* It also would be quite straightforward for this component to make a downloadable MIDI "recording" of the note and pedal events sent to it, even if the browser doesn't support actual Web MIDI device interactions.

@broadwell broadwell marked this pull request as ready for review October 5, 2021 00:14
@simonwiles
Copy link
Member

This is great!

I've had a fun evening trying to set up virtual MIDI devices on my Arch system, and playing around with this. I confess I'm a bit out of my depth with MIDI sequencers and synthesizers and DAWs and so on, but I've been able to record some output from the Pianolatron (and transcribe it to musical notation!) using Rosegarden, and then play it back from a MIDI device into the Pianolatron, adjusting the audio options on-the-fly (but not the tempo, which I suppose makes sense). I do have a keyboard here with MIDI capabilities, but that's in my eldest's room and he's asleep, so I might give that a whirl over the weekend :)

I have some thoughts about the functionality:

  • I wholeheartedly support the idea of connecting to all available MIDI devices automatically -- for someone new to that side of things it all "just worked"™, and I assume those who have complicated set ups also know how to configure things correctly in their audio subsystem (I've been down the rabbit-hole far enough tonight to have been playing with a new patch bay written in Rust :)). I've certainly no desire to get into writing UI for that (the new keyboard shortcuts updates were gnarly in the end), but I wonder if we mightn't hide all the web-midi stuff behind a "Enable/Disable Web-MIDI" button, or something like that? While the notifications are mostly unobtrusive, the bar for "too obtrusive" is really low for functionality most folks won't ever use, imo. The $midiMessageSeen goes some way to addressing this (especially in non-WebMIDI-supported environments), but I still get multiple notifications on every page load in Chromium.
  • I'm a bit unclear on what the value of having the Pianolatron accept MIDI input really is. In addition to making the code a little more complicated (although you've done a nice job of keeping that clean), I wonder if it also makes some of the behaviour a bit awkward and ambiguous. I can play MIDI back through the Pianolatron (or from a physical keyboard, I suppose) and disable pedaling and expressions... What does that even mean? Can you sell me on the value of MIDI input?

w/r/t the code, I'm very happy with the organization. My only comment would be that the <WebMidi/> component could perhaps be a direct child of the <SamplePlayer/> component, since the former's functionality is entirely scoped beneath the latter. The code is good -- there are some linting errors I've cleaned up in midi-in-out-updates, and some stylistic things I'm tempted to suggest some changes to. I did make it crash once, but I need to be more systematic to see if I can reproduce that reliably. I'm happy to work on the notification-related issues you mentioned in d2d83de.

I'll update if I can reproduce the crash, but I wanted to get this down now.

@broadwell
Copy link
Contributor Author

I wonder if we mightn't hide all the web-midi stuff behind a "Enable/Disable Web-MIDI" button, or something like that?

Yeah, this is probably warranted. The notifications definitely can get rather chatty, and reducing them to a level that most users would consider truly unobtrusive seems like it would difficult to achieve and probably wouldn't be welcomed by the small % of users who actually want to take advantage of the Web MIDI features. So an enable/disable config button (default: disabled) is probably the best option.
Re: the notifications, it's a bit odd that on my system, there's a midi.onstatechange-triggered notification about the inter-app MIDI system being available when the app starts and when the first note is played in the app; this seems rather redundant and maybe one of them could be quashed, but otherwise the messages that pop up seem appropriately informative.

I'm a bit unclear on what the value of having the Pianolatron accept MIDI input really is.

Yes this is definitely borderline usefulness-wise, at the moment. As long as the user can both listen to and play the connected HW or SW MIDI keyboard devices that are receiving the Pianolatron's MIDI output, then this should cover the two main use cases the PIs have mentioned for which combining input from the app and a separate device would be useful, specifically 1) applying partial sustain pedaling to MIDI events coming from the Pianolatron, and 2) playing a duet on a separate keyboard with the music being produced by the app. In the case of 2), it might be somewhat desirable to be able to see the keys and pedals of the Pianolatron piano app responding to both the roll's and the human's input, but that's a pretty limited benefit.

One situation in which accepting MIDI input in the app would be called for would be if the app provides the ability to record a MIDI performance, including any input that a user might provide via an external keyboard. I have a demo of this partially working via a customized version of midi-writer-js, but it's not clear how much more effort is needed to get it to 100% functionality, and I'm also not sure whether this is really desirable, given that many external MIDI devices also provide the ability to record MIDI performances, including any inputs they'd receive from the Pianolatron app.

@broadwell
Copy link
Contributor Author

My only comment would be that the component could perhaps be a direct child of the component

Agreed, see the new db17c89 commit. The only reason I didn't do this earlier was that I was under the mistaken impression that the <SamplePlayer/> is recycled each time a new roll loads, like <RollViewer/>, and so it would generate too many MIDI notifications on roll switches. But that's obviously a false premise.

@broadwell broadwell force-pushed the midi-in-out branch 2 times, most recently from 2672c81 to 959a3d0 Compare October 15, 2021 03:07
simonwiles and others added 22 commits January 14, 2022 09:02
There might be a rough edge or two here to be smoothed out later, but this is a solid first pass.
It turns out that hardware MIDI devices are even less happy about this than the stock ToneJS Piano code, and will enter an endless loop of pedal up/down actions if sent consecutive alternative pedal up/down events without any time between them -- as is done in the SamplePlayer to silence all held/sustained notes immediately upon Pause or Rewind. Fortunately the MIDI devices I've tested sound fine on Pause or Rewind when the pedal up/down trick is not used -- in this respect as in many others, they are better performers than the ToneJS Piano.
This allows notifications about the availability of external MIDI connections (or lack thereof), the majority of which are generated just as the app is initialized, to remain on screen long enough to be read. Presumably the purpose of the `clearNotification()` call in `resetApp()` is to clear any lingering error messages from previous rolls, so it's OK not to do it when the app first runs. Note that there's still a potential problem whereby the MIDI status notifications can pile up all at once so that it's still difficult to read any of them before they disappear, but this can be handled in a subsequent update.
There may be more elegant ways to do some of this. Destroying the <WebMidi/> component when it's not in use seems clean, although there is the minor issue of the `midi.onstatechange` handler sticking around. In any case, the checkbox renders some of the previous notifications unnecessary, and it kind of makes sense not to show the checkbox at all if Web MIDI isn't supported in the browser -- though I kind of liked the "too bad your browser can't do this" popup in the previous commits.
This exposes the **actual** issue with the `<Notification/>` component, which is that it's a singleton and is being clobbered.  Will fix in due course.
This isn't what it should look like, where it should go, or even how it should behave (entirely).  Other than that, it's a good commit... :)
It shouldn't be the job of the `<WebMidi/>` component to manage this.

However, this does affect the way the `<Keyboard/>` component (the other client for these functions) works a little -- clicking the keys now removes the `depressed` class if it was previously applied (i.e. if the roll player state is in the middle of (a) note(s)).  This is in line with the behaviour when using the keys on a WebMIDI-connected device.

If this is considered undesirable (and I think I'd lean that way, if not perhaps all that hard), then both the on-screen keyboard and WebMIDI-connected devices could could be decoupled from `$activeNotes`, but I'd rather do that with a argument to the `startNote()` and `stopNote()` functions (similar to the `fromMidi` argument).
This is some opinionated stylistic refactoring.  The main changes introduce more destructuring and switch statements (which are technically faster, but the point here is more about readability).  I think I can make the case that all of these changes are in line with modern best practices, but I'm happy to entertain counter arguments :)
@simonwiles
Copy link
Member

Okay, this is now ready for your perusal and, hopefully, approval. I think this is probably how it should be done, tbh. It's complete and neat as it stands, but leaves space to add more fine-grained control and additional UI should it be desired.

@broadwell
Copy link
Contributor Author

The MIDI panel looks great and is, I agree, the best way to handle the UI/notification features.

I've noticed one bug so far, which is that if I have the MIDI panel open with WebMidi enabled (Chrom* only, obvs) and then select a different roll, the roll viewer goes blank and doesn't display any tiles or overlays, even though the new roll still plays. Also, the right-hand panel doesn't switch back to the basic settings, as it does when loading a different roll in any other scenario and with any other settings panel selected. So maybe this is an issue with how the MIDI settings panel is destroyed (or not) in this scenario?

Also, I did once manage to get the player into a state of "runaway pedal" (when the sustain pedal goes haywire, flipping on/off continuously), which I've occasionally seen before with previous versions of the branch. But this only happened when I accidentally disconnected and reconnected the network cable during a roll playback on a connected MIDI piano, which caused degraded performance from the app all around. So it's probably just worth keeping an eye out for it.

Destroying and re-mounting the right sidebar (in particular) when the roll is changed can cause problems if child components aren't destroyed properly due to outro transitions (see sveltejs/svelte#5268).  This has caused a number of bugs with the `<RollViewer/>` component not being recreated properly.

One option is to eschew all outro transitions in other parts of the app; but I think a better solution in this specific case is to avoid destroying and recreating this part of the component tree unless absolutely necessary.
@simonwiles
Copy link
Member

I've noticed one bug so far, which is that if I have the MIDI panel open with WebMidi enabled (Chrom* only, obvs) and then select a different roll, the roll viewer goes blank and doesn't display any tiles or overlays, even though the new roll still plays. Also, the right-hand panel doesn't switch back to the basic settings, as it does when loading a different roll in any other scenario and with any other settings panel selected. So maybe this is an issue with how the MIDI settings panel is destroyed (or not) in this scenario?

Yeah, this is caused by sveltejs/svelte#5268 again, as you guessed. Removing the transitions on the MIDI device "items" (the <li/>s in the "Connected..." lists) solves the problem, and just using intro transitions and removing the outro transitions is probably sufficient. However, in this case I've just committed a better fix, which is to not recycle that part of the component tree at all when the roll is changed. This is a change that is bigger in scope, though, and could potentially affect other things, so I'd be grateful if you could give it another test and help me be confident it hasn't broken something elsewhere. I'm about 95% confident we're good... :s

@simonwiles
Copy link
Member

Also, I did once manage to get the player into a state of "runaway pedal" (when the sustain pedal goes haywire, flipping on/off continuously), which I've occasionally seen before with previous versions of the branch.

I've noticed this for this first time recently too, but I can't work out how to reproduce it reliably yet. Presumably it's a feedback loop between the <WebMidi/> component and the <SamplePlayer/> component. One thought I do have is that it adds weight to the case for disabling WebMIDI by default even on MIDI-enabled browsers (I've seen this even without an external device connected) -- right now I've defaulted to on for Chromium-based browsers. What do you think?

@broadwell broadwell force-pushed the midi-in-out branch 2 times, most recently from 8a6b9ef to a35c450 Compare January 20, 2022 21:23
simonwiles and others added 5 commits January 24, 2022 15:52
This is a little nicer when enabling/disabling WebMIDI.
Pedal events send to external MIDI devices can be re-emitted back to the app (under at least some circumstances), causing feedback loops.  With this commit, external pedal events are ignored unless explicitly enabled, in which case "app-side" pedal events are disabled instead.
@simonwiles simonwiles merged commit 23d0027 into main Jan 25, 2022
@simonwiles simonwiles deleted the midi-in-out branch January 25, 2022 05:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants