Repository: cuthbertLab/music21
Pull request #1336 (2022-07-02 22:39:00)

Fix issue #1335: voice numbers written in MusicXML must be unique…
…within the StaffGroup.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/50548308/badge)](https://coveralls.io/builds/50548308)

Coverage increased (+0.004%) to 93.057% when pulling **676079260c9bfa574cbab9d83ec13cc24aa83857 on gregchapman-dev:gregc/voiceNumbers** into **48a3fa6780cc4c45d1926681af249372e85c3177 on cuthbertLab:master**.

mscuthbert: I think that this looks good. @jacobtylerwalls did a lot of work on the PartStaffExporter, so I'd like him to weigh in before merging.  Please remove newly added code that is commented out.  thanks!
mscuthbert: I'm a little concerned that we're going further in the direction of manipulating XML that we have generated (which I didn't like in the initial PartStaffExporter proposal) and away from changing Python objects such as Voices etc. Can we do more preprocessing before XML is written (since this is already a deepcopy) to keep the complexity down?
gregchapman-dev: OK, I'm back to looking at this "voice numbers" PR.

I will definitely add support for unnumbered measures, and I'll try to manipulate the voice.id rather than the generated XML.
gregchapman-dev: I like this a lot better.  I scan the score to compute the voiceNumberOffset for each PartExporter, _then_ do the parse.  Since I'm not doing measure-by-measure offsets (I use the same offset in every Measure in a Part), the measure numbers don't matter at all.  And the partExp.voiceNumberOffset is used to offset the voice.id, avoiding further manipulation of generated XML.
gregchapman-dev: > My main question is just why we are tracking the voice number offsets at the part level? I would have thought they need to be tracked at the measure level.

For simplicity, because I found I didn’t care so much about the case below (using higher voice numbers than necessary in some measures).  But since you do care, I will use your cool idea about zip_measures, and track voice number offsets at the measure level...

> Wouldn't this PR now unnecessarily turn the bass clef in measure 2 into voices 3 and 4 (instead of leaving it as 1 and 2?) This would be worth testing in some musicxml readers -- especially when the voice numbers end up above 4. And a unit test with multiple measures would be good.

I will add such a test.

jacobtylerwalls: Great. Yeah it's not so much that I care at a philosophical level, just that of all the musicxml readers out there, some of them (MuseScore?) I would expect to do funny things when the voice numbers start getting high.
gregchapman-dev: OK, ready for another review.
gregchapman-dev: Wow, flake8 got a lot pickier throughout music21.  Hope that doesn't block me from getting this in.
jacobtylerwalls: > Wow, flake8 got a lot pickier throughout music21. Hope that doesn't block me from getting this in.

See #1347, you could merge master now.
gregchapman-dev: I'm doing the following:

`for measureStack in OffsetIterator(staffGroupScore).getElementsByClass(stream.Measure):`

and I never get any measureStacks.  Does OffsetIterator not recurse?  The staffGroupScore I'm testing with contains 2 PartStaff objects, each of which has a Measure.

gregchapman-dev: I believe this is complete, and ready for review now.
mscuthbert: > I'm doing the following:
> 
> `for measureStack in OffsetIterator(staffGroupScore).getElementsByClass(stream.Measure):`
> 
> and I never get any measureStacks. Does OffsetIterator not recurse? The staffGroupScore I'm testing with contains 2 PartStaff objects, each of which has a Measure.

AH.  it might not.  SHOOT.

I'm working on new Iterator types right now, and I think I've gotten down how to make them work more simply.  So I should be able to add a recurse=True to OffsetIterator instead.  Okay, no requirement to implement it here.
mscuthbert: Everything looks great!  Congrats!

Patch:
diff --git a/music21/musicxml/m21ToXml.py b/music21/musicxml/m21ToXml.py
index 0af7d5dd7..949d0252f 100644
--- a/music21/musicxml/m21ToXml.py
+++ b/music21/musicxml/m21ToXml.py
@@ -1514,6 +1514,8 @@ class ScoreExporter(XMLExporterBase, PartStaffExporterMixin):
             # before attempting to identify and count instruments
             self._populatePartExporterList()
             self.groupsToJoin = self.joinableGroups()
+            self.setPartExporterStaffGroups()
+            self.renumberVoicesWithinStaffGroups()
             self.parsePartlikeScore()
         else:
             self.parseFlatScore()
@@ -1728,6 +1730,60 @@ class ScoreExporter(XMLExporterBase, PartStaffExporterMixin):
         pp.parse()
         self.partExporterList.append(pp)
 
+    def setPartExporterStaffGroups(self):
+        '''
+        Figures out the containing StaffGroup for every PartExporter that has one.
+
+        Called automatically by .parse()
+        '''
+        for partExp in self.partExporterList:
+            joinableGroup = None
+            for sg in self.groupsToJoin:
+                if partExp.stream in sg:
+                    joinableGroup = sg
+                    break
+
+            partExp.staffGroup = joinableGroup
+
+    def renumberVoicesWithinStaffGroups(self):
+        '''
+        Renumbers voices (as appropriate) in each StaffGroup, so that
+        voices have unique numbers across the entire group.
+
+        Called automatically by .parse()
+        '''
+        staffGroupsProcessed: t.List = []
+        for partExp in self.partExporterList:
+            if partExp.staffGroup is None:
+                # no staffGroup to process
+                continue
+
+            if partExp.staffGroup in staffGroupsProcessed:
+                # we already processed this one
+                continue
+
+            # renumber the voices in this StaffGroup
+            staffGroupScore = stream.Score(partExp.staffGroup.getSpannedElements())
+            measuresStream = staffGroupScore.recurse().getElementsByClass(stream.Measure).stream()
+            nextVoiceId: int = 1
+            for measureStack in OffsetIterator(measuresStream):
+                for m in measureStack:
+                    for v in m[stream.Voice]:
+                        if not isinstance(v.id, int):
+                            # it's not an integer, leave it as is, and don't move nextVoiceId
+                            continue
+                        elif v.id < defaults.minIdNumberToConsiderMemoryLocation:
+                            # it's a low integer, leave it as is, and jump nextVoiceId to v.id + 1
+                            nextVoiceId = v.id + 1
+                        else:
+                            # it's a memory location, set v.id to nextVoiceId and increment
+                            v.id = nextVoiceId
+                            nextVoiceId += 1
+
+            # remember we did this one, so we don't do it again
+            staffGroupsProcessed.append(partExp.staffGroup)
+
+
     def postPartProcess(self):
         '''
         calls .joinPartStaffs() from the
@@ -2612,6 +2668,9 @@ class PartExporter(XMLExporterBase):
         # has changed
         self.lastDivisions = None
 
+        # The staffGroup to which this part belongs (if it belongs to one)
+        self.staffGroup: t.Optional[layout.StaffGroup] = None
+
         self.spannerBundle = partObj.spannerBundle
 
     def parse(self):
@@ -3084,8 +3143,8 @@ class MeasureExporter(XMLExporterBase):
         self.measureOffsetStart = 0.0
         self.offsetInMeasure = 0.0
         self.currentVoiceId: t.Optional[int] = None
-        self.nextFreeVoiceNumber = 1
-        self.nextArpeggioNumber = 1
+        self.nextFreeVoiceNumber: int = 1
+        self.nextArpeggioNumber: int = 1
         self.arpeggioNumbers: t.Dict[expressions.ArpeggioMarkSpanner, int] = {}
 
         self.rbSpanners: t.List[spanner.RepeatBracket] = []  # repeatBracket spanners
@@ -3161,7 +3220,10 @@ class MeasureExporter(XMLExporterBase):
             m: stream.Voice
             if isinstance(m.id, int) and m.id < defaults.minIdNumberToConsiderMemoryLocation:
                 voiceId = m.id
+                self.nextFreeVoiceNumber = voiceId + 1
             elif isinstance(m.id, int):
+                # This voice id is actually a memory location, so we need to change it
+                # to a low number so it can be used in MusicXML.
                 voiceId = self.nextFreeVoiceNumber
                 self.nextFreeVoiceNumber += 1
             else:
diff --git a/music21/musicxml/partStaffExporter.py b/music21/musicxml/partStaffExporter.py
index d84ef335b..5e3e83da6 100644
--- a/music21/musicxml/partStaffExporter.py
+++ b/music21/musicxml/partStaffExporter.py
@@ -899,6 +899,12 @@ class Test(unittest.TestCase):
         s.insert(0, layout.StaffGroup([ps1, ps2]))
         root = self.getET(s)
         notes = root.findall('.//note')
+
+        # since there are no voices in either PartStaff, the voice number of each note
+        # should be the same as the staff number.
+        for mxNote in notes:
+            self.assertEqual(mxNote.find('voice').text, mxNote.find('staff').text)
+
         forward = root.find('.//forward')
         backup = root.find('.//backup')
         amountToBackup = (
@@ -955,6 +961,62 @@ class Test(unittest.TestCase):
         self.assertEqual(len(measures), 2)
         self.assertEqual(len(notes), 12)
 
+    def testJoinPartStaffsD2(self):
+        '''
+        Add measures and voices and check for unique voice numbers across the StaffGroup.
+        '''
+        from music21 import layout
+        from music21 import note
+        s = stream.Score()
+        ps1 = stream.PartStaff()
+        m1 = stream.Measure()
+        ps1.insert(0, m1)
+        v1 = stream.Voice()
+        v2 = stream.Voice()
+        m1.insert(0, v1)
+        m1.insert(0, v2)
+        v1.repeatAppend(note.Note('C4'), 4)
+        v2.repeatAppend(note.Note('E4'), 4)
+        ps1.makeNotation(inPlace=True)  # makeNotation to freeze notation
+
+        ps2 = stream.PartStaff()
+        m2 = stream.Measure()
+        ps2.insert(0, m2)
+        v3 = stream.Voice()
+        v4 = stream.Voice()
+        m2.insert(0, v3)
+        m2.insert(0, v4)
+        v3.repeatAppend(note.Note('C3'), 4)
+        v4.repeatAppend(note.Note('G3'), 4)
+        ps2.makeNotation(inPlace=True)  # makeNotation to freeze notation
+
+        s.insert(0, ps2)
+        s.insert(0, ps1)
+        s.insert(0, layout.StaffGroup([ps1, ps2]))
+        root = self.getET(s)
+        measures = root.findall('.//measure')
+        notes = root.findall('.//note')
+        # from music21.musicxml.helpers import dump
+        # dump(root)
+        self.assertEqual(len(measures), 1)
+        self.assertEqual(len(notes), 16)
+
+        # check those voice and staff numbers
+        for mxNote in notes:
+            mxPitch = mxNote.find('pitch')
+            if mxPitch.find('step').text == 'C' and mxPitch.find('octave').text == '4':
+                self.assertEqual(mxNote.find('voice').text, '1')
+                self.assertEqual(mxNote.find('staff').text, '1')
+            elif mxPitch.find('step').text == 'E' and mxPitch.find('octave').text == '4':
+                self.assertEqual(mxNote.find('voice').text, '2')
+                self.assertEqual(mxNote.find('staff').text, '1')
+            elif mxPitch.find('step').text == 'C' and mxPitch.find('octave').text == '3':
+                self.assertEqual(mxNote.find('voice').text, '3')
+                self.assertEqual(mxNote.find('staff').text, '2')
+            elif mxPitch.find('step').text == 'G' and mxPitch.find('octave').text == '3':
+                self.assertEqual(mxNote.find('voice').text, '4')
+                self.assertEqual(mxNote.find('staff').text, '2')
+
     def testJoinPartStaffsE(self):
         '''
         Measure numbers existing only in certain PartStaffs: don't collapse together


Test patch:
diff --git a/music21/musicxml/test_m21ToXml.py b/music21/musicxml/test_m21ToXml.py
index 08ddeee57..860ad01c7 100644
--- a/music21/musicxml/test_m21ToXml.py
+++ b/music21/musicxml/test_m21ToXml.py
@@ -146,11 +146,49 @@ class Test(unittest.TestCase):
         v1.id = 234
         xmlOut = self.getXml(m)
         self.assertIn('<voice>234</voice>', xmlOut)
-        self.assertIn('<voice>1</voice>', xmlOut)  # is v2 now!
+        self.assertIn('<voice>235</voice>', xmlOut)
         v2.id = 'hello'
         xmlOut = self.getXml(m)
         self.assertIn('<voice>hello</voice>', xmlOut)
 
+    def testVoiceNumberOffsetsThreeStaffsInGroup(self):
+        n1_1 = note.Note()
+        v1_1 = stream.Voice([n1_1])
+        m1_1 = stream.Measure([v1_1])
+        n1_2 = note.Note()
+        v1_2 = stream.Voice([n1_2])
+        m1_2 = stream.Measure([v1_2])
+        ps1 = stream.PartStaff([m1_1, m1_2])
+
+        n2_1 = note.Note()
+        v2_1 = stream.Voice([n2_1])
+        m2_1 = stream.Measure([v2_1])
+        n2_2 = note.Note()
+        v2_2 = stream.Voice([n2_2])
+        m2_2 = stream.Measure([v2_2])
+        ps2 = stream.PartStaff([m2_1, m2_2])
+
+        n3_1 = note.Note()
+        v3_1 = stream.Voice([n3_1])
+        m3_1 = stream.Measure([v3_1])
+        n3_2 = note.Note()
+        v3_2 = stream.Voice([n3_2])
+        m3_2 = stream.Measure([v3_2])
+        ps3 = stream.PartStaff([m3_1, m3_2])
+
+        s = stream.Score([ps1, ps2, ps3])
+        staffGroup = layout.StaffGroup([ps1, ps2, ps3])
+        s.insert(0, staffGroup)
+
+        tree = self.getET(s)
+        # helpers.dump(tree)
+        mxNotes = tree.findall('part/measure/note')
+        for mxNote in mxNotes:
+            voice = mxNote.find('voice')
+            staff = mxNote.find('staff')
+            # Because there is one voice per staff/measure, voicenum == staffnum
+            self.assertEqual(voice.text, staff.text)
+
     def testCompositeLyrics(self):
         xmlDir = common.getSourceFilePath() / 'musicxml' / 'lilypondTestSuite'
         fp = xmlDir / '61l-Lyrics-Elisions-Syllables.xml'