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

MusicXML import of common and cut time is too strict #20207

Closed
lvinken opened this issue Nov 26, 2023 · 7 comments · Fixed by #20197
Closed

MusicXML import of common and cut time is too strict #20207

lvinken opened this issue Nov 26, 2023 · 7 comments · Fixed by #20197
Assignees
Labels
MusicXML P3 Priority: Low

Comments

@lvinken
Copy link
Contributor

lvinken commented Nov 26, 2023

Issue type

Import/export issue

Bug description

The time signature with common symbol is imported only when the signature is 4/4, cut when 2/2. This is overly strict. In case common or cut is not recognized, the time signature is ignored and the nominal measure duration will be set to 4/4, which may be incorrect. See determineTimeSig() in files importmxmlpass1.cpp and importmxmlpass2.cpp.

Steps to reproduce

  1. import attached file Time_sig_common_cut_4_2_MS420.zip

Screenshots/Screen recordings

No response

MuseScore Version

MuseScore unstable prerelease for 4.2.0

Regression

No.

Operating system

Ubuntu 22.04

Additional context

No response

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Nov 26, 2023

We have:

    // determine if timesig is valid
    if (beats == "2" && beatType == "2" && timeSymbol == "cut") {
        st = TimeSigType::ALLA_BREVE;
        bts = 2;
        btp = 2;
        return true;
    } else if (beats == "4" && beatType == "4" && timeSymbol == "common") {
        st = TimeSigType::FOUR_FOUR;
        bts = 4;
        btp = 4;
        return true;
    } else

Would this:

    if (timeSymbol == "cut") {
        st = TimeSigType::ALLA_BREVE;
    } else if (timeSymbol == "common") {
        st = TimeSigType::FOUR_FOUR;
    } else if (!timeSymbol.isEmpty() && timeSymbol != "normal") {
        LOGD("determineTimeSig: time symbol <%s> not recognized with beats=%s and beat-type=%s",
        qPrintable(timeSymbol), qPrintable(beats), qPrintable(beatType));         // TODO
        return false;
    }

    btp = beatType.toInt();
    QStringList list = beats.split("+");
    for (int i = 0; i < list.size(); i++) {
        bts += list.at(i).toInt();
    }

be sufficient and pass muster?

As #20197 (master), #20198 (4.2.0) are in very close proximity and such a change would cause merge conflicts, I might add that there.

@cbjeukendrup
Copy link
Contributor

@Jojo-Schmitz I guess it would indeed be fine to include that in #20197 and #20198. That solution doesn't ensure that the time signature has correct measure length for those symbols (i.e. a whole note) but that's no hard requirement in MuseScore either, right?

@Jojo-Schmitz
Copy link
Contributor

Well, not ensuring the 'correct' measure length is what this issue is all about ;-)

@cbjeukendrup
Copy link
Contributor

cbjeukendrup commented Nov 27, 2023

Well there is a distinction between allowing 4/4 instead of 2/2 and allowing 3/8 instead of 2/2 :)

But yeah, I guess it's fine anyway.

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Nov 27, 2023

I guess this now allows allows for the just removed CUT_TRIPLE, 9/8

@lvinken
Copy link
Contributor Author

lvinken commented Nov 27, 2023

Wrt Jojo's proposed code, I do agree but would recommend removing the "with beats=%s and beat-type=%s" part from the LOGD, as it is no longer relevant.

Note that MuseScore's GUI allows setting the time signature symbol to common or cut for any time signature, without affect measure duration. E.g. you can create a 3/4 measure with time signature symbol common (whether that makes sense is different question). Why would the MusicXML importer be more restrictive ? In case of doubt, beats and beat type are more likely to be correct thatnthe symbol. See https://musescore.org/en/node/356715 for a real example.

@Jojo-Schmitz
Copy link
Contributor

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MusicXML P3 Priority: Low
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants