djipco / webmidi

Tame the Web MIDI API. Send and receive MIDI messages with ease. Control instruments with user-friendly functions (playNote, sendPitchBend, etc.). React to MIDI input with simple event listeners (noteon, pitchbend, controlchange, etc.).
Apache License 2.0
1.53k stars 115 forks source link

noteoff events not being fired. #211

Closed glend1 closed 2 years ago

glend1 commented 2 years ago

I have the following code;

midiDevice.addListener('noteon', (e) => {
    console.log("on")
    let data = normalizeMidi(e.note.number, sharp)
    start(data)
});
midiDevice.addListener('noteoff', (e) => {
    console.log("off")
    let data =  normalizeMidi(e.note.number, sharp)
    stop(data)
});

When I press a note button on my midi keyboard noteon is triggered when i release the button noteoff isn't. I know this because nothing is showing up in the console for off but on is fired twice.

djipco commented 2 years ago

I just tried your code and it works fine for me. Are you using v3.0.6 ?

glend1 commented 2 years ago

yes. latest version. it might be a next.js thing then. it used to work in v2.

djipco commented 2 years ago

The problem is that version 3 brought Node.js support and these frameworks (React, Next, etc.) tend to use the CommonJS version of modules even for front-end projects. It makes it complicated to know which backend to use. In the browser, the library uses the Web MIDI API but in Node.js, it uses the jzz plugin... How do I know which one to use? I nailed it for React but something else seems to be going on in Next...

glend1 commented 2 years ago

If you allow the user to choose which version then the problem goes away.

djipco commented 2 years ago

If you allow the user to choose which version then the problem goes away.

The only way to do that would be to have 2 npm packages, one for browser and one for Node. I was trying to not go that down that road...

glend1 commented 2 years ago

couldn't enable() have an option where you can set it?

djipco commented 2 years ago

Doing it through enable() is too late. Modules have already been loaded by then. I will have to take a deeper look at conditional imports which may have better support now...

glend1 commented 2 years ago

you could have 3 different imports for the required behaviour. for example;

import { WebMidi } from 'webmidi'; for auto-detecting.

import { WebMidi-browser } from 'webmidi'; for browser.

import { WebMidi-node } from 'webmidi'; for node.

you could even include in your documentation to use aliases, so that all the examples still work import { WebMidi-browser as WebMidi } from 'webmidi';

djipco commented 2 years ago

If it wasn't for Webpack, there would be no need for all of that. I would much prefer find a solution that does not introduce extra complexity and generate unnecessary confusion. On the flip side, the status quo is generating confusion for Webpack users.

This is starting to drive me nuts...

P.S. According to the Webpack documentation, you should be able to use the externals configuration directive to exclude specific modules from the bundle. Theoretically, excluding the jzz module by using a custom webpack.config.js for Next.js should make the problem go away.

glend1 commented 2 years ago

Im not a fan of changing a configuration for a library. espeically when v2 works.

djipco commented 2 years ago

Okay... it took me forever but I think I have a solution that's completely transparent for end users of the library. Can you try v3.0.7 (available now on npm) and let me know how it works for you?

glend1 commented 2 years ago
ReferenceError: require is not defined in ES module scope, you can use import instead
This file is being treated as an ES module because it has a '.js' file extension and 'E:\project\node_modules\webmidi\dist\esm\package.json' contains "type": "module". To treat it as a CommonJS script, rename it to use the '.cjs' file extension.

Im not quite sure what file this is refering to. i changed the file extensions in the esm directory (to cjs) and it didnt help because the file couldn't be found

djipco commented 2 years ago

This actually makes sense. The fix I did yesterday works when developers are using require to import external modules. It does not work for you because you have "type": "module" in your package.json file and are using import to bring in external modules. This means only half the problem is solved... I'll try and get this resolved soon.

For some odd reason, it was working for me yesterday when I tested it. There must have been something else happening.

glend1 commented 2 years ago

For some odd reason, it was working for me yesterday when I tested it. There must have been something else happening.

I know this feeling too well.

djipco commented 2 years ago

For whatever reason, if you import the file directly it works but if you let Webpack figure it out, it doesn't. This is what I hate about all this JavaScript tooling... You're never quite sure what's going on behind the scenes.

This works:

import {WebMidi} from "./node_modules/webmidi/dist/esm/webmidi.esm.js";

This does not work:

import {WebMidi} from "webmidi";

This is why I thought it was working yesterday. Anyway, let me figure it out...

djipco commented 2 years ago

This time I think I nailed it. Please try v3.0.8. I tested it and in the works in all these scenarios:

Please tell me it works for you too... πŸ™πŸ» πŸ™‚

glend1 commented 2 years ago

This has fixed the issue described in #209 however, noteoff events still aren't fired instead a noteon event is being fired.

At this point I feel like its probably something in my code, but logging the event to the console shows that the event type is noteon when I release the key..

I have also noticed that in v2 connected events would be fired if there is already midi devices plugged in when the webpage is loaded. this doesn't appear to be the case for v3. I suspect this has to do with the new restructuring of the initialising function and I'm not sure you consider this a bug. but it might help you locate the source of this bug.

djipco commented 2 years ago

Thanks for getting back to me. I'm glad that you can confirm #209 is fixed. It has been quite complicated to resolve in a satisfying manner.

Regarding noteoff events, I have not been able to exactly reproduce the problem you described. However, I did witness issues when Next.js performs a "Fast Refresh". In such a case, old listeners remained while new listeners were added. Also, the behaviour in the Terminal console was different from the behaviour in the browser's console. So, I'm guessing the problem relates to that. When you simply close and restart Next.js' dev server, everything goes back to normal.

Regarding the connected events, they do function slightly differently than in version 2. Here's the actual sequence of events when calling WebMidi.enable() in v3:

  1. midiaccessgranted event is triggered once the user has granted access to use MIDI.
  2. connected events are triggered (for each available input and output)
  3. enabled event is triggered when WebMidi.js is fully ready
  4. specified callback (if any) is executed
  5. promise is resolved and fulfilled with the WebMidi object.

So, if you want to catch connected events for already-connected devices, you should add the listeners before calling enable():

WebMidi.addListener("connected", e => console.log(e.type));
WebMidi.enable();

If you add a listener for connected events after the enabled event is fired, you will not catch connected events for already connected devices (this is the part that changed). Doing it this way allows you to only listen for newly connected devices (by adding listener after the enabled event is fired) or listen to all of them (by adding the listener earlier). I hope this all makes sense.

I'd like to take a moment to thank you for reporting the issues. I have had a hard time finding early testers for v3. Your contribution is very valuable to me and the community. πŸ™πŸ»

Having said that, I'm going to close this issue. Do not hesitate to file a new one if any problems persist.

glend1 commented 2 years ago

Regarding noteoff events, I have not been able to exactly reproduce the problem you described. However, I did witness issues when Next.js performs a "Fast Refresh". In such a case, old listeners remained while new listeners were added. Also, the behaviour in the Terminal console was different from the behaviour in the browser's console. So, I'm guessing the problem relates to that. When you simply close and restart Next.js' dev server, everything goes back to normal.

I am aware of this behaviour. Fast refreshes occur when you change the source code. this isn't what is at play here. No fast refreshes are happening. I am also destroying the listeners responsibily so this doesn't happen.

Thank you for the information regarding connected events.

djipco commented 2 years ago

I am aware of this behaviour. Fast refreshes occur when you change the source code. this isn't what is at play here. No fast refreshes are happening. I am also destroying the listeners responsibily so this doesn't happen.

If you could somehow create a small example that reproduces the problem, I'd be happy to look into it. So far, I haven't been able to witness the problem myself.

Thank you.

glend1 commented 2 years ago

Steps to repoduce;

npx create-next-app@latest

npm install --save webmidi

then replace the contents of index.js with

import { useEffect } from 'react'
import { WebMidi } from 'webmidi'

const i = 1

export default function Home() {
  useEffect(() => {
    WebMidi.enable()
  }, [])
  function addlistener() {
    WebMidi.inputs[i].addListener("noteon", (e) => {
      console.log(e)
    })
    WebMidi.inputs[i].addListener("noteoff", (e) => {
      console.log(e)
    })
  }
  return (
      <button onClick={addlistener}>click me</button>
  )
}

npm run dev

then press the button in the browser located at localhost:3000.

now whenever you press a midi button on your device a noteon event will appear in the console, when you release the midi button you will recieve a second noteon event.

Note: you will have to change const i = 1 to equal a device that will send midi.

djipco commented 2 years ago

I just followed your exact instructions and I get the expected result: when I press a key I get a noteon and when I release the key I get a Β noteoff. I'm not sure why you are not getting the same thing...

glend1 commented 2 years ago

our environments must be different. im using chrome and windows 10. you?

I only have 1 physical midi device (novation launchkey mini mk3). so i can't test it on different things.

i could try a different computer.

maybe i could test with a virtual device somehow. any ideas how to do that?

EDIT: what version of node? im using v16.13.0

djipco commented 2 years ago

I tested it in Chrome on macOS. To test it, you could install some sort of software keyboard and route back MIDI into Chrome using LoopMIDI.

P.S. I'm using Node v16.11.1 but that shouldn't matter.

glend1 commented 2 years ago

i just remembered it worked in v2. hmm....

glend1 commented 2 years ago

Ok, i figured out whats happening. My midi controller is sending a noteon for a noteoff. its just changing the velocity and attack to 0 and therefor is implying a noteoff for some reason v2 was reporting a noteoff.

Sorry for reporting a false problem. Although, the novation launchkey mini is a popular controller so others may have this problem. ill look into seing if i can change my controllers behaviour... or something...

are noteoff events common? how many other controllers just "mute" the note? am I blowing this problem out of proportion?

djipco commented 2 years ago

You are not reporting a false problem. In fact, you just found a bug in the library πŸ˜‰. I should've thought about it when you first mentioned it...

Per the MIDI specification, a noteon with a velocity value of 0 must be considered a noteoff. This is useful when using running status as it allows to shave a few bytes from MIDI messages. Many devices use this technique. The library is the problem here, not your device.

Since it was working in v2, this is a regression bug. I don't know how it crept back into the codebase. To prevent this from happening again, I added a unit test specifically for that case.

Version 3.0.10 is available and it fixes the problem. Sorry about that...