More accurate spanner stop/start on musicxml import

[gregchapman-dev]

Import of <direction> spanners now always generate SpannerAnchors, and take direction@offset into account. This also fixes some bugs where self.nLast and "whatever the next GeneralNote is" weren't the right things to use.

Export of SpannerAnchor-based spanners produces "extra jumpy" MusicXML files (lots more <backup> and <forward> elements), which are valid, but unusual, and import of those "extra jumpy" files needed some fixing here as well.

[coveralls]

coverage: 93.089% (+0.02%) from 93.069% — gregchapman-dev:gregc/moreAccurateSpannerStartStop into cuthbertLab:master

[gregchapman-dev]

Time to work on some test coverage.

[gregchapman-dev]

@mscuthbert This is ready for review.

[mscuthbert]

Hi Greg -- sorry to take some time on this. I'm noticing some changes on Crescendo attachment that I want to discuss a bit more -- returning to conversation.

[mscuthbert]

For stubbed out, say "deprecated in v9.7 (not needed, so does nothing. To be removed in v10.0)"

[mscuthbert]

As it should be I'm seeing a number of additional SpannerAnchors in places where they weren't before. However, some of them seem excessive or incorrect:

Take this from corpus.parse('verdi'), which is MusicXML 2.0 originally encoded by MuseScore 1.1:

      <note default-x="105.25" default-y="-10.00">
        <pitch>
          <step>D</step>
          <octave>5</octave>
          </pitch>
        <duration>12</duration>
        <voice>1</voice>
        <type>eighth</type>
        <stem>down</stem>
        <staff>1</staff>
        <beam number="1">continue</beam>
        <notations>
          <slur type="stop" number="1"/>
          </notations>
        <lyric number="1">
          <syllabic>begin</syllabic>
          <text>piu</text>
          </lyric>
        </note>
      <direction placement="below">
        <direction-type>
          <wedge type="crescendo"/>
          </direction-type>
        <staff>1</staff>
        </direction>
      <note default-x="157.33" default-y="-10.00">
        <pitch>
          <step>D</step>

with corresponding stop:

    <measure number="21" width="183.77">
      <note default-x="12.00" default-y="5.00">
        <grace slash="yes"/>
        <pitch>
          <step>G</step>
          <alter>-1</alter>
          <octave>5</octave>
          </pitch>
        <voice>1</voice>
        <type>eighth</type>
        <accidental>flat</accidental>
        <stem>up</stem>
        <staff>1</staff>
        </note>
      <direction placement="below">
        <direction-type>
          <wedge type="stop"/>
          </direction-type>
        <staff>1</staff>
        </direction>
      <note default-x="48.84" default-y="-20.00">
        <pitch>
          <step>B</step>
          <alter>-1</alter>

laDonnaEMobile.musicxml.zip

Despite two possible staves, this seems like a clear case of the wedge being encoded to go from D to Gb (it probably should've been encoded to go after the next chord, but it wasn't...). Here's how Music21 currently encodes it and some nearby spanners:

<music21.dynamics.Crescendo <music21.note.Note D><music21.note.Note G->>
<music21.spanner.Slur <music21.note.Note F><music21.note.Note F>>
<music21.spanner.Slur <music21.chord.Chord B-4 G5><music21.note.Note F>>
<music21.dynamics.Diminuendo <music21.note.Rest 16th><music21.note.Rest 16th>>

Here's how it's encoded after this PR:

<music21.dynamics.Crescendo <music21.spanner.SpannerAnchor at 1.0><music21.spanner.SpannerAnchor at 0.0>>
<music21.spanner.Slur <music21.note.Note F><music21.note.Note F>>
<music21.spanner.Slur <music21.chord.Chord B-4 G5><music21.note.Note F>>
<music21.dynamics.Diminuendo <music21.spanner.SpannerAnchor at 0.75><music21.spanner.SpannerAnchor at 1.5>>

and that notes are not part of the spanned elements at all, even after calling .fill()

>>> cr.getSpannedElements()
[<music21.spanner.SpannerAnchor at 1.0>, <music21.spanner.SpannerAnchor at 0.0>]
>>> cr.fill()
>>> cr.getSpannedElements()
[<music21.spanner.SpannerAnchor at 1.0>, <music21.spanner.SpannerAnchor at 0.0>]

Though fill doesn't seem to be working properly even on master, drat...but it never really worked.

My thought was that SpannerAnchors would only be created for places that there wasn't a logical note to attach directions to. They were supposed to be an object for unusual cases not a standard part of music21 that users would encounter anytime they were working with standard objects like crescendos or diminuendos unless the crescendo did something like end on beat 3 of a whole note. Was I mistaken?

[gregchapman-dev]

I was being a bit lazy, perhaps? I can use self.nLast as the start element of a direction spanner, but only if it has the correct offset. Same with "whatever the next GeneralNote is", but that logic will be a bit trickier, since when that next note shows up is potentially a fair bit later (and in a few different places in the code), and we'll need to have the "correct offset" info saved off somewhere, so we can (1) check it, and (2) create and insert a SpannerAnchor at the correct offset if necessary.

I'm not sure what happened to fill(), but I will say that it is used successfully for Ottavas all the time (so that transpose() can work properly), with a Stream argument (if you don't pass in a Stream, it falls back to using getFirst().activeSite). So perhaps the no-stream-argument case isn't well-tested (or activeSite was not the stream you wanted). I'll take a look.

I will work on making both of these changes (only use SpannerAnchors if you have to, and fix Spanner.fill as necessary).

[mscuthbert]

Thanks!

re fill(): Yeah, I don't see an example in the docs of filling between multiple measures so it's possible that hadn't been properly implemented. sigh. :-D

[gregchapman-dev]

I think that fill() should require a stream argument (and perhaps allow a list of streams, for the case where pedal marks should be filled from both left and right hand staves of a piano part within an orchestral score). I think I was being too helpful when I allowed None, implying sp.getFirst().activeSite. That's almost never useful.

[gregchapman-dev]

Sigh... I merged 9.7.1 (master) back into this PR, and tests are failing. I'll investigate.

[mscuthbert]

Sigh... I merged 9.7.1 (master) back into this PR, and tests are failing. I'll investigate.

I'm so sorry -- I don't remember making changes that were supposed to affect this.

[gregchapman-dev]

Oh I'm definitely not blaming you! My current theory is a bad merge (user error, or git error). I'll figure it out.

[gregchapman-dev]

It was indeed a bad merge of xmlToM21.py.

[gregchapman-dev]

@mscuthbert this is ready for review (unless you want me to remove the makeRests change).

[gregchapman-dev]

Converting to draft, waiting for the split-out PRs to land. Then I'll remerge, and make sure everything still tests clean. First step is to remove the makeRests change that was rejected.

[mscuthbert]

Hi -- is this about fixing behavior if makeNotation=False? I am not interested in having any changes that only improve this setting. Thanks.

[gregchapman-dev]

No, now that I have removed the makeRests change you rejected, this PR no longer has anything to do with makeNotation=False. (I will double check the diffs to be sure.)

[gregchapman-dev]

For the record, run_tests (3.12) failure was a "coveralls is not responding" error. Hopefully that will be fixed well before I turn this back into a non-draft PR.

[mscuthbert]

Greg -- I'm so sorry that I've let all this stall for a year -- after PyCon I want to look again. Thanks!

[gregchapman-dev]

I'm actually no longer sure that supporting offset fully for MusicXML elements is a good idea. Sometimes it's really important (e.g. I've seen pedal marks in MusicXML files that will miss a spanner element without it), and sometimes it's not so smart (e.g. I've seen directions in MusicXML files that are just being shifted left for typographical reasons, including to a negative offset within the measure).

What do you think?

[mscuthbert]

I made the changes needed for the merge to #1807 -- is this downstream from pedal or upstream? -- Upstream.

[mscuthbert]

I'm actually no longer sure that supporting offset fully for MusicXML elements is a good idea. Sometimes it's really important (e.g. I've seen pedal marks in MusicXML files that will miss a spanner element without it), and sometimes it's not so smart (e.g. I've seen directions in MusicXML files that are just being shifted left for typographical reasons, including to a negative offset within the measure).

What do you think?

I think that since Finale 2010ish and definitely in modern software like Dorico (and I think MuseScore), there is now an easy way of distinguishing between moving an element for typographical reasons and doing it for offset/playback reasons, so I think going forward more and more musicxml files will have correct offset elements, so we should respect it.

Nice to have feature later would be to be able to pass converter.parse('x.xml', ignoreDirectionOffsets=True) so those old scores work out better -- but that's definitely a different PR/downstream.

Let me know if you're on board with the new staffKey handling or if we need to think of something else.

Oh, and xmlToM21.py line 8 addition in my recent push is long-long overdue.