GSoC 2020: measure repeats #6365

IsaacWeiss · 2020-07-23T01:17:26Z

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

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

libmscore/layout.cpp

IsaacWeiss · 2020-07-31T01:20:42Z

libmscore/layout.cpp

@@ -3735,7 +3721,7 @@ System* Score::collectSystem(LayoutContext& lc)
                } else {
                    m->removeSystemHeader();
                }
-                while (true) {
+                for (;;) {
                    // TODO: what if the nobreak group takes the entire system - is this correct?


@MarcSabatella I think the answer is yes, but why might it not be?

No idea, but somehow it seems like something to be sure to test.

mu4/domain/importexport/internal/musicxml/importmxmlpass2.cpp

IsaacWeiss · 2020-08-06T01:59:37Z

For the record, the answer to that "(?)" in the latest commit message is a no. Doesn't produce correct output if the MeasureRepeat is in a multi-staff instrument (@MarcSabatella, testing and insights on this welcome).

MarcSabatella · 2020-08-07T20:17:12Z

mu4/domain/importexport/internal/musicxml/exportxml.cpp

+    } else {
+        Part* part = m->score()->parts().at(partIndex);
+        for (int i = 0; i < part->nstaves(); ++i) {
+            int staffIdx = part->staff(i)->idx();


Probably there is some slightly more clever way of dealing with this - if you look at the implementation of idx(), you'll see it's relying on indexOf, which is probably slow operation. Not that it really matters, and I know I told you this was the way to do it. But still, you asked for feedback, that's what I've got... :-)

Improved efficiency by only calling it once, before the loop commences.

mu4/domain/importexport/internal/musicxml/exportxml.cpp

IsaacWeiss · 2020-08-10T03:01:25Z

Some notes on MusicXML handling of measure repeats in Sibelius (Ultimate 2020.6), Finale (26.3.1.520), and MuseScore (this branch):

Sibelius is just bad. It doesn't include the "number" attribute in the <measure-style> tag surrounding the <measure-repeat> tag (for when a part/instrument has more than one staff and the measure repeat is not the same in all staves); it doesn't write the "slashes" attribute; it can't even import its own exports.
Finale is better, unsurprisingly; I think MusicXML started there. Finale writes the "slashes" attribute as well as the staff "number", although it does not repeat the underlying content as the spec demands. Its export is definitely better than its import. The software actually only supports one- and two-measure repeats, and thus converts a four-measure repeat into two consecutive two-measure repeats on import (not good). Strangely, it also gets confused when importing its own exports, getting the end measure wrong.
MuseScore up to now has had no support for importing or exporting (and, of course, only one-measure repeats were supported in the software). I have implemented export for one-measure repeats as well as the new subtypes, including the "slashes" attribute (although I have yet to see a legitimate case where it is meaningful), the "number" attribute in the <measure-style> tag, and repeating all the content. This export is read perfectly by Finale, and, as designed, the repitition of the content gives Sibelius something that it can understand as an alternative. The import I have implemented ignores the "slashes" attribute if present, as there is for our purposes an identity between the slashes in the symbol and the number of measures it indicates. But it reads both Sibelius and Finale export correctly (or, in the case of Sibelius export, it reads it at least as well as Finale does).

Everything is thoroughly commented and commits have been squashed. I would welcome review from MusicXML specialist @lvinken.

IsaacWeiss · 2020-08-10T03:08:01Z

Btw, for easier review of the export commit, enable "Hide whitespace changes." Most of writeMeasureTracks() was just indented one additional level.

lvinken · 2020-08-10T19:55:48Z

I am perfectly willing to review, but I am severely time-limited at the moment. Most likely I will be unable to spend any time on this for about six weeks :-(.

lvinken · 2020-08-14T04:44:11Z

Due to an unexpected change of plans, I may have time to look into this on short notice after all. will keep you posted.

lvinken · 2020-08-14T05:47:23Z

Initial results (after clean build on MacOS):
tst_mxml_io fails on:

testBreaksMMRest.xml
testMultiMeasureRest1.xml
testMultiMeasureRest2.xml
testMultiMeasureRest3.xml
testMultiMeasureRest4.xml
testTboxAboveBelow3.xml
testTboxWords1.xml
-> this probably indicates bugs are still present in the MusicXML import/export changes.

I also get a consistent crash on exiting MuseScore, but this may very well be unrelated. Stack trace:
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0 org.qt-project.QtQml 0x0000000119a0e3ad QQmlMetaType::qmlSingletonTypes() + 61
1 org.qt-project.QtQml 0x00000001199e2231 QQmlEngine::~QQmlEngine() + 81
2 org.qt-project.QtQml 0x00000001199e23ae QQmlEngine::~QQmlEngine() + 14
3 org.qt-project.QtCore 0x0000000119f136fb QObjectPrivate::deleteChildren() + 203
4 org.qt-project.QtCore 0x0000000119f134f7 QObject::~QObject() + 1831
5 org.musescore.MuseScore 0x000000011024f36e mu::framework::UiEngine::~UiEngine() + 158 (uiengine.cpp:54)
6 org.musescore.MuseScore 0x000000011024ee75 mu::framework::UiEngine::~UiEngine() + 21 (uiengine.cpp:54)
7 libsystem_c.dylib 0x00007fff7fceaeed __cxa_finalize_ranges + 351
8 libsystem_c.dylib 0x00007fff7fceb1fe exit + 55
9 libdyld.dylib 0x00007fff7fc3e01c start + 8

Finally, I seem to be unable to find any MusicXML test files. Did you create any ? If you did not, please do so.

IsaacWeiss · 2020-08-14T23:37:59Z

mu4/scenes/inspector/models/notation/measurerepeats/measurerepeatsettingsmodel.h

@@ -7,7 +7,7 @@ class MeasureRepeatSettingsModel : public AbstractInspectorModel
 {
    Q_OBJECT

-    Q_PROPERTY(PropertyItem* numberPosition READ numberPosition CONSTANT)
+    Q_PROPERTY(PropertyItem * numberPosition READ numberPosition CONSTANT)


IsaacWeiss · 2020-08-14T23:54:37Z

@lvinken thanks, I've been dealing with a transition to an unfamiliar computer/operating system and only just got everything into place. I'll have a look at those test failures and create tests next week.

lvinken · 2020-08-15T07:25:46Z

Hi Isaac, FYI, I also plan to review your code changes to the MusicXML importer/exporter, but haven't found the time to do so yet.

Wrt validating the behaviour, that is somewhat difficult. I find the text on http://usermanuals.musicxml.com/MusicXML/MusicXML.htm#CT-MusicXML-measure-repeat.htm quite vague and hard to understand. Furthermore, I do not have access to Finale. I do have Finale Notepad, which does not allow me to create measure repeats, but at least seems to import them.

MarcSabatella · 2020-08-28T18:02:16Z

libmscore/scoreElement.h

@@ -399,7 +399,8 @@ class ScoreElement
 #undef CONVERT

    virtual bool isElement() const { return false; }   // overridden in element.h
-    bool isChordRest() const { return isRest() || isChord() || isRepeatMeasure() || isMMRest(); }
+    bool isRestFamily() const { return isRest() || isMMRest() || isMeasureRepeat(); }


The reason I suggested "Type" here is that I was thinking of BarLineType that we used to check for a few related segments types in the same way. But, that's a bit different, so maybe not important to mimic its naming.

IsaacWeiss · 2020-08-28T20:36:34Z

All done.

IsaacWeiss · 2020-09-06T03:15:02Z

Re-rebased

- Fix #279071: "Don't Break" element creates hole at end of system - Allow more than two measures to be grouped - Allow adding/toggling NOBREAK by clicking palette - Rename to "Group Measures" - Adjust placement of break symbols Co-authored-by: MarcSabatella <marc@outsideshore.com>

- Public before private - Members prefixed with "m_"

…btypes 1/2/4 - Draw with font symbols instead of QPainterPath - Automatically group measures and delete contents when adding MeasureRepeat - Bring measure.h and measure.cpp in line with new code guidelines - Hide rests by making measures (per MStaff) aware if they are part of measure repeat group - Keep measures and element in sync at all times by handling (preventing/warning/special-casing) many user actions - Add style options for numbering consecutive one-measure repeats

MMRests: - Add vtests for old-style (all fonts), h-bar thickness and margin (Bravura) MeasureRepeats: - Add script tests for adding, cutting/copying with partial group selected, pasting into group, repeating selection from before group, executing split/join/insert/delete measure commands, adding repeat barlines, and adding time signature - Add mtests for parts, MIDI, and MusicXML import/export - Add vtests - Update ref files

…re#6365

…style-options Bring back style options for Measure Repeats introduced in PR #6365

- Fix #279071: "Don't Break" element creates hole at end of system - Allow more than two measures to be grouped - Allow adding/toggling NOBREAK by clicking palette - Rename to "Group Measures" - Adjust placement of break symbols Co-authored-by: MarcSabatella <marc@outsideshore.com> Backport of musescore#6365, commit 1, plus some includes cleanup

…btypes 1/2/4 - Draw with font symbols instead of QPainterPath - Automatically group measures and delete contents when adding MeasureRepeat - Bring measure.h and measure.cpp in line with new code guidelines - Hide rests by making measures (per MStaff) aware if they are part of measure repeat group - Keep measures and element in sync at all times by handling (preventing/warning/special-casing) many user actions - Add style options for numbering consecutive one-measure repeats Backport of musescore#6365, commit 3, plus some includes cleanup

…btypes 1/2/4 - Draw with font symbols instead of QPainterPath - Automatically group measures and delete contents when adding MeasureRepeat - Hide rests by making measures (per MStaff) aware if they are part of measure repeat group - Keep measures and element in sync at all times by handling (preventing/warning/special-casing) many user actions - Add style options for numbering consecutive one-measure repeats Backport of musescore#6365, commit 3, plus some includes cleanup

IsaacWeiss force-pushed the gsoc-2020-measure-repeats branch 6 times, most recently from 6a531df to 0828d65 Compare July 30, 2020 04:26

IsaacWeiss force-pushed the gsoc-2020-measure-repeats branch from 0828d65 to eab53a9 Compare July 30, 2020 23:11

IsaacWeiss commented Jul 31, 2020

View reviewed changes

libmscore/layout.cpp Outdated Show resolved Hide resolved

libmscore/layout.cpp Outdated Show resolved Hide resolved

libmscore/layout.cpp Outdated Show resolved Hide resolved

IsaacWeiss commented Jul 31, 2020

View reviewed changes

IsaacWeiss force-pushed the gsoc-2020-measure-repeats branch 3 times, most recently from d0dde1a to d6e2d39 Compare August 3, 2020 04:43

MarcSabatella reviewed Aug 3, 2020

View reviewed changes

mu4/domain/importexport/internal/musicxml/importmxmlpass2.cpp Outdated Show resolved Hide resolved

MarcSabatella reviewed Aug 7, 2020

View reviewed changes

mu4/domain/importexport/internal/musicxml/exportxml.cpp Outdated Show resolved Hide resolved

MarcSabatella reviewed Aug 7, 2020

View reviewed changes

mu4/domain/importexport/internal/musicxml/exportxml.cpp Outdated Show resolved Hide resolved

IsaacWeiss force-pushed the gsoc-2020-measure-repeats branch 2 times, most recently from 84e38ba to 19943c2 Compare August 10, 2020 03:00

IsaacWeiss force-pushed the gsoc-2020-measure-repeats branch 2 times, most recently from 2315258 to 03f49e6 Compare August 14, 2020 23:35

IsaacWeiss commented Aug 14, 2020

View reviewed changes

IsaacWeiss force-pushed the gsoc-2020-measure-repeats branch from 03f49e6 to 84ff82f Compare August 16, 2020 19:36

MarcSabatella reviewed Aug 28, 2020

View reviewed changes

IsaacWeiss force-pushed the gsoc-2020-measure-repeats branch from 357a177 to c2296e6 Compare August 28, 2020 19:43

IsaacWeiss mentioned this pull request Sep 4, 2020

Algorithm for spreading staves/systems over a page #6538

Merged

8 tasks

IsaacWeiss force-pushed the gsoc-2020-measure-repeats branch from c2296e6 to dc23d2a Compare September 6, 2020 03:13

IsaacWeiss mentioned this pull request Nov 20, 2020

[MU4] Default workspace #6897

Merged

IsaacWeiss added 5 commits November 20, 2020 11:38

Bring measure.h and measure.cpp in line with new code guidelines

a1b5567

- Public before private - Members prefixed with "m_"

MusicXML import/export and (probably) Guitar Pro import MeasureRepeats

2244111

IsaacWeiss mentioned this pull request Nov 20, 2020

DO NOT MERGE #6898

Closed

Additional changes needed after rebasing

0777643

IsaacWeiss force-pushed the gsoc-2020-measure-repeats branch from dc23d2a to 0777643 Compare November 22, 2020 03:31

vpereverzev merged commit 28e5d11 into musescore:master Nov 22, 2020

Jojo-Schmitz mentioned this pull request Nov 24, 2020

[MU4] Fix build, and warnings too #6925

Merged

IsaacWeiss deleted the gsoc-2020-measure-repeats branch December 27, 2020 14:16

This was referenced Feb 12, 2021

[MU4] Bring back style options for multi-measure rests #7513

Merged

Update Style dialog from MuseScore 3 #7462

Merged

cbjeukendrup added a commit to cbjeukendrup/MuseScore that referenced this pull request Feb 19, 2021

Bring back style options for Measure Repeats introduced in PR musesco…

296ecea

…re#6365

cbjeukendrup mentioned this pull request Feb 19, 2021

Bring back style options for Measure Repeats introduced in PR #6365 #7581

Merged

RomanPudashkin added a commit that referenced this pull request Feb 19, 2021

Merge pull request #7581 from cbjeukendrup/bring-back-measure-repeat-…

6196893

…style-options Bring back style options for Measure Repeats introduced in PR #6365

Jojo-Schmitz mentioned this pull request Jun 21, 2024

Fix GH#16227: MusicXML export is incorrect when using local timsig Jojo-Schmitz/MuseScore#509

Merged

Jojo-Schmitz mentioned this pull request Jun 24, 2024

GSoC 2020: Measure repeats for Mu3 Jojo-Schmitz/MuseScore#519

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GSoC 2020: measure repeats #6365

GSoC 2020: measure repeats #6365

IsaacWeiss commented Jul 23, 2020 •

edited

Loading

IsaacWeiss Jul 31, 2020

MarcSabatella Jul 31, 2020

IsaacWeiss commented Aug 6, 2020

MarcSabatella Aug 7, 2020

IsaacWeiss Aug 10, 2020

IsaacWeiss commented Aug 10, 2020 •

edited

Loading

IsaacWeiss commented Aug 10, 2020

lvinken commented Aug 10, 2020

lvinken commented Aug 14, 2020

lvinken commented Aug 14, 2020

IsaacWeiss Aug 14, 2020

IsaacWeiss commented Aug 14, 2020

lvinken commented Aug 15, 2020

MarcSabatella Aug 28, 2020

IsaacWeiss commented Aug 28, 2020

IsaacWeiss commented Sep 6, 2020

GSoC 2020: measure repeats #6365

GSoC 2020: measure repeats #6365

Conversation

IsaacWeiss commented Jul 23, 2020 • edited Loading

IsaacWeiss Jul 31, 2020

Choose a reason for hiding this comment

MarcSabatella Jul 31, 2020

Choose a reason for hiding this comment

IsaacWeiss commented Aug 6, 2020

MarcSabatella Aug 7, 2020

Choose a reason for hiding this comment

IsaacWeiss Aug 10, 2020

Choose a reason for hiding this comment

IsaacWeiss commented Aug 10, 2020 • edited Loading

IsaacWeiss commented Aug 10, 2020

lvinken commented Aug 10, 2020

lvinken commented Aug 14, 2020

lvinken commented Aug 14, 2020

IsaacWeiss Aug 14, 2020

Choose a reason for hiding this comment

IsaacWeiss commented Aug 14, 2020

lvinken commented Aug 15, 2020

MarcSabatella Aug 28, 2020

Choose a reason for hiding this comment

IsaacWeiss commented Aug 28, 2020

IsaacWeiss commented Sep 6, 2020

IsaacWeiss commented Jul 23, 2020 •

edited

Loading

IsaacWeiss commented Aug 10, 2020 •

edited

Loading