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.

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 ugly MusicXML durations & offsets #1632

Merged
merged 5 commits into from
Oct 4, 2023

Conversation

TimFelixBeyer
Copy link
Contributor

We all know the ugly offsets (3875/480, 161/180, etc.) that happen after importing some musicXML files.
This usually occurs because the file contains tuplets with strange durations that cannot be represented exactly using integer <duration> tags in musicXML. For this reason, rounding errors can accumulate and lead to over/underfull measures.
Currently, if a measure is overfull, music21 just accepts that as a fact and increases the duration of the measure, shifting all following objects and resulting in them having very odd offsets.

This proposal deals with this by automatically detecting such cases, warning the user, and fixing them.
The change only affects behavior if the measure duration resulting from the contained notes differs from the duration dictated by the time signature by less than 0.5 and is a very ugly duration.
In my testing, this fixes the vast majority of imports without breaking anything else.

Let me know what you think!

@TimFelixBeyer TimFelixBeyer changed the title Fix MusicXML import errors Fix ugly MusicXML durations Jul 25, 2023
@TimFelixBeyer TimFelixBeyer changed the title Fix ugly MusicXML durations Fix ugly MusicXML durations & offsets Jul 25, 2023
@coveralls
Copy link

Coverage Status

coverage: 93.038% (+0.002%) from 93.036% when pulling fea95c6 on TimFelixBeyer:patch-5 into 8baa273 on cuthbertLab:master.

@mscuthbert
Copy link
Member

this looks good -- can you rename "PP" in the tests to "pp" -- capital letters should only be used to represent classes, not instances of classes. Then anyone w/ merge rights should feel free to merge.

@TimFelixBeyer
Copy link
Contributor Author

I actually fully agree, just did this to be in-line with all the other tests.
I've now lowercased all instances of PartParser that i could find.

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.

Thanks! Sorry for letting the inconsistencies in in the first place

@mscuthbert mscuthbert merged commit b6af0b5 into cuthbertLab:master Oct 4, 2023
6 checks passed
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