opensheetmusicdisplay / opensheetmusicdisplay

OpenSheetMusicDisplay renders sheet music in MusicXML format in your web browser based on VexFlow. OSMD is brought to you by PhonicScore.com.
https://opensheetmusicdisplay.org
BSD 3-Clause "New" or "Revised" License
1.46k stars 289 forks source link

OSMD renders sus chords as omit3 #930

Closed infojunkie closed 3 years ago

infojunkie commented 3 years ago

If you compare the MuseScore rendering of the attached MusicXML file MuseScore to the same rendering by OSMD OSMD

You will notice that the b13sus4 chords are correctly interpreted by MuseScore, while OSMD renders them as b13(omit3) - missing the combination of (omit3, add4) which would spell out "sus". Here's the relevant MusicXML fragment:

      <harmony>
        <root>
          <root-step>E</root-step>
          <root-alter>-1</root-alter>
        </root>
        <kind text="13sus" use-symbols="no">dominant-13th</kind>
        <degree print-object="no">
          <degree-value>3</degree-value>
          <degree-alter>0</degree-alter>
          <degree-type>subtract</degree-type>
        </degree>
        <degree print-object="no">
          <degree-value>4</degree-value>
          <degree-alter>0</degree-alter>
          <degree-type>add</degree-type>
        </degree>
      </harmony>

As per https://github.com/w3c/musicxml/issues/352, there is some ambiguity as to how MusicXML represents sus chords, so care must be exercised in the interpretation of the format.

infojunkie commented 3 years ago

For reference, I am embedding OSMD in my own Web demo to generate the above screenshot.

sschmidTU commented 3 years ago

This is probably a similar issue to https://github.com/opensheetmusicdisplay/opensheetmusicdisplay/issues/590#issuecomment-750419659, the XML says "subtract", so we display "omit". This is in measure 17 (as you wrote):

      <harmony>
        <root>
          <root-step>E</root-step>
          <root-alter>-1</root-alter>
        </root>
        <kind text="13sus" use-symbols="no">dominant-13th</kind>
        <degree print-object="no">
          <degree-value>3</degree-value>
          <degree-alter>0</degree-alter>
          <degree-type>subtract</degree-type>
        </degree>
        <degree print-object="no">
          <degree-value>4</degree-value>
          <degree-alter>0</degree-alter>
          <degree-type>add</degree-type>
        </degree>
      </harmony>

We should probably handle these tags differently, or at least give the option to display them differently, as suggested in https://github.com/opensheetmusicdisplay/opensheetmusicdisplay/issues/590#issuecomment-750419659 (same link as above).

sschmidTU commented 3 years ago

missing the combination of (omit3, add4) which would spell out "sus"

That's probably the crux, we need to check for things like this and parse it as "sus".

infojunkie commented 3 years ago

I can recommend the library chord-symbol to perform chord logic on your behalf - it's worked well for me and the maintainer is quite helpful.

sschmidTU commented 3 years ago

That sounds good! Though we may just correct it on our side as we basically built a chord reader/analysis system ourselves, but if the library is lightweight and spares us a lot of work for improvements, that may be a very good idea.

infojunkie commented 3 years ago

Yes, chord-symbol does not have a MusicXML import function (i.e. accept a MusicXML harmony fragment and output a "good" chord name). But the next best thing would be to construct an arbitrary chord name and pass it to chord-symbol for normalization.

Maybe @no-chris can advise here?

no-chris commented 3 years ago

Hello!

The issue I see with music XML is that while it allows to properly describe the intervals of a chord using kind + degree, it would need a lot more kind values to allow “proper” rendering of all possible chords.

Already using <kind text=“13sus”>dominant-13th</kind> seems like a hack since the text attribute is only supposed to describe how the kind should be spelled in a score and not how the kind should be spelled based on the combination of the kind and the altered degrees that are described below.

Though we may just correct it on our side

I would advise to work with a fixed set of known chords like I did for chord-symbol, using dictionaries like The real book for ex.

as we basically built a chord reader/analysis system ourselves

Are you referring to this? https://github.com/opensheetmusicdisplay/opensheetmusicdisplay/blob/acd0b4ca6ec44e148cc9cca392d6b05b28e21bd3/src/MusicalScore/VoiceData/ChordSymbolContainer.ts#L38 (btw, congrats for the extraordinary readability of the code 👏🏻 )

If I understand well you rely on the text attribute to build the chord descriptor, which will always be challenging for the reason explained above and require edge case handling.

As for using chord symbol as @infojunkie kindly suggested, I can see at least 2 blockers for now:

  1. chord-symbol does not support double accidentals
  2. some music xml kinds cannot be parsed today (Tristan, the special sixth…)

Maybe I could add those to the backlog if you have interest in using chord-symbol, let me know. I'm curious what is your take on if the library is lightweight?

infojunkie commented 3 years ago

Already using dominant-13th seems like a hack

Yes, the kind/@text attribute can contain any text and is not meant to be reliably used for parsing. However, the combination of kind value + list of degrees does produce an accurate and unambiguous list of chord degrees, which fed to chord-symbol can then produce a reliable chord name. The chord naming itself is subject to parameterization, e.g. "Fm" vs. "F Min" vs. "F-" and that's what OSMD can ideally expose as an API - hopefully via naming standards that are understood by all (users + modules).

On the other hand, OSMD could also trust kind/@text as being the reference of how to display the chord name - without any further processing.

infojunkie commented 3 years ago

Noting that whatever happens here, it is needed to coordinate with the existing PR that deals with chord degrees.

sschmidTU commented 3 years ago

what is your take on if the library is lightweight?

If the library is >100KB in our build, we probably won't use it, since OSMD is only 1.01MB right now. But i'm guessing it has a very small build size anyways.

as we basically built a chord reader/analysis system ourselves

Are you referring to this? [ChordSymbolContainer.ts]

Also ChordSymbolReader. https://github.com/opensheetmusicdisplay/opensheetmusicdisplay/blob/acd0b4ca6ec44e148cc9cca392d6b05b28e21bd3/src/MusicalScore/ScoreIO/MusicSymbolModules/ChordSymbolReader.ts#L10

hartman42 commented 3 years ago

Yeah, I saw that too... looking on npm it has an unpacked size of 1.34 MB.

Those N.C. that are getting rendered as B chords might also be a problem...

If they all get the chord kind of "none", I'm thinking we could just add that chord kind to the list and then check for it first when we calculate the chord text and return it there at the top.

Thoughts?

no-chris commented 3 years ago

The build has currently a size limit of 68 kb

https://github.com/no-chris/chord-symbol/blob/39e7cff1806e3677d7c89ba15a6d6116ed8b349a/package.json#L64

sschmidTU commented 3 years ago

That sounds alright. The actual build integrated with webpack into OSMD is probably even smaller.

sschmidTU commented 3 years ago

@hartman42

Those N.C. that are getting rendered as B chords might also be a problem...

If they all get the chord kind of "none", I'm thinking we could just add that chord kind to the list and then check for it first when we calculate the chord text and return it there at the top.

Oh, right, N.C. means 'No Chord'. Yes, simply checking for that (first) and returning 'N.C.' should be sufficient i think.

Ideally we'll make the chord text customisable here as well, via EngravingRules.setChordSymbolLabelText(ChordSymbolEnum.nochord, string) and add the nochord enum. But that's pretty simple, I can add that afterwards.

Thank you very much for your continued work on this, this is looking to be a great improvement!

no-chris commented 3 years ago

Yes, the kind/@text attribute can contain any text and is not meant to be reliably used for parsing. However, the combination of kind value + list of degrees does produce an accurate and unambiguous list of chord degrees, which fed to chord-symbol can then produce a reliable chord name.

That sounds like a valid approach. The normalized version of the chord is built only based on the intervals and the intents that can easily be derived from the music xml kind. In other words, chord symbol can parse an harmony object and output a normalized chord name only from the kind and degrees without too much effort. I've been playing locally with a POC and the effort would be reasonable.

Noting that whatever happens here, it is needed to coordinate with the existing PR that deals with chord degrees.

It looks like this PR already does a lot of what chord-symbol would be used for, so I'm not sure how relevant the integration would be in the end.

sschmidTU commented 3 years ago

Yes, I would probably prefer not to add another library, and just implement the functionality we would need/use from chord-symbol.

matt-uib commented 3 years ago

One more thought on this: as also happens for other data in the musicxml, the kind+degrees can be given just wrong. I would also guess, that in some of the engraving software you just type in a text anyway and the software will then parse and convert to kind+degrees in musicxml... So: Typically kind/@text has been verified by someone or is the way some musician wants to show it. So i would not throw it away, but rather use this for final display and maybe it is also even better to also try to parse it. I would first parse the given text. if that doesn't work, i would go for kind and degrees. But this could also be done later in a next version. Going for kind+degrees now and improving the parsing there is of course great already.

no-chris commented 3 years ago

Typically kind/@text has been verified by someone or is the way some musician wants to show it.

Yes and no. If I type Ami7(b9,#9) it is very likely that whichever software generates the musicXml file will:

In that case, kind/@text is not the way the musician wants to show it. kind/@text per se is more likely to be "wrong" (or incomplete) every time you have altered/added/omitted degrees.

This is, again, because of the ambiguity of the kind/@text attribute, which is supposed to describe how to spell the kind and, sometimes, some degrees as well, and in that case, those degrees should have the 'print-object' attribute set to no... For what I can see this does not seems to be widely understood and supported.

hartman42 commented 3 years ago

Anyone seeing a problem with just removing chord degrees with the print-object attribute set to "no"?

sschmidTU commented 3 years ago

That sounds reasonable to me. Usually print-object = "no" means the element should not be displayed (e.g. a note would be invisible), so that would seem to eapply here too.

Actually, i just checked. and the MusicXML documentation says this for degree:

The print-object attribute can be used to keep the degree from printing separately when it has already taken into account in the text attribute of the kind element.

http://usermanuals.musicxml.com/MusicXML/MusicXML.htm#EL-MusicXML-degree.htm

But since programs don't seem to agree how to unifiedly export chords, i think we have a bit of leeway here.

no-chris commented 3 years ago

@hartman42 more edge cases for you to handle, if you feel like it 😄

hartman42 commented 3 years ago

@sschmidTU - what if we use the text attribuite if it's there and hide the degrees with print-object = "no"... if the text attribute isn't there, then we do what we're doing... I'm inclined to set this up with an engraving rule even. something like useTextAttributeForChordKind? Set it true to use the chord kind/@text attribute if it's there. Set it to false to ignore it.

@no-chris - I can set that up in MuseScore 3.0... it might be nice if we could get it exported with software that's follows the musicxml spec a little better? something like... I don't know... Sibelius? I don't have much experience with different notation software.

no-chris commented 3 years ago

If a software "properly" generates the harmony object, meaning using non-printable degrees, then you are good with simply discarding those degrees when building the chord name.

The problem is when this degree/@print-object attribute is missing. In that case, you need to rely on heuristics to avoid displaying chord such as C69(add9) or CmiMa13(add9, 11, 13), as you are currently doing for sus4 and sus2

Those examples come from gaps I've spotted while working on a ChordSymbol to MusicXml converter. There might be more, but those already cover the main use cases I guess.

Depending on the requirements, you could also add alt to the list, but that one is a bit trickier.

sschmidTU commented 3 years ago

It seems like no-chris is more knowledgable here and what he's saying sounds reasonable to me. Though in general, having EngravingRules to choose between multiple sensible options for displaying is always nice to have.

hartman42 commented 3 years ago

I probably went a little overboard here but... here's what I came up with...

I created a class for custom chord kinds... the idea is that we can add an alternate name for the chord kind with a combination of the chord kind and degrees (adds, alters, and subtracts). It then renames the chord kind to the alternate name if the chord matches everything and hides the degrees.

So, for a CmiMa9 situation that gives us this XML:

      <harmony print-frame="no">
        <root>
          <root-step>C</root-step>
          </root>
        <kind text="mmaj7" parentheses-degrees="yes">major-minor</kind>
        <degree>
          <degree-value>9</degree-value>
          <degree-alter>0</degree-alter>
          <degree-type>add</degree-type>
          </degree>
        </harmony>

, I have a custom chord kind with an alternate name of "m(maj9)", chord kind as "majorminor", and "9" in the "adds" degrees array. This changes the chord kind to the alternate name so... This: image Becomes this: image

Here's what I have set up so far the engraving rules:

        // addCustomChordKind(alternateName, chordKindText, adds, alters, subtracts)
        this.addCustomChordKind("alt", "major", ["#5", "b9", "#9"], ["b5"], []);
        this.addCustomChordKind("7alt", "dominant", ["#5", "b9", "#9"], ["b5"], []);
        this.addCustomChordKind("7sus4", "dominant", ["4"], [], ["3"]);
        this.addCustomChordKind("7sus4", "suspendedfourth", ["7"], [], []);
        this.addCustomChordKind("9sus4", "dominantninth", ["4"], [], ["3"]);
        this.addCustomChordKind("9sus4", "suspendedfourth", ["9"], [], []);
        this.addCustomChordKind("11sus4", "dominant11th", ["4"], [], ["3"]);
        this.addCustomChordKind("11sus4", "suspendedfourth", ["11"], [], []);
        this.addCustomChordKind("13sus4", "dominant13th", ["4"], [], ["3"]);
        this.addCustomChordKind("13sus4", "suspendedfourth", ["13"], [], []);
        this.addCustomChordKind("7sus2", "dominant", ["2"], [], ["3"]);
        this.addCustomChordKind("7sus2", "suspendedsecond", ["7"], [], []);
        this.addCustomChordKind("9sus2", "dominantninth", ["2"], [], ["3"]);
        this.addCustomChordKind("9sus2", "suspendedsecond", ["9"], [], []);
        this.addCustomChordKind("11sus2", "dominant11th", ["2"], [], ["3"]);
        this.addCustomChordKind("11sus2", "suspendedsecond", ["11"], [], []);
        this.addCustomChordKind("13sus2", "dominant13th", ["2"], [], ["3"]);
        this.addCustomChordKind("13sus2", "suspendedsecond", ["13"], [], []);
        this.addCustomChordKind("m(maj9)", "majorminor", ["9"], [], []);
        this.addCustomChordKind("m(maj11)", "majorminor", ["11"], [], []);
        this.addCustomChordKind("m(maj13)", "majorminor", ["13"], [], []);
        this.addCustomChordKind("69", "majorsixth", ["9"], [], []);
        this.addCustomChordKind("mi69", "minorsixth", ["9"], [], []);

Since we have the ability now to rename the text returned for each chord kind, I went ahead and set up a function that allows us to rename these as well. That way we can change it to this: image using: osmd.EngravingRules.renameCustomChordKind("m(maj9)","−Δ9")

Since this is my first real foray into typescript - not to mention I could probably count on one hand the number of class modules I've written - any and all feedback on this PR #933 would be greatly appreciated!

sschmidTU commented 3 years ago

The ability to set custom chord names (kinds) is great! I assume this means the PR #933 is more or less complete/functional now? Then i'll be happy to review. This system of custom chords seems very reasonable and not at all overblown.

Further code discussion in the PR.

sschmidTU commented 3 years ago

fixed in #933.

image