Skip to content

makeRests() gets bar duration context for voices from measures and recurses into Parts and checks for measure padding#986

Merged
jacobtylerwalls merged 6 commits into
cuthbertLab:masterfrom
jacobtylerwalls:makeRests-keyword-priority
May 2, 2021
Merged

makeRests() gets bar duration context for voices from measures and recurses into Parts and checks for measure padding#986
jacobtylerwalls merged 6 commits into
cuthbertLab:masterfrom
jacobtylerwalls:makeRests-keyword-priority

Conversation

@jacobtylerwalls
Copy link
Copy Markdown
Member

@jacobtylerwalls jacobtylerwalls commented Apr 26, 2021

Handling some cases not handled by #922.

Before
This describes the first bullet point under "Now", below.
#922 was the first effort toward recursing in makeRests(). It wasn't quite enough -- final rests in Voices weren't created from the last element to the end of the bar, because the Voice objects don't have a .barDuration. Then, a loose rest was eventually created outside of the voices on the Measure object (see the regression test I included--the first test would have failed). Update: I'm actually only describing the MIDI parsing workflow. Reading voices as defined in a musicXML file is unaffected.

Now

  • refactored makeRests to handle voices early enough that measure bar duration is accessible for voices
  • recursed over parts, since that was the remaining nested stream container to handle
  • defined and documented priority between keyword args timeRangeFromBarDuration and refStreamOrTimeRange
  • made timeRangeFromBarDuration account for measure padding
  • documented what was a missing feature on makeVoices() that now we handle creating trailing rests in voices when an incomplete measure is provided
  • rests created by musicxml export AFTER makeNotation runs are now hidden

Future
timeRangeFromBarDuration should probably default True. Nope! Too presumptuous--don't want to "complete" every incomplete voice parsed from musicXML. Some tests fail in interesting ways if I flip the default, suggesting possibly better results, but also that some work might be needed with .paddingLeft. Handled the padding issues.

@coveralls
Copy link
Copy Markdown

coveralls commented Apr 26, 2021

Coverage Status

Coverage increased (+0.008%) to 92.251% when pulling 750ac2d on jacobtylerwalls:makeRests-keyword-priority into 4dc319e on cuthbertLab:master.

@jacobtylerwalls
Copy link
Copy Markdown
Member Author

Ah, the .paddingLeft issues are a regression in #922 because we uncovered functionality and didn't handle this case. makeRests() is called in MusicXML export, just didn't do much if you already had measures (again, only streams made from MIDI were typically in that scenario before v7, but now even those have measures).

I'll push another commit to handle paddingLeft

Copy link
Copy Markdown
Member

@mscuthbert mscuthbert left a comment

Choose a reason for hiding this comment

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

I think it looks good -- I don't remember all the subtilties, but if you're confident, feel free to merge. Otherwise I'll take a deep dive.

Comment on lines +6497 to +6516
s[stream.Measure].first().paddingLeft = 2.0
s[stream.Measure].first().paddingRight = 1.0
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm still getting used to this. :-D


- `inPlace` defaults False
- Recurses into parts, measures, voices
- Gave priority to `timeRangeFromBarDuration` over `refStreamOrTimeRange`
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

btw -- I wonder if refStreamOrTimeRange can be removed -- is it ever used?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If we remove it, we'll be putting a constraint that you can only make rests in measures and voices, not flat streams. I'm not keen on taking that away.

After some thought I think it's time to default timeRangeFromBarDuration True. Especially since now we've given it priority.

Copy link
Copy Markdown
Member Author

@jacobtylerwalls jacobtylerwalls Apr 28, 2021

Choose a reason for hiding this comment

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

Nope, on even more thought, it's too sensitive to default that parameter True, because now that we recurse, we would descend into every part and measure and voice and complete every voice, filling measures with all kinds of unwanted rests. Better to leave how currently configured -- in the musicxml parsers, measures should get timeRangeFromBarDuration=True, but voices should not--which is what we have without adding more changes here.

@jacobtylerwalls
Copy link
Copy Markdown
Member Author

jacobtylerwalls commented Apr 28, 2021

I think it looks good -- I don't remember all the subtilties, but if you're confident, feel free to merge. Otherwise I'll take a deep dive.

Thanks for having a look. I think it's time to merge this. Found another rest issue, but one that's piano-specific, can reproduce on v.3.1.0 and doesn't look any worse with this patch, so if you want to don your diving goggles we can do that then. In the meantime we have some output that's wrong we can fix by merging this and bumping the minor version number.

EDIT: GIve me another day before merging -- I want to walk through a couple things to make sure they are necessary / covered by tests.

@jacobtylerwalls jacobtylerwalls changed the title makeRests() gets bar duration context for voices from measures and recurses into Parts makeRests() gets bar duration context for voices from measures and recurses into Parts and checks for measure padding Apr 28, 2021
@jacobtylerwalls jacobtylerwalls force-pushed the makeRests-keyword-priority branch from 99c2c27 to 6cd6b00 Compare April 28, 2021 16:51
@jacobtylerwalls
Copy link
Copy Markdown
Member Author

This is worth a second (shallow) dive given that I added three more commits. Feel good about this now, but again, would appreciate a double-check.

jacobtylerwalls added a commit to jacobtylerwalls/music21 that referenced this pull request May 1, 2021
jacobtylerwalls added a commit to jacobtylerwalls/music21 that referenced this pull request May 1, 2021
jacobtylerwalls added a commit to jacobtylerwalls/music21 that referenced this pull request May 1, 2021
…curses into Parts

Defined priority between keyword args `timeRangeFromBarDuration` and `refStreamOrTimeRange`
…ration

Use timeRangeFromBarDuration in makeVoices() and MusicXML export. Trailing rests in voices aren't made if we rely only on refStreamOrTimeRange. But it's not safe to use timeRangeFromBarDuration until accounting for measure padding.
@jacobtylerwalls
Copy link
Copy Markdown
Member Author

jacobtylerwalls commented May 1, 2021

I'll be merging this shortly. Not trying to rush anything! Just have another PR waiting on this, and in hindsight, the last set of changes doesn't really seem to require another substantive review. Those last changes were:

  • Avoiding redundant calls to makeRests given this PR's new recursion by part
  • Hiding rests created on musicxml export, since we have decided spacer rests == hidden rests (matches MusicXML import)

I'm squashing the trivial whitespace and typo fixes but leaving substantive commits for the independent pieces in the offchance we need to revert anything later. But this leaves the master branch in a better state to be merging this now, in order to resolve the issue with measure padding not being observed (causing extra rests on export).

This matches MusicXML import, since voices do not necessarily fill the measure.
@jacobtylerwalls jacobtylerwalls force-pushed the makeRests-keyword-priority branch from 74255e4 to 750ac2d Compare May 2, 2021 12:56
@jacobtylerwalls jacobtylerwalls merged commit af66685 into cuthbertLab:master May 2, 2021
@jacobtylerwalls jacobtylerwalls deleted the makeRests-keyword-priority branch May 2, 2021 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants