Vexflow: Problem with chord accidental on wrong line.

[thinkingspace]

Based on my understanding the following code should add a flat to the A, but it is rendered on the B

var notes = [
    new Vex.Flow.StaveNote({ keys: ["B/4", "A/4"], duration: "q",  stem_direction: 1})
];  

//Add flat to the A  
notes[0].addAccidental(1, new Vex.Flow.Accidental('b'));

[Silverwolf90]

Late response, but I can provide some insight to this confusing bug. It's not really super clear to the user that this happens, but StaveNote keys are sorted by pitch after construction. So actually, you want to the add the accidental to key 0 instead of 1.

notes[0].addAccidental(0, new Vex.Flow.Accidental('b'));

[mscuthbert]

Should this be closed?

[0xfe]

Ugh... this is not a bug, but it's still terrible and it should be fixed to something more intuitive.

[mscuthbert]

Would a warning on keys not in order be sufficient?

[0xfe]

Maybe. Here's what I think are the possible options:

1) Allow indexing based on original order (in addition to current methods) -- This could work, but I'm not a fan of adding more unnecessary complexity to the API. We could change the behavior of the current methods, but that would quietly break existing users (late).
2) Don't sort keys -- Might be too invasive, because it's unclear how much of the codebase depends on key ordering.
3) Warn if not in order -- This would help, but I'm not sure that it's sufficient since warnings are easy to ignore, and the resulting functionality is almost certainly incorrect.
4) Exception if not in order -- This seems like a good mix of 1) and 3). You'll have to willfully ignore the exception. It's still a behavioral change, which I try to avoid, but might be acceptable since it's only breaking users that are using it incorrectly.

I think 4) might be the best way to go.

[0xfe]

Another good option would be to overload addAccidental as so:

notes[0].addAccidental("A/4", new Vex.Flow.Accidental('b'));

[mscuthbert]

I think #4 is the best option in the longer run. I would suggest going with option 3 immediately and then switching to option 4 one version later, so that people who have been giving their keys unsorted can have a version letting them know that their code will break in the next upgrade. I always think that when possible a one-version (3 month?) deprecation period is a good thing to have since there could be uses of VexFlow where someone is never adding accidentals to chords and therefore not seeing the bug.

Overloading addAccidental seems like a good plan iff it also allows for adding accidentals to keys that don't exist in the chord.

[0xfe]

Overloading addAccidental seems like a good plan iff it also allows for
adding accidentals to keys that don't exist in the chord.

Why would one need to add accidentals for keys that don't exist in the
chord?

Mohit Muthanna [mohit (at) muthanna (uhuh) com]

[mscuthbert]

it's common for cluster chords or for microtonal accidentals (where the microtonal inflection applies to all the notes of the chord) and sometimes for musica ficta (specifying that a note's accidental is implied.

[0xfe]

Okay, implemented option 3, which is least likely to break users. This displays a warning on the console if keys are not provided in order.

[stephaneeybert]

To replicate sorting also be done on a custom data model I would need to know what is meant with "sorting by pitch".

I have a chroma value as string and an octave value as number. Can I render the same pitch with these two ?

I need to sort my notes to replicate the sorted keys.

Or can I pass in the dotted boolean when creating a multi keys stave note ?

[rowild]

It seems that the re-write of a chord is triggered, when there are at least 5 notes in minor/major second distance. A rendering of tonal.js's h11 chord shows a wrong result. Would love to know how to fix this. Any ideas?

[gristow]

@stephaneeybert, you need to sort the keys before creating a StaveNote and give a sorted array to the constructor. The sorting used by VexFlow is sort by lowest-to-highest placement on the staff. So, E# should be sorted lower than Fb.

@rowild it seems you may be asking about a different issue. Can you post pictures & or code for the problem you're describing as a new issue?

[rowild]

@gristow Thank you for your feedback! - I prepared a codesandbox here and a screenshot:

https://codesandbox.io/s/list-of-tonaljs-chords-ntjye

If you hit a key (e.g. "C4") and the select the chrod "h11" (m11b5), you can examine the results. In the case of "C4", there should be a "g flat" somewhere, but isn't. In the case of "F4" there should be an "e-flat" etc.

I thought this problem would be related to this issue here, which is why I posted my comment. If not and you still think it is an issue, please let me know, what you want me to do (opening a new issue...)

Thank you and stay save!!!

[rowild]

EDIT: Actually - I think my calculation with the accidentals is not correct... need to check that... refering to the example screenshot: The "D4" and "F4" seem to be placed to a position in the array that comes before the notes that are in the 5th register. This is probably the reason why I get the warning message: "Warning: Unsorted keys in note will be sorted. See https://github.com/0xfe/vexflow/issues/104 for details."

How would I have to write an array that gets sorted correctly?

[gristow]

Yes, that's exactly it: You'll need to sort the notes into pitch order (ignoring accidentals), in this case as:

• C4-D4-Eb4-F4-Bb4-Gb5

(Although, I have to say... that's a pretty gnarly spacing for an 11b5 chord!)

[rowild]

@gristow Do you know if I have to write my own function to sort the pitches or if VexFlow provides some helper methods? So far I have been unsuccessfull...

And I agree: the voicing is ... different...

EDIT: I think I need to change my approach coming from tonal.js and not use note names, but some other parameter for sorting. it also looks as if the mentioned chord h11 is the only one, that is wrongly sorted...

[0xfe]

EDIT: corrected

You can use the keyProperties table to get the values for the pitches -- here's a sorting example:

function getValue(note) {
  parts = note.split("/");
  return Vex.keyProperties.note_values[parts[0].toUpperCase()].int_val + (parseInt(parts[1]) * 12);
}

notes = ['a/4', 'a/3', 'b/4']
notes.sort((a, b) => getValue(a) - getValue(b)))
// output: ["a/3", "a/4", "b/4"]

[0xfe]

Also note that Array.sort is not stable ordering, so if you have multiple pitches with the same value, the ordering is not deterministic.

[rowild]

@0xfe Thank you for sharing the code, but I am not sure, if it works correctly? I keep getting incorrect results...

The code is implemented in this codesandbox:
https://codesandbox.io/s/list-of-tonaljs-chords-ntjye
in the NoteDisplay.vue component, in case you want to have a look.

I should mention that the m11b5 (h11) chord in the tonal.js library seems to be the only one that is ordered incorrectly.

[rowild]

I am stuck! Is the algorithm wrong or am I doing something wrong!
Please advice!

[0xfe]

Your line 226 in NoteDisplay.vue should be:

return (VF.keyProperties.note_values[parts[0].toUpperCase()].int_val + parseInt(parts[1], 10) * 12);

My fault, the code snippet I give you was wrong. I fixed it. Basically you're sorting by value, where value = octave * 12 + key.int_val

[SalahAdDin]

It seems so interesting.