More accurate spanner stop/start on import (and make import of exported MusicXML from those scores work, too)

[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.025% (+0.02%) from 93.009%
when pulling 00bebd9 on gregchapman-dev:gregc/moreAccurateSpannerStartStop
into 501e13d on 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]

@mscuthbert Tests look like they will pass, so this PR is ready to go. The most controversial change here is, I believe, that I modified makeRests to insert split rests instead of a complex duration rest. If you're not up for that, I can undo it; it was briefly necessary, but I believe it isn't necessary anymore.

[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.