Distinction of start and stop in HammerOn/PullOff

[louisbigo]

music21 version

7.0.1

In guitar tabs, a hammer-on concerns two notes : the origin note and the destination note. This distinction is done in MusicXML with the attribute type :

<technical>
  <hammer-on number="1" type="start">H</hammer-on>
  <string>3</string>
  <fret>5</fret>
</technical>

...

<technical>
  <hammer-on number="1" type="stop"/>
  <string>3</string>
  <fret>7</fret>
</technical>

The m21 class HammerOn (in articulations.py) does not make this distinctions. I see two possibilities to add this distinction in m21: creating subclasses HammerOnStart and HammerOnStop that inherit from HammerOn (maybe the clean way) or adding an attribute type to HammerOn (to fit with the MusicXML type attribute):

The same apply for PullOff articulations.

Ultimately, there should be a way to identify that two hammer-on (one start and one stop) are linked together (maybe with note IDs ?) but that probably goes beyond this issue.

[jacobtylerwalls]

I think there's a third possibility -- make them spanners. Then it's easier to track the links, and also to generate the note-by-note tags on xml export without requiring users to generate or maintain Python objects on every note. (They do kind of look like slurs in guitar tabs.) Thoughts?

Related maybe to #249

[mscuthbert]

HammerSpanner sounds like the right approach.

[louisbigo]

A HammerOnSpanner (and a PullOffSpanner) subclass in spanner.py sounds good to me too. From my understanding, there's no need to override or add any methods as it is pretty similar to standard spanners. Maybe one thing : I think a HammerOnSpanner should not include more than two spanned elements. But maybe there's no need to check this for now.

class HammerOnSpanner(Spanner):
    '''
    A hammer-on represented as a spanner between two Notes.
    '''

    def __init__(self, *arguments, **keywords):
        super().__init__(*arguments, **keywords)

[jacobtylerwalls]

IMO let's revisit adding a limit after you start using the feature, and maybe you can report as to the incidence of overfilled spanners in real files. (Combination trills?) But yes, further down the road.

Little point: if your constructor does nothing besides call the one on super(), you can just omit it with pass.

Do you have time to provide a patch for musicxml translation? @mscuthbert , can we remove the existing placeholder HammerOn class in the articulations module as a small incompatible change, or do we need to deprecate for one version?

A few tips, in case helpful. (but of course, start your own investigation wherever you like!) On import, I would try using xmlOneSpanner() in some fashion. On export, you will probably end up removing HammerOn and PullOff from xmlObjects.TECHNICAL_MARKS to avoid the placeholder we created in #780.

[louisbigo]

Sorry it took me a while to come back on that issue.

Import: using xmlOneSpanner seems indeed like the good option. I just had to insert the following lines in xmlNotationsToSpanners :

for mxObj in mxNotations.findall('technical/hammer-on'):
    hammerOn = self.xmlOneSpanner(mxObj, n, articulations.HammerOn)

for mxObj in mxNotations.findall('technical/pull-off'):
    pullOff = self.xmlOneSpanner(mxObj, n, articulations.PullOff)

export: the best option I found was to create a new function objectAttachedSpannersToTechnicals analogous to objectAttachedSpannersToNotations that would be called in noteToNotations (see below). The reason for this is that <hammer-on> tags need to be inserted within <notations><technical>…</technical></notations>.

tests: It looks like m21 includes special test files, like testPrimitive.spanners33a. Should we add a measure with HO/PO in this file, or would it be better to create a dedicated tab test file to stay consistent ?

def objectAttachedSpannersToTechnicals(self, obj, objectSpannerBundle=None):
    '''
    return a list of <technical> from spanners related to the object that should appear
    in the technical tag of the notations tag (hammer-on, pull-off etc.)

    >>> n0 = note.Note('C')
    >>> n1 = note.Note('D')
    >>> n2 = note.Note('F')
    >>> n3 = note.Note('E')
    >>> hammerOn01 = articulations.HammerOn([n0,n1])
    >>> pullOff23 = articulations.PullOff([n2,n3])
    >>> m = stream.Measure()
    >>> m.insert(0, hammerOn01)
    >>> m.insert(0, pullOff23)
    >>> m.append(n0)
    >>> m.append(n1)
    >>> m.append(n2)
    >>> m.append(n3)
    >>> mex = musicxml.m21ToXml.MeasureExporter(m)
    >>> out = mex.objectAttachedSpannersToTechnicals(n0, m.spannerBundle)
    >>> out
    [<Element 'hammer-on' at 0x102857f40>]
    >>> mex.dump(out[0])
    <hammer-on number="1" type="start">H</hammer-on>
    >>> out = mex.objectAttachedSpannersToTechnicals(n3, m.spannerBundle)
    >>> mex.dump(out[0])
    <pull-off number="1" type="stop" />


    '''
    technicals = []
    if objectSpannerBundle is not None:
        sb = objectSpannerBundle
    else:
        sb = self.objectSpannerBundle

    if not sb:
        return technicals

    for su in sb.getByClass('HammerOn'):
        mxHammerOn = Element('hammer-on')
        if su.isFirst(obj):
            mxHammerOn.set('type', 'start')
            mxHammerOn.text = 'H'
        elif su.isLast(obj):
            mxHammerOn.set('type', 'stop')
        else:
            continue
        mxHammerOn.set('number', "1")
        technicals.append(mxHammerOn)

    for su in sb.getByClass('PullOff'):
        mxPullOff = Element('pull-off')
        if su.isFirst(obj):
            mxPullOff.set('type', 'start')
            mxPullOff.text = 'P'
        elif su.isLast(obj):
            mxPullOff.set('type', 'stop')
        else:
            continue
        mxPullOff.set('number', "1")
        technicals.append(mxPullOff)

    return technicals

---

def noteToNotations(self, n, noteIndexInChord=0, chordParent=None):
    
    ...

    if isSingleNoteOrFirstInChord and chordParent is not None:
        notations.extend(self.objectAttachedSpannersToNotations(chordParent))
    elif chordParent is not None:
        pass
    else:
        notations.extend(self.objectAttachedSpannersToNotations(n))

    # TODO: slur
    # TODO: glissando
    # TODO: slide

    spannerTechnicals = self.objectAttachedSpannersToTechnicals(n)
    if spannerTechnicals:
        if mxTechnicalMark is None:
            mxTechnicalMark = Element('technical')
        mxTechnicalMark.extend(spannerTechnicals)

    for x in (mxArticulations,
              mxTechnicalMark,
              mxOrnaments):
        if x:
            notations.append(x)

    # TODO: dynamics in notations
    # TODO: arpeggiate
    # TODO: non-arpeggiate
    # TODO: accidental-mark
    # TODO: other-notation
    return notations

[jacobtylerwalls]

Wonderful, thanks for the update. I'd be glad to look at a PR.

Your solution for export looks right-track, since having to insert spanner logic into articulationToXmlTechnical() could work but wouldn't be very readable. Is there a reason your changes only target the else branch in your diff, not the branch above where if isSingleNoteOrFirstInChord and chordParent is not None:?

And for tests: I notice we already have a test file with hammer-ons and pull-offs in testPrimitive.notations32a, which should be sufficient to test your changes. If you'd like to make sure we can read this from a guitar file context, perhaps you could insert some <notations><technical> tags into https://github.com/cuthbertLab/music21/blob/master/music21/musicxml/testTab.xml, but I don't think it's strictly necessary.

[jacobtylerwalls]

We can discuss more on the PR, but I would be inclined to leave HammerOn in the articulations module and just let it inherit from Spanner in addition to FretIndication. No reason to call it HammerOnSpanner, since we don't have SlurSpanner etc., and TremoloSpanner only exists to distinguish it from a single-note tremolo, but it doesn't sound like we are aiming to support two kinds of HammerOn(s).

[louisbigo]

Good point, I realize indeed that a hammer-on can take place on a note even within a chord so it should not be conditioned by chordParent. So I think I’ll juste have to move it outside the if statement (I updated the code above).

Ok too for leaving HammerOn in articulations.py with double inheritance :

class HammerOn(FretIndication, spanner.Spanner):
    '''
    A hammer-on represented as a spanner between two Notes.
    '''
    pass

For some reason the methods isFirst and isLast inherited from Spanner don’t work anymore with the double inheritance, I’ll have to check that.

Thanks also for pointing the notations32a test file, I’ll try to figure out some relevant tests in the PR.