New PedalMarks implementation #1766
Conversation
…ted but not exported. Note that I apparently lied when I said that <pedal "*"><pedal "Ped"> was being carefully imported as a bounce. I have added TODO: comments to mark where that needs to be done.
|
Oh, I didn't update the tests yet. So they will fail madly. |
|
Indeed they did. I'm on it. |
|
Tests are passing! All that's left (beyond responding to review comments) is to figure out, while parsing |
… tired of writing the same inherit/override code multiple times.
|
Greg -- since you have an "All that's left is to..." -- is this good to review/merge or should I wait on that? |
|
It would be good to get your review on what I have. I'm working on a post-pass over the PedalMarks in each Part to merge them with a bounce if appropriate, but it might take a bit. I could do that in a separate PR if you want to merge soon. |
|
Actually, yes please, take this PR as complete and ready for review. I will write a new issue/PR for the fact that xmlToM21.py doesn't produce PedalBounces in a PedalMark with sign="yes" (or line="no"). What it does produce in those cases is demonstrably correct, but could be improved. |
|
Wait! I have an m21ToXml.py bug. |
|
lemme know when it's fixed (and please add a test so I don't break it in the future!) |
|
Will do. |
… will turn into new <pedal> starts. MusicXML import: Put all PedalMark spanners in the last (i.e. lowest) PartStaff for a <part>.
…ffsetIterator fixes a bug where occasionally the first objGroup would go through the loop twice.
….e. a hidden rest). This happens a lot with <pedal type="start"><forward...><pedal type="stop>.
|
Well, so much for "one more bug to fix"! So far I have found and fixed four pretty bad bugs in m21ToXml.py and xmlToM21.py. I ended up doing some pretty extensive testing (one example: parse 4000+ Humdrum scores using converter21, write back to MusicXML, then parse those 4000+ MusicXML files, and compare with the imported Humdrum using musicdiff), and I'm still poring over the results, fixing things, re-running all those test cases, ad infinitum. I am writing specific regression tests for each bug fix. Hopefully this will settle down soon, at which point I will push all the fixes/tests. |
|
Eek -- thanks for finding these -- not surprised that there would be Humdrum bugs, but bad bugs in XML I'm embarrassed by and hope we can fix -- if they're unrelated to pedaling, can you make a separate PR? If they're related, keep here. You should be able to check "Ready for review" when you'd like me to take a look. |
|
The Humdrum bugs are mine (I don't use music21's Humdrum code). So far I've only found one MusicXML bug that wasn't specific to
I reran all my Humdrum -> MusicXML tests, and found that this fix changed (for the better) a bunch of DynamicWedge durations as well. I'm tracking down more wrong PedalMark durations, so there may be more of this. |
|
My gut feel is that there are some corner-case bugs in music21/musicxml having to do with |
|
This is my fixes so far. |
|
Ah, here's the thing: xmlDirectionTypeToSpanners doesn't take totalOffset into account for start or stop (I'm pretty sure it didn't even receive it as an argument until I needed it for PedalTransitions). It just assumes that (1) the next GeneralNote to be emitted is at the correct offset for starts, and (2) self.nLast (the last GeneralNote to be emitted) is at the correct offset for stops. That doesn't take into account any intervening |
…o longer trip over intervening <backup> and <forward> elements, and also so we take into account the offset attribute of the direction.
|
@mscuthbert I'm going to need some advice. Should I take direction offset into account when positioning the start of (say) an ottava or pedal mark? For example, I see a lot of |
… being the same object).
…a test for wrong offset.
…nal parameters for new behavior. I am assuming that old clients of this API will be happy with the fix to insert the pending element as first in the spanner. If not, I'll add an additional optional parameter, addAsFirst=False, to handle those old clients.
…AccurateSpannerStartStop
…AccurateSpannerStartStop
…that, and touch up the tests.
…est in a Finale document".
|
There are 3 PRs I am proposing. The main one (PR #1766) is for a new implementation of PedalMarks; that's pretty straightforward, I'm just modifying the recently merged implementation to match your requested features. It's pretty cool. In order to test that thoroughly (in both the MusicXML reader and writer), I wrote a test that takes a list of MusicXML files, reads them into music21, writes them back out to MusicXML, reads that written file back into music21, and then uses musicdiff APIs to compare the two music21 scores. I discovered very quickly that pedal marks in MusicXML files often have offsets that extend them to cover notes that are before the The first bug I had to fix will take a little explanation: When exporting to MusicXML from a measure with SpannerAnchors, the notes are written first, then a Then I remembered that I had stolen that code from PR #1636, and it was starting to look like the stolen code was not going to be accepted. Jacob invited me to help on PR #1636, so I quickly wrote a fix I thought he would like, and submitted it as PR #1777, and merged that fix into #1768 and #1766 (replacing the old stolen code) to try it out. So, while they truly can be though of as separate PRs fixing separate things, they are nested in the sense that the PedalMarks PR has all the fixes from the other two, and the "more accurate spanners" PR has the "no more hidden rests from xmlForward" fix. This is because without those fixes, my PedalMarks PR doesn't work right, and can't really be tested thoroughly. I'm hoping that these PRs can be treated as separate fixes for separate bugs, and accepted in the following order: first PR #1777 (no more hidden rests from xmlForward), then PR #1768 (more accurate spanners), and then PR #1766 (better PedalMarks). After each PR is merged to master, I will of course merge master back into the other PR(s) and retest before the next PR is merged. |
|
I'm going to mark this as a draft PR until we get the other two PRs approved and merged into this one. |
New implementation. Also fixed a bug where positions were being imported but not exported. Note that I apparently lied when I said that <pedal "*"><pedal "Ped"> was being carefully imported as a bounce. I have added TODO: comments to mark where that needs to be done.