One fix in m21ToXML.py:parseFlatElements() #1806
Conversation
…n hidden rest, not just for an inexpressible duration hidden rest. (2) jump to end of measure needs to pass the distance to end of measure, not the offset of end of measure.
|
This is the first split-out PR from PR #1768. |
|
Why do we prefer a gap to a complex rest? Isn't multiple rests even better? |
|
I was going for the "least amount of change", and I knew a gap would work since we were already doing it for inexpressible durations. |
|
Ah. It sounds more like a bug to fix that we do it for inexpressible. Let me look more closely there. |
|
Are there tests that go with this? Something that would fail before and not fail now----I still don't have a clue what the problem is that this PR is solving! I must be very dense! |
|
When you say gap are you saying forward tag? Currently if I put a rest of 5.0 followed by a qtr note what happens in the xml and after this change what happens is I guess what I'm not clear on. Forward tags are only appropriate in multi voice staves really. |
|
Yes, the code (prior to this PR) was turning inexpressible hidden rests into forward tags, and I added code (in this PR) to also turn complex hidden rests into forward tags. I had some scores that had complex hidden rests in them, and they would crash during export to MusicXML without this fix. I think that if you want it, both inexpressible and complex hidden rests could be turned into a sequence of non-complex hidden rests (not sure about inexpressible, but it seems like I've read something somewhere about how to do it). I can certainly add a test that tries to write a MusicXML file from a code-constructed score that has a complex hidden rest in it. |
|
Oh, and answering your further comment: your example of hidden rest of 5.0 followed by a qtr note would crash before this change ('complex durations not writable to MusicXML', or something like that), and after the change would write a forward tag of duration 5.0 followed by a quarter note. (Note that this only happens for hidden rests.) |
|
Great -- can you add that as a test and show. If you look at the Test class in test_m21ToXml it has a helper method (Please don't add a |
…ow creates forward tag).
|
I used self.getET(obj) instead. Without my change this test raises MusicXMLException; with my change, it survives, and I check that the appropriate number of notes (1), forwards (1), and rests (0) are found in the ElementTree. |
music21/musicxml/test_m21ToXml.py
Outdated
| ''' | ||
| complexRest = note.Rest() | ||
| complexRest.quarterLength = 5.0 | ||
| complexRest.style.hideObjectOnPrint = True |
There was a problem hiding this comment.
ah -- these are hidden rests already -- okay -- that was an important part.
| # if necessary, jump to end of the measure. | ||
| if self.offsetInMeasure < firstPassEndOffsetInMeasure: | ||
| self.moveForward(firstPassEndOffsetInMeasure) | ||
| self.moveForward(opFrac(firstPassEndOffsetInMeasure - self.offsetInMeasure)) |
There was a problem hiding this comment.
Is this part of the same bug fix or a separate bug fix -- it doesn't seem to have anything to do with complex or not complex types? Thus is it actually being tested by the new tests below? I don't think so, so please test it.
There was a problem hiding this comment.
Unrelated. I'll try to write a test for it.
There was a problem hiding this comment.
For the record: this is a bug I introduced back when I was implementing SpannerAnchors in the first place. I assumed the wrong thing about what offset should be passed to moveForward. The end result was that MusicXML write from SpannerAnchor-y scores was pretty busted. And I only noticed now because we're making SpannerAnchors now during MusicXML read, so the bug is triggered.
There was a problem hiding this comment.
Still working on this test.
There was a problem hiding this comment.
Just pushed this new test.
music21/musicxml/test_m21ToXml.py
Outdated
| self.assertEqual(len(tree.findall('.//forward')), 1) | ||
| self.assertEqual(len(tree.findall('.//note')), 1) | ||
| self.assertEqual(len(tree.findall('.//rest')), 0) |
There was a problem hiding this comment.
Do we test that the forward tag moves forward 5 beats? It's little things like this that help make sure bugs don't creep in in the future.
There was a problem hiding this comment.
Good idea. I'm on it.
There was a problem hiding this comment.
Just pushed this test improvement.
music21/musicxml/test_m21ToXml.py
Outdated
| p.append(m) | ||
| s = stream.Score() | ||
| s.append(p) | ||
| tree = self.getET(s, makeNotation=False) |
There was a problem hiding this comment.
So all of this only applies if makeNotation is False?????? I'm flabbergasted -- tempted to remove it as an option. There is a reason why we make notation before showing -- to prevent crashes like this. Im really not interested in debugging anything else that comes forward only in makeNotation=False mode.
There was a problem hiding this comment.
I'll try to avoid fixing things specific to makeNotation=False in the future. Sorry about that!
|
OK, I think that's all the tests you asked for. |
|
This PR is ready for (re-)review. |
music21/musicxml/m21ToXml.py
Outdated
| if n.isRest and n.style.hideObjectOnPrint and n.duration.type == 'inexpressible': | ||
| if (n.isRest | ||
| and n.style.hideObjectOnPrint | ||
| and n.duration.type in ('inexpressible', 'complex')): |
There was a problem hiding this comment.
Greg -- 'complex' durations don't exist in m21ToXml unless makeNotation=False. I don't want fixes in here that cause different code paths for the two -- a complex duration in a stream at this point is an error.
|
There are still things specific to makeNotation=False here. Please remove. |
gregchapman-dev
changed the title
|
Ok, it's just the math fix now. |
|
Thanks! |
3 participants
Two fixes in parseFlatElements: (1) prefer a gap to a complex duration hidden rest, not just for an inexpressible duration hidden rest. (2) jump to end of measure needs to pass the distance to end of measure, not the offset of end of measure.