Skip to content

Conversation

@jacobtylerwalls
Copy link
Member

  • Fixes Beaming corrections for stripTies() #994: stripTies() sets StreamStatus.beams = False
  • for that to have effect, refactored fixupNotationMeasured() in musicxml export so that it doesn't use a dummy stream as an iterator and then query that dummy stream for its beam status
  • called splitAtDurations() earlier in musicxml export so that it happens before fixupNotationMeasured remakes beams
  • moved full-measure rest splitting logic out of musicxml exporter and into splitAtDurations()

splitAtDurations earlier during musicxml export (and recurse)
@coveralls
Copy link

coveralls commented May 7, 2021

Coverage Status

Coverage increased (+0.006%) to 92.267% when pulling bfb6e97 on jacobtylerwalls:stripTies-beams into a8ae14a on cuthbertLab:master.

@mscuthbert
Copy link
Member

Other than that Q on dot groups, looks good to me! And probably worth merging first since the actual problem solved is so much bigger.

@jacobtylerwalls
Copy link
Member Author

Super, is there a review you need to submit so I can see the dot groups stuff?

Exporting nested measures wasn't supported, so don't make it look like it's supported now
Copy link
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.

Ah! Here's the review!

Comment on lines 2877 to 2887
# split at durations, if not a full-measure rest (e.g. whole rest in 9/8)
if 'GeneralNote' in classes and obj.duration.type == 'complex' and not (
'Rest' in classes and (
obj.fullMeasure in (True, 'always')
or (obj.fullMeasure == 'auto' and obj.duration == self.stream.barDuration)
)
):
objList = obj.splitAtDurations()
else:
objList = [obj]

Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why this can be removed? Are we doing it elsewhere? Ah, I see below. Interesting! Does it work with dotGroups, since they're only split above?

Perhaps dotGroups should also be split in splitAtDurations (as an optional parameter) since they're something most software, including the musicxml spec, can't handle.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think both this and the 'inexpresible' bit above show that there is still a need to call splitAtDurations() in these edge cases. I'll restore the else: objList = [obj] bits so that all we skip here is the full-measure rest logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

For anyone else: the reason this was moved into splitAtDurations() was because it needed to be moved somewhere earlier during musicxml parsing so that beams could be made afterward (fixupNotationMeasured), and it seemed convenient to just make it splitAtDuration()'s responsibility.

@mscuthbert
Copy link
Member

Sorry about that -- mentioned the review now.

n =  note.Note()
n.duration.dotGroups = [1, 1]
n.show()

if this works, we're fine.

Before this patch this would fail downstream. Just noticed now it will silently export invalid musicxml, so now raise an informative error.
@jacobtylerwalls
Copy link
Member Author

if this works, we're fine.

checked this against the most recent commit on this branch and get correct output (i.e. same as master)

@jacobtylerwalls
Copy link
Member Author

Thanks for catching the multiple StreamIterators; I had just internalized that first() was faster because it short-circuits and have bee using it all over the place without thinking, but certainly faster to just get something once.

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.

Beaming corrections for stripTies()

3 participants