Combination of pickup measure and linked staff makes score corrupted

[mercuree]

Issue type

File corruption

Description with steps to reproduce

This issue is based on a report from #24970 (comment)
Two String Concerto No. 1 FrTB MS4 - Corrupted??.mscz.zip

1. Create empty score using mandolin instrument
2. Enter dotted half note
3. Go to measure properties
4. Set measure duration (actual) to 3/4
5. Check "Exclude from measure count"
6. OK
7. Create a linked staff
8. Change staff type to Tab. 4-str. common UPD. This step is not required
9. Try to save file
10. "This score has become corrupted" message appears

Reproduced in Musescore 4.0.2 and 4.5.0 master
Can't reproduce in Musescore 3.6.2

Supporting files, videos and screenshots

linked_staff_corruption.mp4

What is the latest version of MuseScore Studio where this issue is present?

MuseScore Studio version (64-bit): 4.5.0-250240302, revision: github-musescore-musescore-73c5d40

Regression

Yes, this used to work in MuseScore 3.x and now is broken

Operating system

windows 10

Additional context

No response

Checklist

• This report follows the guidelines for reporting bugs and issues
• I have verified that this issue has not been logged before, by searching the issue tracker for similar issues
• I have attached all requested files and information to this report
• I have attempted to identify the root problem as concisely as possible, and have used minimal reproducible examples where possible

[scorster]

@mercuree

Thanks for this post and the link to my post and attached score.

Interesting that the metric corruption occurs with just the pickup and linked staff. Those were there from the start and I only got the corruption alert, as reported, after adding the percussion staff and added a three measures and a few notes.

Thanks for being so gracious! After I posting in your other thread I noticed I was off topic. Should I delete my comment from that thread and place it here?

[mercuree]

@scorster

Those were there from the start and I only got the corruption alert, as reported, after adding the percussion staff and added a three measures and a few notes.

Interesting, but I can't reproduce it.
The corruption only happens when I create a linked staff, and the main staff must contain a pickup measure and the "exclude from measure count" flag must be enabled.
Do you have a version of the file without corruption?

Thanks for being so gracious! After I posting in your other thread I noticed I was off topic. Should I delete my comment from that thread and place it here?

Please don't delete your comment. I already reference it in this issue.

[scorster]

@mercuree

Thanks for your continuing interest in illuminating and eliminating corruption causes.

Here's an uncorrupted version of the score

Two String Concerto No. 1 FrTB MS4.mscz.zip

[mercuree]

@scorster

Here's an uncorrupted version of the score

This version not only does not have a percussion staff, but it also does not have a linked staff, so there is no corruption in it.
If you create a linked staff for any of the instruments and try to save the file, you will immediately get a corruption message

[scorster]

@mercuree wrote> If you create a linked staff for any of the instruments and try to save the file, you will immediately get a corruption message

Wondering if a corruption issue report has been opened on the matter of corruption occurring immediately upon adding a linked staff? I haven't been able to find one.

[mercuree]

@scorster sorry for the late response

Wondering if a corruption issue report has been opened on the matter of corruption occurring immediately upon adding a linked staff? I haven't been able to find one.

In this comment you have attached a corrupted file Two String Concerto No. 1 FrTB MS4 - Corrupted??.mscz.zip that definitely contains linked staff.
This staff is set to invisible but still causes an error.

Do you think you definitely didn't add it? Maybe you used some plugin that did it automatically?
The previous uncorrupted version does not contain that linked staff

[scorster]

@mercuree

I intentionally added the linked staff ... which apparently led to the file's metric corruption.

Regarding my use cases for adding and hiding the linked staff:

1. in this score I wanted the linked staff hidden in the main score and showing in a Part.

2. in a score with just an instrument + linked TAB I want in the main score:

• the option to show both staves (and I can do so)
• the option to show just the treble staff or just the tablature staff—depends on the student who's here and how I want the score to print. (and I can do so)
• Ideally—perhaps "alternately" is the better phrase— I'd like:

1. the options of setting the main score staff visibility as I want
2. and then set staff visibility as wanted within a Part, however I want (perhaps TAB only) ...
3. and additionally another Part with Treble only. (I'm not sure if 2 & 3 can coexist, but that's because I'm not deeply familiar with the way Parts work in MuseScore)

Whatever the case, adding a linked staff should not corrupt the score.

And I should have freedom to have various visibility settings throughout the score, without concern on Save.

Thanks for all your attention on this!

scorster

[DetunizedGravity]

I closed #27527 as duplicate but I would like to report here something that was shown in it but that does not appear here: the corruption only happens if the measure with the custom time signature contains two items (either can be a note or a silence) of decreasing duration. If all notes/silences in the measure are in order of increasing duration, the bug is not triggered.

How any of this could be linked to the fact that measure is excluded from the measure count is completely beyond me, though.

[skalra4]

Hello, I was working on #27527, I was curious how the tick is set for the anacrusis (pickup) measures. I'm trying to understand how that is copied over into the linked staff. In basic 4/4 time my understanding of a 4 beat measure is that Fraction(0,0) -> Fraction(1,4) -> Fraction(2,4) -> Fraction(3,4) -> Fraction(4,4) (which normalizes to 1)? I want to investigate if the copying of the ticks messes with how rests are being copied over? I've also noticed that when I reproduce the bug, the first segment as a tick == Fraction(0,1), I'm assuming this is the pickup? Could someone clarify this for me?

[cbjeukendrup]

@skalra4 Here is a visualisation of the data structure with regard to measures, segments, and staves:

The Measure class (well, MeasureBase) contains a member that stores the start tick of the measure. Segments contain a member that stores the tick of that segment relative to the beginning of its measure. Segment::measure returns the "absolute" tick of the segment.
Suppose the first segment has the duration of one 8th note; then the tick of the second segment is Fraction(1, 8). Suppose that second segment contains a 16th note; then the tick of the third segment is Fraction(1, 8) + Fraction(1, 16) = Fraction(3, 16).
(Also, suppose you have a score with two staves that contains a quarter note on the first staff, and two eight notes on the second staff; in this case, the shortest durations determine where there are segments, so in this case there are two segments that have a duration of Fraction(1,8).)

Recall from #27527 that the actual elements (Chords, Rests) are stored in the Segments' m_elist.
So when adding a linked staff, no Measure or Segment instances are duplicated; only the existing Segments' m_elist vector is lengthened, to accommodate the elements of the added staff. However, those new positions are initialised with nullptr, because by default there are no elements yet.
Then, at some point in Excerpt::cloneStaff, the elements from the old staff are copied to the new one.

To find out where exactly, you need to figure out where m_elist is modified. In Qt Creator, you'd place the cursor on the name m_elist (anywhere where that occurs, for example in segment.h) and press Ctrl+Shift+U (Cmd+Shift+U on Mac) to see where it is used. It's used in quite a few places, but only a handful of those are assignments (do include indexed assignments, like m_elist[track] = el;), so those are the places we're interested in. Set a breakpoint at every assignment, so that whenever something is put into a Segment's m_elist, you get the chance to inspect what kind of element is being added in which Segment (check the m_tick of the Segment in the debugger).

For example, in the example from #27527 (comment), you only want to see an assignment happen when the Segment has tick Fraction(0,0), Fraction(1,4) and Fraction(5,16), but according to the actual result, you should also unexpectedly see an assignment in a segment with tick Fraction(1,16). When that happens, look at the stack trace in the debugger, to find out how it can happen that a rest ends up at that tick.

[skalra4]

@cbjeukendrup Thank you very much. Seeing a visual of the data structure is very helpful. I will try this now. This is my first time using Qt Creator so I appreciate the tips and tricks for debugging.

[cbjeukendrup]

By the way, if you're more comfortable in another IDE, and you can find out how to use CMake-based projects with that other IDE, then you can use that as well, because the navigation and debugging tools that I mention should be available in pretty much any IDE.

[skalra4]

@cbjeukendrup I like Qt Creator. It's more about handling a large codebase that's unfamiliar to me. I usually use VSCode for University of Michigan projects, but we never need to handle such large-scale debugging. This is a good experience for me to learn more about the tools that all IDEs have to offer. I appreciate all the advice. It's making the process more enjoyable and making me want to figure out the issue more and more.

[skalra4]

Could someone explain to me the difference between the types Rest, MMRest, and ChordRest? I've been stepping through a one-measure score with an eighth, sixteenth, and quarter notes. From what I've noticed, when the linked staff is created, it is first designed as an empty standard staff with the rests sixteenth, eighth, and quarter. I suspect that the shuffling and cloning are incorrect. I believe when notes are getting cloned from the original staff, the duration timings are not matching with the standard stock staff, and that's why some extra notes might be getting added. If someone could give me some insight into the types, that would be very helpful.

[MarcSabatella]

Rest is any normal rest in your score. ChordRest is the parent class, that includes both rests and chords (one or more notes that share a stem etc). mmrest is multi-measure rest - those are what is displayed if you enable that option, where multiple adjacent empty measures are combined to show one horizontal bar with a number to indicate how many empty measures there are.

[cbjeukendrup]

From what I've noticed, when the linked staff is created, it is first designed as an empty standard staff with the rests sixteenth, eighth, and quarter.

That sounds plausible, actually. When the new staff is created, it is filled with rests, according to the default rests distribution, which, for a 7/16 pickup measure, is first a 16th, then an 8th and then a quarter rest.
But then, the original staff is cloned, as if those default rests had never been created. So now there's both those default rests, and everything that was cloned from the original staff.

Why does this only happen for anacrusis measures? That's because for normal measures, the default rests consist only of full-measure rests in the first segment of each measure, so this will be overwritten by whatever is cloned from the original staff.

I think we should not create those default rests first when cloning a staff, because they will/should be overwritten by whatever is cloned anyway. So the path towards a fix would be to figure out where those are created, and then ensure that that code doesn't run in the case of creating a linked staff.

[skalra4]

I'll look into that more. I'm still trying to confirm whether or not this is the exact reason, but I can maybe look into creating a condition check, for when the new measure segment is created with the default rest, and choose to build a new segment from the original, or maybe just making a deep copy of the measure/segment structure and giving it the correct staff id?

[skalra4]

I've also noticed that m_elist is never changed for the notes, only for the TimeSig, KeySig, and Clef. Unless I'm missing something, and m_elist is assigning the notes somewhere.

[skalra4]

I've drawn this for simplicity but I discovered this from manually walking through *m_next and *m_prev pointers in my debugger and examining the different segments.

This image shows the linked list structure of what happens for the basic case I've been working with. The problem is that the segments do not line up. As you can see, the first segment contains the eighth rest of the top staff of m_elist, but vector index 4 does not. The second or what that first segment *m_next points to contains nothing in index 0, but an eighth rest in index 4. This repeats with another segment, and then they sync up again. I still don't fully understand how the rests are being cloned, it has been hard to isolate that logic, stepping through. But I do now have concrete proof of where the mismatch occurs and why the score is different on a data structure level.

[skalra4]

I have found the exact reason why the "duplicate" rest appears. It is not actually a duplicate rest, it is a rest left behind from the default staff. In excerpt::cloneStaff if the original engravingObject* oe == 0, then it just moves to the next Segment object. So the first segment object's m_elist[4] correctly equals an eighth rest, however, because the next Segment has m_elist[0] == 0x0 but m_elist[4] == 1/8 Rest, the object does not get updated to replicate the empty segment like the original staff. I believe the change would require creating some sort of check for the given segment whether the original staff m_elist item is empty and the linked staff m_elist item contains something. This check needs to happen before the if(oe == 0 || oe->generated()). I would love some insight as to how to approach fixing this. This feels like a really specific situation that this would happen in. Below is a visual representation:

[DetunizedGravity]

I actually have an issue with the logic of merging the 2 tests "== 0" and "generated". If I understand well "generated" the test means "we don't need to act on this segment since it is has never been modified and so the copy is already identical". That's sound. But that's not the case of the "== 0" test. When one is copying over something that may not be empty, an empty value is something to be copied, not ignored. Unless this is valid in another context than ours, in which case the current code is logically wrong because incomplete.

[skalra4]

I wonder if there should be an overarching condition for whether the object == 0 and then inside THAT condition, check whether the object is generated or not?

[DetunizedGravity]

I haven't written C++ code for some 30 years now, but I believe that if the oe pointer is "== 0" it is not an instanciated object. So it has no "generated()" method to call. It might even be the reason for the "continue" here: "if there is no object to copy, then let's move to the next, especially because we would crash on the first method call on oe." But that logic would not be completely valid here: if there is a "0" pointer at that place of the original there probably should also be a "0" pointer in the copy, and in any case not some default value. I would try moving the "== 0" to its own "if (oe == 0) { ne = 0 }" but then you would have to update the "if (ne) { ... }" block later on, to make sure that it behaves as intended by us, and does not crash on an invalid pointer.

Please remember that unlike you, I can only make hypothesis by reading code on the github web site and your comments. You are much better placed than I am to understand the logic at play and make the good decisions. Case in point, the hypotheses I made in the duplicate ticket I had created were false. So take everything I write with caution.

[skalra4]

I've successfully cloned the staff properly! I was wondering if there is a specific testing framework or a way to do some QA assurance before I submit my pull request, just to make sure my change doesn't cause any unwanted behavior. Also maybe I can potentially add a test case for this in case it ever appears again?

[cbjeukendrup]

@skalra4 Great work! Sorry that I hadn't had time yet to reply to your comments earlier, but it's great to see you figured it out yourself. Feel free to go ahead and create the PR, so that we can already give feedback!

There is not really a QA procedure for contributors themselves; our internal QA team will test the PR before it is merged. But it is appreciated if you do some (relatively superficial) testing by yourself too.

You may want to add a unit test indeed. See src/engraving/tests. links_tests.cpp might be a good place to add a test case. I'd say, create a simple MSCX file with a one-staff 7/16 pickup measure with customised rests (that would previously cause a corruption when creating a linked staff). Then duplicate that file, to create a reference file, and in this reference file, create a linked staff. Then in the .cpp file, read the first MSCX file, then call the code to create a linked staff, and write the result to a file, then compare that file to the reference file. You can use the existing tests as a reference.

[DetunizedGravity]

@skalra4 Thank you so very much! :) This bug was a thorn in my side.

[skalra4]

@DetunizedGravity It was my pleasure! This has been a great first open-source contribution experience. I hope that my PR is accepted after I write the custom test. I'm looking forward to contributing more!

[mercuree]

@skalra4

I'm looking forward to contributing more!

My suggestion: There is an issue that has been living in Musescore for 11 years (since Musescore 2.0) and apparently nobody cares😁. I think it's not hard to fix. If you're interested, here's the link and if needed, I can provide a little more information there.

[skalra4]

Hello! I return with questions! I am trying to write the unit test for this case but the file I am writing seems to be corrupted even though inspecting the data structure it now has the correct m_elist order. I was wondering if someone could give me some insight into how to use the ScoreComp and ScoreRW functions correctly. I'm not sure I'm following all the necessary intermediate steps to ensure I produce a valid score. Here is a picture of my current test case:

I will be adding some robust commenting in my next commit but I'd like to get this working so I can check it off in my PR. Any advice would be greatly appreciated!

[cbjeukendrup]

The only thing that seems different in StaffSettingsModel::createLinkedStaff is that staff would be created with ostaff->clone(). And perhaps a staff->setScore() is necessary. But for the rest it basically looks right.

Two unrelated naming convention things:

• instead of String outputPath = ScoreRW::rootPath() + u"/" + LINKS_DATA_DIR + u"testPickupLinkedStaffCloned.mscz";, do String outputPath = u"testPickupLinkedStaff.mscz"; (so that the file is written to the working directory, like in the other tests)
• instead of u"testPickupLinkedStaffReference.mscz";, call that file u"testPickupLinkedStaff-ref.mscz, for consistency with other tests.

OH wait. ScoreComp::saveCompareScore only works with MSCX files, not MSCZ. MSCZ files are compressed and thus binary, rather than text, and saveCompareScore uses diff to do text file comparison.
You can create a MSCX file by choosing the "Uncompressed MSCX folder" option in the "Save as…" dialog. A problem is that the latest MuseScore versions write MSCX files in a slightly different way: they create an entire folder with different files. What you can do, is that you just take only the MSCX file out of that folder, and put it in the LINKS_DATA_DIR folder. Then you may need to set useRead302InTestMode to false at the beginning of the test case, and back to its original value at the end. See for example TEST_F(Engraving_BeamTests, drumKitBeam).

[skalra4]

I have tried to save the file as .mscx, but no matter whether I select "Uncompressed experimental folder", it does not work. It continues to only save the file as .mscz.

[cbjeukendrup]

You may need to manually remove the .mscz extension from the filename field, or even manually add the .mscx extension instead. On macOS, the "Show all filename extensions" setting in Finder > Settings > Advanced, may also influence the behaviour.