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

Wrong octave system-wide #42

Closed
aik099 opened this issue Jun 8, 2018 · 12 comments
Closed

Wrong octave system-wide #42

aik099 opened this issue Jun 8, 2018 · 12 comments
Assignees
Labels
Type: enhancement Request for improvement
Milestone

Comments

@aik099
Copy link

aik099 commented Jun 8, 2018

When I press Middle C, then noteon event reports it as C3 while it should be C4.

I'm using event listening code from README.md:

 // Listen for a 'note on' message on all channels
  input.addListener('noteon', "all",
    function (e) {
      console.log("Received 'noteon' message (" + e.note.name + e.note.octave + ").");
    }
  );

Same happens, the other way around: attempt to play C3 actually plays C4:

output.playNote('C3', 'all', {duration: 2000, velocity: 0.25});

Here are octaves shown by Synthesia and it clearly shows that when C3 is reported by noteon event, then Synthesia (and other programs) display it as C4:

2018-06-08_0853

I'm suspecting an issue when in math, when decoding/encoding midi messages.

When I press 1st key on the keyboard (A0) it's reported as A-1, which according to README.md is a special code for stop playing note on all channels (see output.stopNote("C-1"); in there).

@aik099 aik099 changed the title Wrong octave reported in "noteon" event Wrong octave system-wide Jun 8, 2018
@aik099
Copy link
Author

aik099 commented Jun 8, 2018

Removing last - 1 from WebMidi.prototype.getOctave method should fix octave reporting in events:

old: return Math.floor(parseInt(number) / 12 - 1) - 1;
new: return Math.floor(parseInt(number) / 12 - 1);

Examples:

  • MIDI Code: 60 (Middle C); Result: Floor(60 / 12 - 1) = 4
  • MIDI Code: 21 (A0): Result: Floor(21 / 12 - 1) = 0

@aik099
Copy link
Author

aik099 commented Jun 8, 2018

Changing octave + 2 into octave + 1 in the WebMidi.prototype.noteNameToNumber method should fix the playNote method:

old: var result = ((octave + 2) * 12) + semitones;
new: var result = ((octave + 1) * 12) + semitones;

Examples:

  • Note: C4 (note "C", octave: 4); MIDI Code: ((4 + 1) * 12) + 0 = 60
  • Note: A0 (note "A", octave: 0): MIDI Code: ((0 + 1) * 12) + 9 = 21

@aik099
Copy link
Author

aik099 commented Jun 8, 2018

I can send a PR with above described changes as well.

@aik099
Copy link
Author

aik099 commented Jun 8, 2018

I've also found https://www.midi.org/forum/830-midi-octave-and-note-numbering-standard article where a lot confusion happens between fact what octave Middle C is coming from: C3, C4, C5 or something else.

We can make octave numbering offset configurable, in WebMidi.enable method (3rd parameter) but I'm afraid, that it will only make things worse, because it would allow for negative octave numbers as a result and no standard at the end.

@djipco
Copy link
Owner

djipco commented Jun 9, 2018

We can make octave numbering offset configurable, in WebMidi.enable method (3rd parameter) but I'm afraid, that it will only make things worse, because it would allow for negative octave numbers as a result and no standard at the end.

As far as I'm concerned, having negative octave numbers is not an issue and should not be considered a criteria. I actually think it would be a good idea to allow for on-the-fly system-wide octave offsetting. However, I'd like to make an informed decision here. Here's what we know:

  • The scientific pitch notation considers middle C to always be C4.

  • The MIDI standard does not define a pitch for middle C but it does consider middle C to be note number 60. Different manufacturers have assigned middle C to various octaves/pitches (usually C3, C4 or C5).

  • This article and this other one say that General MIDI requires note number 69 to be pitched at A440. I could not find official confirmation for this. This would make middle C to be C4.

  • The MIDI Tuning Standard states that note number 69 should be tuned at 440Hz by default, which would also make middle C (60) to be C4.

Given that, I would tend to agree to go with C4. However, this is a breaking change. What are people's views on this?

@djipco djipco self-assigned this Jun 10, 2018
@djipco djipco added the Type: enhancement Request for improvement label Jun 10, 2018
@djipco djipco added this to the 2.1 milestone Jun 10, 2018
@francoisgeorgy
Copy link
Contributor

Hi,

I always thought that middle C = C4 was a standard or at least a "de facto" standard. I'm quite surprised this is not really the case.

But, it seems that there is kind of a consensus toward that C4 value. So, for me this is the best candidate. I vote for it :-)

Sorry not to be able to bring more to the discussion.

@aik099
Copy link
Author

aik099 commented Jun 11, 2018

@francoisgeorgy , maybe you can tweet about it. My twitter followers are developers mostly, so I'm not even trying 😉 to tweet about it.

@djipco
Copy link
Owner

djipco commented Jun 18, 2018

I always thought that middle C = C4 was a standard or at least a "de facto" standard. I'm quite surprised this is not really the case.

You would find many people to disagree with that statement. For example, various pieces of hardware and software treat MIDI note 60 (middle C) as C3. Some notable examples that come to mind are Yamaha's hardware, Apple's Logic, Steinberg's Cubase, etc. Also, many people were taught middle C = C3 in music school.

Given the variations in practices and opinions on this matter, I think the best course of action is to align with some sort of standard. In this case, the most obvious one seems to be scientific pitch notation.

So, I'm going to change WebMidi.js to define middle C as C4. However, this change is going to be accompanied by a way to globally modify this behaviour.

@djipco
Copy link
Owner

djipco commented Jun 18, 2018

As discussed, middle C has been configured to be C4 in the latest /src version. A new setting called WebMidi.octaveOffset is also available to globally offset octave numbering. This means you can use WebMidi.octaveOffset = -1 to maintain compatibility with existing projects.

Please test it out using /src/webmidi.js and report back here. I'll publish it to NPM once I have confirmation that it works properly.

@aik099
Copy link
Author

aik099 commented Jun 18, 2018

I'll added a note about code in 5a0e0ae commit directly, because you're not using a PR.

Will test shortly.

@aik099
Copy link
Author

aik099 commented Jun 19, 2018

Tested, that behavior explained in code above WebMidi.octaveOffset setting works as described:

  • the C4 becomes C5 if I use +1 offset
  • the C4 becomes C3 if I use -1 offset

@djipco
Copy link
Owner

djipco commented Jun 21, 2018

Fixed in release 2.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: enhancement Request for improvement
Projects
None yet
Development

No branches or pull requests

3 participants