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

[MU4] Fix #329470: Wrong/missing transpositioning on MusicXML import #10586

Merged
merged 2 commits into from
Apr 25, 2022

Conversation

Jojo-Schmitz
Copy link
Contributor

@Jojo-Schmitz Jojo-Schmitz commented Feb 16, 2022

If the optional element "diatonic" is missing, calculate it from "chromatic".

Resolves: https://musescore.org/en/node/329470
Same issue in 3.x.

Also adds/enables all (most?) available MusicXML unit tests (commenting out those that fail, working on those is out of score of this PR).

@Jojo-Schmitz
Copy link
Contributor Author

@lvinken: please review

Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Feb 16, 2022
Warn (in Debug" mode) if the mandatory element "chromatic" is missing or 0.
If the optional element "diatonic" is missing, calculate it from "chromatic".

Backport of musescore#10586
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Feb 16, 2022
Warn (in Debug mode) if the mandatory element "chromatic" is missing or 0.
If the optional element "diatonic" is missing, calculate it from "chromatic".

Backport of musescore#10586
@lvinken
Copy link
Contributor

lvinken commented Feb 17, 2022

As indeed diatonic is optional but chromatic is required, the code change is correct. I apparently overlooked the possibility of missing diatonic, which led to an invalid interval in my implementation. Thanks for fixing this.

Note that a missing chromatic would be caught by the schema validation.

A testfile would be a nice addition, to prevent this requirement from being missed in future.

@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Feb 17, 2022

OK, I'll drop the check for the mandatory "chromatic", as it is caught elsewhere already.

@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Feb 17, 2022

I see a testChangeTranspose.xml, which we might use for this, by making a copy (e.g. testChangeTranspose-no-diatonic.xml) and removing the optional diatonic, but I don't see where that test is actually run, so don't know where and how to add it (or them both)
That test is used in 3.x (as void changeTranspose() { mxmlIoTest("testChangeTranspose"); } in tst_mxml_io.cpp), but apparently not in master? Added it back and it does pass the test too.
Can't get the new test void changeTranspose() { mxmlIoTestRef("testChangeTranspose-no-diatonic"); } to work though.

Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Feb 17, 2022
Warn (in Debug mode) if the mandatory element "chromatic" is missing or 0.
If the optional element "diatonic" is missing, calculate it from "chromatic".

Backport of musescore#10586
@Jojo-Schmitz Jojo-Schmitz force-pushed the transpose-xml branch 6 times, most recently from 0204097 to 015f7cc Compare February 17, 2022 14:10
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Feb 17, 2022
If the optional element "diatonic" is missing, calculate it from "chromatic".

Backport of musescore#10586
ToDo: get the mtest to work
@Jojo-Schmitz Jojo-Schmitz force-pushed the transpose-xml branch 3 times, most recently from ee2dea6 to 10217ab Compare February 18, 2022 07:53
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Feb 18, 2022
If the optional element "diatonic" is missing, calculate it from "chromatic".

Backport of musescore#10586
@Jojo-Schmitz
Copy link
Contributor Author

As far as I'm concerned this PR is ready for getting merged ;-)

@lvinken
Copy link
Contributor

lvinken commented Feb 20, 2022

Looks good to me, thanks.

@Jojo-Schmitz Jojo-Schmitz force-pushed the transpose-xml branch 3 times, most recently from 2b9cd54 to 7ce5528 Compare February 23, 2022 07:31
Jojo-Schmitz referenced this pull request Feb 23, 2022
Export the volta text to the MusicXML ending element, allowing custom text such as "1-3".

Backport of #10623, plus 2 more mtest fixes
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Feb 24, 2022
If the optional element "diatonic" is missing, calculate it from "chromatic".

Backport of musescore#10586
If the optional element "diatonic" is missing, calculate it from "chromatic".
but disable the failing ones.
Additionally fix one test and sort them alphabetically.
@igorkorsukov igorkorsukov merged commit 2723468 into musescore:master Apr 25, 2022
@Jojo-Schmitz Jojo-Schmitz deleted the transpose-xml branch April 25, 2022 13:45
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request May 12, 2022
If the optional element "diatonic" is missing, calculate it from "chromatic".

Backport of musescore#10586
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Mar 5, 2023
If the optional element "diatonic" is missing, calculate it from "chromatic".

Backport of musescore#10586
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

3 participants