[MU3] Fix #311792: Override alter tag with note tuning value if present

[infojunkie]

Resolves: https://musescore.org/en/node/311792

This patch modifies function writePitch() in exportxml.cpp to override the "alter" MusicXML element with the value found in the note->tuning() if present. This allows the manual note tuning to be preserved during MusicXML export.

A test TestMxmlIO::noteTuning() is added to verify the desired behaviour.

• 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

[Jojo-Schmitz]

You ticked the 2nd to last box of that PR comment template, but in the commit title forgot the # ;-)

@lvinken mind to check, this being a MusicXML fix?

[infojunkie]

You ticked the 2nd to last box of that PR comment template, but in the commit title forgot the # ;-)

Yes terribly sorry! I read the guidelines after pushing the commit... Do you have a suggestion to fix that?

[Jojo-Schmitz]

Quite simple: git commit --amend, add the #, git push -f

[infojunkie]

Thanks. I used the recipe at https://www.educative.io/edpresso/how-to-change-a-git-commit-message-after-a-push, because it was the second-to-last commit that needed a change. Out of curiosity, do you use the squash option during PR merge?

[Jojo-Schmitz]

No, usually not, we'd rather have the submitter to do it ;-)

[lvinken]

First remarks:
I'm glad to see a test case, will have to check if it coverage is sufficient.

• please check if the code generates correct output for:
  • a G# without tuning
  • a G# with a small tuning (e.g. 10)
  • a G with a large tuning (e.g. 110)
    The last two, I believe, should produce the same alter value.
    Also, comparing a float with 0 is not a good idea due to inaccuracies in the floating point representation. Typically a small epsilon value is used instead. Similarly using a float as a boolean is not OK.
    More to follow later, have to go to work now.

[Jojo-Schmitz]

As MuseScore's UI allows tuning with 2 decimal digits only, and as comparing float to 0.0 and using it as a bool isn't a good idea anyway, maybe better use if (fabs(tun) > 0.01) {, similar to how it is used in

MuseScore/mscore/musescore.cpp

Line 6114 in 090b8ca

else if (note->ppitch() != pitch || fabs(tuning - note->tuning()) > 0.01) {

See also @lvinken 's comment about this

[infojunkie]

Fixed the comparison

[Jojo-Schmitz]

Now please squash those 3 commits into one. git rebase -i HEAD~3 (last 3 commits), replace the 2nd and 3rd pick with fixup, save, git push -f

[infojunkie]

please check if the code generates correct output for:
a G# without tuning
a G# with a small tuning (e.g. 10)
a G with a large tuning (e.g. 110)
The last two, I believe, should produce the same alter value.

Thanks for the useful test case. I have a few questions and comments:

• My understanding is that the Musescore note "tuning" value, as well as the MusicXML "alter" value, are not meant to take enharmonicity in consideration. This means that G# with a small tuning t vs G with the same equivalent tuning (100+t) will NOT result in a same "tuning" value. I verified this by creating a sheet with your suggested case, and examined the resulting Musescore file XML. The explicitly given tunings are preserved (i.e 10 vs 110). Is my understanding wrong?

• Currently exporting the file above to MusicXML seems to miss the "alter" value for the G# + 10c note. Will fix this as part of my PR.

[infojunkie]

Now please squash those 3 commits into one. git rebase -i HEAD~3 (last 3 commits), replace the 2nd and 3rd pick with fixup, save, git push -f

Thanks, I'll do that once I'm done with all changes here if you don't mind. Just a note, though: GitHub gives the option to "squash and merge" automatically, without using git rebase and git push -f. Happy to comply with your process in any case.

[Jojo-Schmitz]

Sure, take your time. As said, to the best of my knowledge the folks doing the merges do not squash.

[infojunkie]

One more comment to @lvinken : there's a TODO in writePitch() saying:

// TODO what if both alter and alter2 are present? For Example: playing with transposing instruments

In my opinion, alter + alter2 would make sense, since alter seems to deal with transposing instruments, while alter2 deals with accidentals and note tunings.

If you agree, can I include this fix too in this PR?

[lvinken]

On this one I cannot immediately provide a full answer to and I do not have the time available to work out the details. In retrospect, I am not sure transposition is the main issue here. The current implementation uses a special case for a few microtonal accidentals, ignores other and also ignores the tuning values. This implies the output may be incorrect in several corner cases.

All I can add at this time is: please try to make sure whatever the MuseScore GUI can do(in terms of creating internal data structures and visualisation on screen), leads to the expected MusicXML output.

[infojunkie]

All I can add at this time is: please try to make sure whatever the MuseScore GUI can do(in terms of creating internal data structures and visualisation on screen), leads to the expected MusicXML output.

OK, thanks. I'll do my best to enumerate the possible combinations and cases.

[Jojo-Schmitz]

Something went severely wrong with this, I guess you used git merge rather than git pull --rebase

[infojunkie]

Something severely went wrong with this, I guess you used git merge rather than git pull --rebase

Thanks, I think I fixed.

[infojunkie]

I added code + updated tests to ensure that the 2 alter values can be combined to produce expected results. Next, I will add more comprehensive tests before I submit this PR for review.

[Jojo-Schmitz]

Not double(alter+alter2), i.e. casting the result of the addition rather than just one of the 2 and in a more C++ way?

[infojunkie]

Not sure, one of them is int and the other double.

[Jojo-Schmitz]

OK, I see, but the result is a double in any case. No cast needed at all.

[Jojo-Schmitz]

What happens if you drop that cast? If that doesn't result in a compiler warning, just drop it.
The default result of any arithmetic involving a double and an int is to convert the result into a double anyhow.
See also https://docs.microsoft.com/en-us/cpp/cpp/standard-conversions?view=msvc-160#arithmetic-conversions

[infojunkie]

Thanks, dropping the cast also seems to pass the tests.

[infojunkie]

I've added more comprehensive tests to cover the different cases in the code, as per the attached screenshot.

These include manual tunings over regular and extended accidentals, as well as ensuring that the hard-coded accidentals in the export function are correctly handled.

[infojunkie]

I've squashed my commits and rebased to the latest 3.x. Waiting for tests then it's ready from my side.