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 tempo rounding issue in MusicXML export #7309

Merged
merged 1 commit into from Feb 4, 2021

Conversation

matangover
Copy link

@matangover matangover commented Jan 23, 2021

This is a long PR description for a seemingly simple issue :)

Since MuseScore internally stores tempo in units of 'beats per seconds' and truncates it to a fixed precision, the exported MusicXML tempo was sometimes incorrectly rounded. For example, 92 BPM in MuseScore was exported as 91.9998 and 88 BPM was exported as 88.0002.

To eliminate this error, tempo is now rounded to up to 2 decimal places when exporting (that is, up to 2 digits to the right of the decimal separator). Since the MuseScore interface itself only allows up to 2 decimal places anyway (in the Inspector), this is not expected to limit users.

Disclaimer: if files with fractional tempos are imported (with more than 2 decimal places), their tempo will be rounded to 2 decimal places on export, but this is presumably very rare (compared to the very common integer tempi which this PR fixes).

To reproduce the bug: Open MuseScore 3.6 and create a new score. Drag a 'quarter = 80' tempo marking to the score. Export the score to MusicXML – see the tempo is exported incorrectly (sound tempo="79.9998"). After applying this PR, it would be exported correctly.

Note: the underlying cause to this bug is actually not related to MusicXML export, it is due to the fact that tempo is stored imprecisely in mscx/mscz files. To illustrate this, try the following: Open MuseScore 3.6 and create a new score. Drag a 'quarter = 80' tempo marking to the score. In the Inspector, untick 'Follow text', and then re-tick it (this causes re-computation of the tempo internally). Export the score to MusicXML – see the tempo is exported correctly (sound tempo="80"). Save the score to mscx/mscz, close the score, and re-open the mscx/mscz. Export again to MusicXML – now the tempo is exported incorrectly (sound tempo="79.9998").

However, this PR doesn't fix the underlying issue because I'm not sure that this fix would be desirable to you. I submitted that change in #7310. In any case, even if the other PR is applied, the current PR is still needed, because of two reasons: 1) old mscx/mscz files that users have lying around, 2) the user's stored palette configurations will already have the tempo imprecisely stored.

  • I signed CLA
  • I made sure the code in the PR follows the coding rules
  • I made sure the code compiles on my machine
  • I made sure there are no unnecessary changes in the code
  • I made sure the title of the PR reflects the core meaning of the issue you are solving
  • I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
  • I made sure the commit message title starts with "fix #424242:" if there is a related issue
  • I created the test (mtest, vtest, script test) to verify the changes I made

Since MuseScore internally stores tempo in units of 'beats per seconds'
and truncates it to a fixed precision, the exported MusicXML tempo
was sometimes incorrectly rounded. For example, 92 BPM in MuseScore
was exported as 91.9998 and 88 BPM was exported as 88.0002.

To eliminate this error, tempo is now rounded to up to 2 decimal places
when exporting. Since the MuseScore interface itself only allows up to
2 decimal places (in the Inspector), this is not expected to limit any
users.
@matangover matangover changed the title Fix tempo rounding issues in MusicXML export Fix tempo rounding issue in MusicXML export Jan 23, 2021
@vpereverzev
Copy link
Member

I encourage you to create this PR for master, not for 3.x

@matangover
Copy link
Author

Is this the general guidance for all prs? Are they being cherry picked later?

@vpereverzev
Copy link
Member

I'm currently working on 3.6.1 patch release and it'll include only fixes for critical regressions. There will be no "good to have" changes, because otherwise we'll go into infinite loop of fixing issues.

@matangover
Copy link
Author

Ah ok, will there be further 3.x releases?

@vpereverzev
Copy link
Member

Never mind, we'll port it into master directly

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.

None yet

2 participants