Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue #1335: voice numbers written in MusicXML must be unique… #1336

Merged
merged 13 commits into from
Aug 23, 2022
Merged

Fix issue #1335: voice numbers written in MusicXML must be unique… #1336

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
6760792
Fix issue #1335: voice numbers written in MusicXML must be unique wit…
gregchapman-dev Jul 2, 2022
2f3b9bb
Oops. Delete commented-out old code.
gregchapman-dev Jul 4, 2022
3c0203e
A better place to adjust voice numbers.
gregchapman-dev Jul 5, 2022
e7ff0d7
An even better implementation.
gregchapman-dev Jul 15, 2022
f1d13a9
Let's do this per measure.
gregchapman-dev Jul 17, 2022
1628687
One more test: three PartStaffs in StaffGroup, two Measures in each P…
gregchapman-dev Jul 17, 2022
5635a38
Address Jacob's latest review.
gregchapman-dev Aug 1, 2022
19063be
Sequence, then.
gregchapman-dev Aug 1, 2022
11037c9
Merge branch 'master' into pr/1336
mscuthbert Aug 3, 2022
edacf1f
Merge branch 'master' into pr/1336
mscuthbert Aug 10, 2022
4ec4d6a
Merge branch 'master' into gregc/voiceNumbers
gregchapman-dev Aug 22, 2022
6a7027b
Rewrite to use OffsetIterator and be much simpler. Doesn't work yet.
gregchapman-dev Aug 23, 2022
da3ba8c
Works now.
gregchapman-dev Aug 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
161 changes: 81 additions & 80 deletions music21/musicxml/m21ToXml.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1490,6 +1490,7 @@ def parse(self):
# before attempting to identify and count instruments
self._populatePartExporterList()
self.groupsToJoin = self.joinableGroups()
self.prePartProcess()
self.parsePartlikeScore()
else:
self.parseFlatScore()
Expand Down Expand Up @@ -1735,6 +1736,66 @@ def parseFlatScore(self):
pp.parse()
self.partExporterList.append(pp)

def prePartProcess(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this title should be more explicit: renumberVoicesWithinPartGroups, or something like that. Then I think for simplicity, it would be better to call two functions -- setPartExporterStaffGroups() before this is called, and then this function, which should itself call a helper function for every partExporter with a staff group. Doing more than one thing per function leads to added complexity.

Once you have a partExporter with the spannedElements, there's no need for all the simultaneous measure finding, just do something like this:

staffGroupScore = Score(partExp.staffGroup.getSpannedElements())
for measureStack in OffsetIterator(staffGroupScore).getElementsByClass(Measure):
     maxVoiceId = 0
     for m in measureStack:
           for v in m[Voice]:
                 ...  # do something

I think that would make the logic much simpler.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And then we can just renumber voices here and not make the PartExporter have to keep track of them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, I can't believe I didn't think of OffsetIterator!

'''
Scans the parts/measures/voices in each joinable group to compute
the voiceNumberOffset needed in each part. Parts that aren't in a
joinable group get offset 0.

Called automatically by .parse()
'''
partExporterForPart: t.Dict[stream.Part, PartExporter] = {}

# Compute partExp.staffGroup and partExp.maxNumVoices
for partExp in self.partExporterList:
partExporterForPart[partExp.stream] = partExp

joinableGroup = None
for sg in self.groupsToJoin:
if partExp.stream in sg:
joinableGroup = sg
break
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a bit of an O(n^2) approach. It's probably fine since n=num parts, and likely to be very fast, but in general there are ways to do this in O(n) time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nevermind, see above.


partExp.staffGroup = joinableGroup
if partExp.staffGroup is None:
partExp.maxVoiceNum = 0
continue

# part is in a staffGroup, so we need to find the maximum number
# of voices used in any measure in this part. We also find the
# maximum "low enough to not be considered a memory location"
# voice id. The maxVoiceNum for a part is the higher of
# maxNumVoices-1 and maxNonMemLocationVoiceId.
maxNumVoices = 0
maxNonMemLocationVoiceId = 0
for measure in partExp.stream[stream.Measure]:
voices = tuple(measure[stream.Voice])
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved
maxNumVoices = max(maxNumVoices, len(voices))
for voice in voices:
if (isinstance(voice.id, int)
and voice.id < defaults.minIdNumberToConsiderMemoryLocation):
maxNonMemLocationVoiceId = max(maxNonMemLocationVoiceId, voice.id)
partExp.maxVoiceNum = max(maxNumVoices - 1, maxNonMemLocationVoiceId)
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved

# Compute partExp.voiceNumberOffset
for partExp in self.partExporterList:
if partExp.staffGroup is None:
partExp.voiceNumberOffset = 0
continue

staffGroup = partExp.staffGroup

staffGroupStreamList = staffGroup.getSpannedElements()
partIndexInGroup = staffGroupStreamList.index(partExp.stream)
voiceNumberOffset = 0
# add up the offsetForPart of each part before us in the group
for i in range(0, partIndexInGroup):
pex = partExporterForPart[staffGroup[i]]
offsetForPart = pex.maxVoiceNum + 1
voiceNumberOffset += offsetForPart

partExp.voiceNumberOffset = voiceNumberOffset

def postPartProcess(self):
'''
calls .joinPartStaffs() from the
Expand Down Expand Up @@ -2548,9 +2609,17 @@ def __init__(self,
# has changed
self.lastDivisions = None

# maxVoiceNumber is keyed by measureNumberWithSuffix, value is the maximum
# exported voice number used for that measure (in this Part).
self.maxVoiceNumbers: t.Dict[str, int] = {}
# voiceNumberOffset is the offset that must be applied to any voice numbers automatically
# generated in this part, in order to avoid voice numbers (either explicitly set, or
# automatically generated) in other parts in this staffGroup. This is computed in a
# pre-pass over all the voices in all the measures in all the parts (in ScoreExporter).
self.voiceNumberOffset: int = 0

# The maximum voiceNum found in the voices in all the measures in this part.
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved
self.maxVoiceNum: int = 0

# The staffGroup to which this part belongs (if it belongs to one)
self.staffGroup: t.Optional[layout.StaffGroup] = None

self.spannerBundle = partObj.spannerBundle

Expand Down Expand Up @@ -2617,7 +2686,6 @@ def parse(self):
measureExporter.spannerBundle = self.spannerBundle
try:
mxMeasure = measureExporter.parse()
self.maxVoiceNumbers[m.measureNumberWithSuffix()] = measureExporter.maxVoiceNumber
except MusicXMLExportException as e:
e.measureNumber = str(m.number)
if isinstance(self.stream, stream.Part):
Expand Down Expand Up @@ -3026,8 +3094,6 @@ def __init__(self,
self.offsetInMeasure = 0.0
self.currentVoiceId: t.Optional[int] = None
self.nextFreeVoiceNumber: int = 1
self.maxVoiceNumber: int = 0
self.voiceNumberOffset: t.Optional[int] = None

self.rbSpanners: t.List[spanner.RepeatBracket] = [] # repeatBracket spanners

Expand Down Expand Up @@ -3085,74 +3151,15 @@ def mainElementsParse(self):

def getVoiceNumberOffset(self) -> int:
'''
Returns the offset that should be added to any voice numbers in this measure,
in order to avoid voice numbers in other simultaneous measures within a staff
group. If this measure is not in a staff group, the offset is 0.
This is computed once, and cached, for performance.
'''
if self.voiceNumberOffset is not None:
# return the cached computed offset
return self.voiceNumberOffset

def getMaxVoiceNumber(
scoreExp: ScoreExporter,
partStaff: stream.Part,
measure: stream.Measure) -> int:
# find the partExporter for partStaff
partStaffExp: t.Optional[PartExporter] = None
for partx in scoreExp.partExporterList:
if partx.stream is partStaff:
partStaffExp = partx
break
if partStaffExp is None:
return 0

maxVoiceNumber: int = partStaffExp.maxVoiceNumbers.get(
measure.measureNumberWithSuffix(),
0
)
return maxVoiceNumber

voiceNumberOffset: int = 0

Returns the offset that should be added to any voice numbers in this measure,
in order to avoid voice numbers in other simultaneous measures within a staff
group. This voice number offset is pre-computed for each part before exporting
any measures. So we just get it from the parent partExporter here.
'''
partExp: t.Optional[PartExporter] = self.parent
if partExp is None:
return voiceNumberOffset

scoreExp: t.Optional[ScoreExporter] = partExp.parent
if scoreExp is None:
return voiceNumberOffset

# scoreExp.groupsToJoin is a list of StaffGroups
if not scoreExp.groupsToJoin:
return voiceNumberOffset


assert(isinstance(self.stream, stream.Measure))
myMeasure: stream.Measure = self.stream

for group in scoreExp.groupsToJoin:
if partExp.stream not in group:
continue

for partStaff in group:
if partStaff is partExp.stream:
# skip our own PartStaff
continue

otherMeasure: stream.Measure = partStaff.measure(
myMeasure.measureNumberWithSuffix()
)
if otherMeasure is None:
# if measures don't have numbers, we can't do this
continue

maxVoiceNumber: int = getMaxVoiceNumber(scoreExp, partStaff, otherMeasure)
voiceNumberOffset = max(voiceNumberOffset, maxVoiceNumber)

# cache and return the new computed offset
self.voiceNumberOffset = voiceNumberOffset
return voiceNumberOffset
return 0
return partExp.voiceNumberOffset

def parseFlatElements(self, m, *, backupAfterwards=False):
'''
Expand All @@ -3172,20 +3179,14 @@ def parseFlatElements(self, m, *, backupAfterwards=False):
if isinstance(m, stream.Voice):
m: stream.Voice
if isinstance(m.id, int) and m.id < defaults.minIdNumberToConsiderMemoryLocation:
# this voice id is a low number, assigned by someone for a reason. We should
# not change it, but we will avoid it if we need to change some other voice id.
voiceId = m.id
self.maxVoiceNumber = max(self.maxVoiceNumber, voiceId)
elif isinstance(m.id, int):
# this voice id is actually a memory location, so we need to change it
# 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. Note that we need to
# carefully make it unique across the entire part (i.e. if the part is
# actually a group of PartStaffs, it must be unique across all those
# PartStaffs, not just this one).
# add an offset to make it unique across the entire staffGroup.
voiceId = self.nextFreeVoiceNumber
self.nextFreeVoiceNumber += 1
voiceId += self.getVoiceNumberOffset()
self.maxVoiceNumber = max(self.maxVoiceNumber, voiceId)
else:
voiceId = m.id
else:
Expand Down
12 changes: 6 additions & 6 deletions music21/musicxml/partStaffExporter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -959,7 +959,7 @@ def testJoinPartStaffsD2(self):
from music21 import note
s = stream.Score()
ps1 = stream.PartStaff()
m1 = stream.Measure(1)
m1 = stream.Measure()
ps1.insert(0, m1)
v1 = stream.Voice()
v2 = stream.Voice()
Expand All @@ -970,7 +970,7 @@ def testJoinPartStaffsD2(self):
ps1.makeNotation(inPlace=True) # makeNotation to freeze notation

ps2 = stream.PartStaff()
m2 = stream.Measure(1)
m2 = stream.Measure()
ps2.insert(0, m2)
v3 = stream.Voice()
v4 = stream.Voice()
Expand All @@ -995,16 +995,16 @@ def testJoinPartStaffsD2(self):
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, '3')
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, '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, '1')
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, '2')
self.assertEqual(mxNote.find('voice').text, '4')
self.assertEqual(mxNote.find('staff').text, '2')

def testJoinPartStaffsE(self):
Expand Down