musescore / MuseScore

MuseScore is an open source and free music notation software. For support, contribution, bug reports, visit MuseScore.org. Fork and make pull requests!
https://musescore.org
Other
11.82k stars 2.57k forks source link

ERROR - Element `beat-unit` #22974

Open Dima-S-Jr opened 1 month ago

Dima-S-Jr commented 1 month ago

Issue type

Import/export issue

Bug description

The mscz file I attached below is exported with errors in the code. Because of this, Dorico freezes when trying to play the score.

Steps to reproduce

MusicXML file error when importing to Dorico:

  1. Open the attached mscz file
  2. Export the file you opened to MusicXML
  3. Import the MusicXML file into Dorico
  4. A dialog box about the failed validation will appear.

Снимок экрана (368)

  1. Click on OK
  2. Click on Play
  3. Dorico freezes.

https://github.com/musescore/MuseScore/assets/134041121/75b5e301-f13a-4066-9dd3-a731ed813d4b

MusicXMLBug.zip

Screenshots/Screen recordings

  1. Dorico: Снимок экрана (368)

  2. MuseScore Studio 4: Снимок экрана (369)

MuseScore Version

MuseScore Studio version (64-bit): 4.3.0-241231433, revision: github-musescore-musescore-5f36e74

Regression

I don't know

Operating system

Windows 11

Additional context

I do not know if there has already been something like this on GitHub. This situation is happening so far only with the score I have attached. Since I need to import a file to Dorico in a short time, I really ask the participants who understand issues of this kind to explain to me why this is happening. I will try to resort to some workarounds to avoid such errors.

bkunda commented 1 month ago

It seems like this issue is occurring at the export stage. We can investigate it, but unfortunately it's not something we can prioritise as urgent at the moment.

Jojo-Schmitz commented 1 month ago

Doesn't crash for me, same OS, same MuseScore version, so I can only reproduce steps 1-6

At step 6 (close score just imported), do you save or not? (Doesn't make a difference to me, neither crashes)

At step 2 (export to MusicXML): which options do you use? Asking because I get that import error on line 554 rather than 621, exporting with "Manually added system and page breaks only", your error shows when exporting with "All layout" (but it still doesn't crash for me then)

Culprit XML:

        <direction-type>
          <metronome parentheses="no" default-x="-37.68" default-y="33.73" relative-y="20.00">
            <beat-unit></beat-unit>
            <per-minute>160</per-minute>
            </metronome>
          </direction-type>

Mu3 doesn't have that export issue BTW.

Changing that <beat-unit></beat-unit> to <beat-unit>quarter</beat-unit> fixes that error message on import (and is what Mu3 puts there). Whether it fixes the crash is nothing I can say, as I can't reproduce that

Jojo-Schmitz commented 1 month ago

Code got to be https://github.com/musescore/MuseScore/blob/60cbf530ae049188086d6677df3a987f951c7ddb/src/importexport/musicxml/internal/musicxml/exportxml.cpp#L4833-L4846 and this: https://github.com/musescore/MuseScore/blob/60cbf530ae049188086d6677df3a987f951c7ddb/src/importexport/musicxml/internal/musicxml/exportxml.cpp#L4881-L4890 both is pretty old code, no recent changes

Jojo-Schmitz commented 1 month ago

This might fix it:

diff --git a/src/importexport/musicxml/internal/musicxml/exportxml.cpp b/src/importexport/musicxml/internal/musicxml/exportxml.cpp
index 020f5ca17e..75d4ca9375 100644
--- a/src/importexport/musicxml/internal/musicxml/exportxml.cpp
+++ b/src/importexport/musicxml/internal/musicxml/exportxml.cpp
@@ -4836,6 +4836,9 @@ static bool findMetronome(const std::list<TextFragment>& list,

 static void beatUnit(XmlWriter& xml, const TDuration dur)
 {
+    IF_ASSERT_FAILED(dur.isValid()) {
+        return;
+    }
     int dots = dur.dots();
     xml.tag("beat-unit", TConv::toXml(dur.type()));
     while (dots > 0) {
Jojo-Schmitz commented 1 month ago

As might:

diff --git a/src/importexport/musicxml/internal/musicxml/exportxml.cpp b/src/importexport/musicxml/internal/musicxml/exportxml.cpp
index 020f5ca17e..75d4ca9375 100644
--- a/src/importexport/musicxml/internal/musicxml/exportxml.cpp
+++ b/src/importexport/musicxml/internal/musicxml/exportxml.cpp
@@ -4880,8 +4883,6 @@ static void wordsMetronome(XmlWriter& xml, const MStyle& s, TextBase const* cons
         xml.startElementRaw(tagName);
         int len1 = 0;
         TDuration dur;
-        TempoText::findTempoDuration(metroLeft, len1, dur);
-        beatUnit(xml, dur);

         if (TempoText::findTempoDuration(metroRight, len1, dur) != -1) {
             beatUnit(xml, dur);
Jojo-Schmitz commented 1 month ago

@miiizen, @rettinghaus, @lvinken?

Dima-S-Jr commented 1 month ago

Doesn't crash for me, same OS, same MuseScore version, so I can only reproduce steps 1-6

At step 6 (close score just imported), do you save or not? (Doesn't make a difference to me, neither crashes)

@Jojo-Schmitz, I just noticed the following. After the first restart of the system, I no longer observed such a crash. It's very strange. But I will change the issue description accordingly. Thanks for the comment.

At step 2 (export to MusicXML): which options do you use? Asking because I get that import error on line 554 rather than 621, exporting with "Manually added system and page breaks only", your error shows when exporting with "All layout" (but it still doesn't crash for me then)

Снимок экрана (371) No matter what type of file is being exported (compressed or uncompressed). Errors occur in both.

Changing that <beat-unit></beat-unit> to <beat-unit>quarter</beat-unit> fixes that error message on import (and is what Mu3 puts there).

And what can I do for my part to avoid such errors when exporting? What workarounds could you suggest?

Jojo-Schmitz commented 1 month ago

Short of manually correcting the XML or just ignoring that error there is no workaround

rettinghaus commented 1 month ago

@Jojo-Schmitz Your proposed solution would produce invalid MusicXML because once there is a metronome element there has to be a beat-unit as well. I'll play around with this a bit.

Jojo-Schmitz commented 1 month ago

I see. But code that potentially writes beat-unit twice (or once empty) seems wrong too

lvinken commented 1 month ago

Had a quick look. Using current master (commit 77c1c943) no MusicXML metronome element is generated when exporting MusicXMLBug.mscz. I don't have 4.3 available and no time to investigate further right now.

rettinghaus commented 1 month ago

The root of the problem is that there is a No-Break Space (U+00A0) in the tempo marking, which throws the metronome detection completely off track. If you put a normal space in there it works just fine. So it should be quite easy to fix?

Dima-S-Jr commented 1 month ago

The root of the problem is that there is a No-Break Space (U+00A0) in the tempo marking, which throws the metronome detection completely off track. If you put a normal space in there it works just fine. So it should be quite easy to fix?

@rettinghaus, thank you so much! Indeed, it helped. I can't even imagine where the non-breaking space could have come from (provided that I enter the tempo from the keyboard and always use a regular space). Thank you very much again)

Jojo-Schmitz commented 1 month ago

So No-Break Space should get added to the regular expression detecting tempo texts. Apparently Mu3 did better on this.

rettinghaus commented 1 month ago

@miiizen could you please take over?