Fix #303623: Fix multiple issues with chord diagrams in MusicXML export #5958

thibault · 2020-04-16T19:10:52Z

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

There are multiple issues in the MusicXML annotations export that prevents chord diagrams to be exported in most cases:

chord diagrams are not exported unless there is a sibling harmony element.
only one chord diagram is exported for a chord.
chord diagrams that are not linked to a chord are not exported.
chord diagrams are not always exported to the correct voice.

This PR rewrites the harmony export algorithm to fix those issues.

This is my first contribution to MuseScore and moreover I'm not familiar with c++ at all, so there will probably be some stuff to fix.

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 · 2020-04-16T20:28:23Z

There are quite a few MusicXML tests failing, see https://travis-ci.org/github/musescore/MuseScore/jobs/675894505#L4385 ff.

thibault · 2020-04-17T05:55:33Z

importexport/musicxml/exportxml.cpp


-            for (const Element* e : seg->annotations()) {
+            int wtrack = -1; // track to write annotation


Despite my best efforts, I still don't understand what are those wtrack checks are used for. Since I'm not too sure, I did'nt add such a check in the following harmonies method. Fill free to tell me where I'm wrong.

I feel like this might be related to this issue?

Wtrack is the track where something is to be written. Required when MuseScore forces things in a staff's first track (e.g. staff text), when no note start at that time. This seems to no longer apply to harmonies.

Jojo-Schmitz · 2020-04-17T06:44:45Z

importexport/musicxml/exportxml.cpp

+            const Harmony* harmony = diagram->harmony();
+            if (harmony) {
+                  exp->harmony(harmony, diagram, offset);
+                  } else {


line break and less indent before else. Maybe make it an else if

Jojo-Schmitz · 2020-04-17T06:45:05Z

importexport/musicxml/exportxml.cpp

+                        const Element* defaultHarmony = harmonies.back();
+                        exp->harmony(toHarmony(defaultHarmony), diagram, offset);
+                        harmonies.pop_back();
+                        } else {


line break and less indent before else

Jojo-Schmitz · 2020-04-17T06:45:22Z

importexport/musicxml/exportxml.cpp

+      // That's why we need to explore the remaining segments to find
+      // `Harmony` and `FretDiagram` elements in Segments without Chords and output them now.
+      for (auto seg1 = seg->next(); seg1; seg1 = seg1->next()) {
+            if (!seg1->isChordRestType()) {


braces not needed here

Jojo-Schmitz · 2020-04-17T06:45:47Z

importexport/musicxml/exportxml.cpp

@@ -6444,4 +6471,3 @@ void ExportMusicXml::harmony(Harmony const* const h, FretDiagram const* const fd
      }

 }
-


unneeded change

Jojo-Schmitz · 2020-04-17T06:47:08Z

@lvinken: mind reviewing this?

thibault · 2020-04-17T07:32:32Z

Hi @Jojo-Schmitz, thank you for your review. I fixed the formatting issues you mentioned.

Jojo-Schmitz · 2020-04-17T07:34:36Z

And now it'd be good if you could squash them all into one commit, and making sure the commit title (not just the PR title) is "Fix #303623: Fix multiple issues with chord diagrams in MusicXML export"

Jojo-Schmitz · 2020-04-17T07:35:45Z

importexport/musicxml/exportxml.cpp

-                         seg, fd);
+            }
+
+      for (const Element* e: harmonies) {


braces not needed here

thibault · 2020-04-17T08:03:11Z

@Jojo-Schmitz Ok, I just force-pushed the squashed version of the branch.

lvinken · 2020-04-18T09:33:52Z

Will have a look, but don't know when yet.

lvinken · 2020-04-24T06:07:24Z

Started reviewing, may take a few days.

lvinken · 2020-04-24T06:42:10Z

mtest/musicxml/io/testHarmony6_ref.xml

+          <root-step>F</root-step>
+          </root>
+        <kind>major</kind>
+        <offset>1</offset>


This is incorrect (which is an existing bug), see https://musescore.org/en/node/304371.

Ok. Since this seems to be an existing bug that is not directly linked nor caused by this PR's modifications, how do you want to play this? Should this be corrected and added to the current PR or handled in a different one?

As long as the regression tests don't break, I can live with leaving this unsolved for some time. It does lead to invalid MusicXML, so I would want it fixed for 3.5.

lvinken · 2020-04-25T09:08:20Z

Review done, thanks for implementing this, as it works a lot better than my previous implementation. No further comments.

thibault · 2020-04-26T17:44:08Z

@lvinken Thank you for your review. @Jojo-Schmitz, is this good to merge?

Jojo-Schmitz · 2020-04-26T18:38:57Z

importexport/musicxml/exportxml.cpp

+            if (harmony) {
+                  exp->harmony(harmony, diagram, offset);
+                  }
+            else if (! harmonies.empty()) {


picky mode: that space after ! is not needed

Jojo-Schmitz · 2020-04-26T18:39:48Z

Looks good to me too, with one minor exception.

thibault · 2020-04-28T13:22:29Z

@Jojo-Schmitz Hi. I just updated the PR with a fix for your last comment.

thibault · 2020-05-06T07:55:54Z

Hi @Jojo-Schmitz. I'm not sure about how the MuseScore project's handles PR merging. This PR has been hanging for a week, should I just be patient or are further actions required from me?

Jojo-Schmitz · 2020-05-06T08:03:00Z

Patience. There a PRs that are several years old.
As far as I can tell it is ready to get merged. I'm not the one to decide though...

thibault · 2020-05-07T14:11:01Z

Hi @Jojo-Schmitz, I didn't want to rush you or anything, just make sure that I had done everything I could to get this fixed. I'll wait then :)

Jojo-Schmitz · 2020-05-07T14:16:33Z

You can't rush me here, I can't merge PRs ;-)

Jojo-Schmitz · 2020-11-08T09:30:29Z

Oh, seems the name 'testHarmony6' is taken meanwhile (since 870db63), so these would need to get renamed to 'testHarmony7'.
So on top of rebasing there are 2 renames and a one-line change in mtest/musicxml/io/tst_mxml_io.cpp.

vpereverzev · 2020-11-16T02:20:43Z

looking forward for rebase (on 3.x probably), then I'll merge it

vpereverzev · 2020-11-18T21:11:27Z

Still looking for rebase

vpereverzev · 2020-11-30T17:02:25Z

Archived due to inactivity (we can re-open it later once the author will return)

kwertyops · 2020-12-27T04:01:45Z

@Jojo-Schmitz and @vpereverzev --

To keep this moving along, I've forked this fork, done the rebase onto 3.x, and submitted a new pull request.

See here: #7167

Jojo-Schmitz · 2023-03-18T11:04:23Z

This seems still to be needed for master

…icXML export Port/rebase of musescore#5958 Fore some reason that PR's test files (testHarmony6.mscx, testHamony6_ref.xml) were already in master (as testHarmony7.mscx, testHamony76_ref.xml) but not used in the unit tests.

@thibault

…icXML export Port/rebase of @thibault's musescore#5958 For some reason that PR's test files (testHarmony6.mscx, testHarmony6_ref.xml) were already in master (as testHarmony7.mscx, testHarmony76_ref.xml), but not used in the unit tests.

@thibault