if insert clef to measure after single-measure system with section break, then should not display courtesy clef

• Sep 6, 2015 - 11:14
Type
Functional
Severity
S4 - Minor
Status
closed
Project
MuseScore

Current behavior when inserting clefs to a *measure* (either by dragging from pallete to a measure or by selecting a whole measure and double clicking clef from palette) is to put them the new the *end* of the *previous* measure (except for case when first measure of score).

I would like to change this behavior such that if the measure is the start of a new section, then the new clef should be inserted (or modify currently existing clef) at start of the measure. For example, here are two measures with a sectionbreak in between:

before-add.png

If decide to insert bass clef on first measure of new section, then the new clef is currently inserted at end of previous measure:

after-add-bass-clef-to-new-section.png

But I'd like the new clef to be only inserted at beginning of the measure. This is because section breaks represent a partition in a multi-section score, such as multiple movements or a collection of separate (independent) scores, and so the behavior of adding a clef to the beginning of a section should be the same as adding a clef to the beginning of the score (which is at the *start* of the measure). To determine if a measure is start of a new section, I will add a new boolean test function MeasureBase::isStartOfSection() which checks for existence of a section break on the previous measure or on any measurebase between the previous measure and current measure, or returns false. Also if start of score, then isStartOfSection will return true. This function call will be used instead of measure->prevMeasure() test at: https://github.com/musescore/MuseScore/blob/master/libmscore/undo.cpp#L…

Comments

• Sep 7, 2015 - 23:52
Title if insert clef to measure after section break, should be added to start of measure, not end of previous measure ⇒ if insert clef to measure after single-measure system with section break, then should not display courtesy clef

credit to mgavioli for determining that specific condition causing the courtesy clef to be rendered is that the measure with section break is on a single-measure system:

see https://github.com/musescore/MuseScore/pull/2210#issuecomment-138263107 for explanation.

I'm changing title and marking this as a "bug".

Reply

• Sep 8, 2015 - 01:09

It seems to me the problem is not with the clef code itself, but with the System class and the method System::lastMeasure() used here by Clef::layout().

When called by Clef::layout(), the ml variable of the system containing the first measure is empty, i.e. the system does not know about any of its own measures (actually, just one, in this case) and the meas->system()->lastMeasure() call always return nullptr.

Then the test meas != meas->system()->lastMeasure() is always true (as the measure containing the clef is always different from null), showClef is set to true and the courtesy clef is shown.

The point is now to understand why the system is not set up to know about its own measures.

Reply

• Sep 8, 2015 - 03:33

I know the code used to accumulate measures into systems is kind of convoluted, and I fixed a number of bugs involving various aspects of that. It's possible I introduced a new one along the way, or just failed to fix this case. But the bugs I rememebr fixing were of a similar character - places where depending on how layoutSystemRow decided it was finished, it might actually not have accounted for all the measures (or frames, which were the main problems I was dealing with) correctly.

Reply

• Sep 8, 2015 - 10:38

I think I found the source of the problem, but I have no clear idea how to fix it.

It happens in the method Score::layoutSystem() (starting here ).

When the new clef is dropped, the score is re-laid out; during score layout, the measure list of each existing system is cleared (or is created empty, in case of a new system). Then Score::layoutSystem() is called for each system, which loops on system measures.

If the measure is the first measure of the system, Measure::minWidth2() is called here ; Measure::minWidth2() will call Score::computeMinWidth() which lays out the measure clefs (see here ).

However, the measure has not been appended to the system measure list yet; this will happen later at this line . During clef layout, the test for system final clef will fail, the clef will not be considered a courtesy clef and will be shown, rather than hidden.

This is the problem. Now, to fix it, I do not understand two things:

1) Can the addition of the measure to the system measure list be moved earlier? Probably not, as during the code in between, there are special cases treated differently.

2) Why the problem shows up for the system first measure only? After all, also the method Measure::minWidth1() used for non-system-initial measures invokes the same Clef::layout() invoked by Measure::minWidth2() and under the same conditions.

So, I'm stuck and I believe someone else should go on in this quest.

Reply

• Sep 8, 2015 - 10:41

Beside, the issue shows up for clefs, but clefs are only a symptom, the problem is with systems / measures layout.

Is it not be the case to update the title accordingly?

Reply

• Sep 8, 2015 - 14:03
Status (old) patch (code needs review) ⇒ active

Miwarre, please change the title how you see fit. I would have rewrote the description, but the timer expired, so I set the title to something that made sense with the description I already wrote (although I could have made a new issue instead).

Honestly it sounds like you have a much better idea of what is going on than I do. I would actually need to study the layout code quite a bit as I have yet to look at layout code. I'll unassign and wait a couple days, and if no one takes it up I'll take a deeper look.

Reply

• Sep 17, 2015 - 08:39
Status (old) active ⇒ patch (code needs review)

I think I've fixed it, but not sure if I went about it the right way...here is diff (and explainaion is in commit message): https://github.com/musescore/MuseScore/compare/master...ericfont:76006-…

Regarding #5:
>> "1) Can the addition of the measure to the system measure list be moved earlier? Probably not, as during the code in between, there are special cases treated differently."

Well I did move the append of the first measure of system ealier. However, I made sure to keep track of the state of !system->measures().isEmpty() from before performing this append and stored it in systemWasNotEmpty so as to not break the test of "if (systemWasNotEmpty && (minWidth + ww > systemWidth))". And I don't overwrite the bool isFirstMeasure until it is time to append a non-first measure. So I have preserved the functionality which ensures that the system has at least one measure.

>> "2) Why the problem shows up for the system first measure only?"

I believe the answer is because the first measure previously would always evaluate meas->system()->lastMeasure() to equal null since the system measure list existed but didn't have any measures in it, while subsequent measures would have a non-null value for meas->system()->lastMeasure().

I would appreciate you look at this before I submit PR. Honestly, I think there may be a simpler way to deal with appending measures to layoutSystem that might require more changes to the code. But I made this fix since I can be more assured that it doesn't change other functionality.

EDIT: tests tst_compat and tst_biab failed with apparetly different...I don't know what that is about. So I'm probably unaware of something important...will look into this tomorrow. Also test_tools failed but that always fails on my arch machine. tst_clef and tst_clef_courtesy passed though. (I'm attaching test results from my arch machine for reference). I did make PR https://github.com/musescore/MuseScore/pull/2222 to determine if passes on travis, because I'm suspect those tests are failing due to something with my machine, since chords seem entirely unrelated to my fix.

I am wondering if a simpler fix could be achieved by only adding test for system->isEmpty() inside of the clef::Layout() showClef calcuation.. UPDATE: travis does pass. So those test failures must be result of something about my arch linux machine being different than the official travis test machine. Please comment about my fix: https://github.com/musescore/MuseScore/pull/2222

Attachment Size
tsst_results_ef_fix-76006_commit-d5130f5c2d9aa28a5fc108c872560cf4172a0910.txt 18.25 KB

Reply

• Sep 17, 2015 - 15:43

hmm...one strange thing I've noticed about my above fix I discovered while testing, using the following sequence of steps:

step 1: start with 3-measure system
76006__step1.png

step 2: add clef to m2 and m3
76006__step2_add-clefs.png

step 3: add section break to m1 and m2
76006__step3_add-section-breaks.png
(notice that the courtesy clefs disappear as desired, since are at end of a section.)

step 4: undo (Ctrl->z) twice
76006__step4_undo.png

step 5: redo (Ctrl->Shift->z) twice
76006__step5_redo.png
Notice now have the undesired display of the courtesy clef at the end of the sections.

But then immediately performing any action that would trigger a re-layout of any measure (such as adding a note to that measure), the unwanted courtesy clefs disappear.

I don't have time at the moment to look into this glitch. This could just be a typical temporary glitch. Or it could be indicating that my PR is flawed.

Reply

• Sep 17, 2015 - 15:45

I can tell you that I have long noticed, but not quite been able to pin down, similar glitches with sections breaks and courtesy clefs and/or key/time signatures. Situations ehere depending on the order you did things in, you might get the courtesies despite the section break. So it's at least possible what you are seeing was there before. It is good to keep a working build of 2.0.2 around while developing to test against.

Reply

• Oct 28, 2015 - 16:02
Status (old) patch (code needs review) ⇒ fixed

Fixed in branch master, commit dec181560d

fix #76006 append first measure before perform minWidth2() calculation

Previously, during Score::layoutSystem() the very first measure was not actually added to the system until after minWidth2() performed its calculation, resulting in Clef::layout() incorrectly determining that a courtesy clef should be added to all single-measure systems ending with a section break. This is because teh showClef calculation in Clef::layout would evaluate (meas->system() && meas != meas->system()->lastMeasure()) to be true since meas->system()->lastMeasure() would be null at the time of executing minWidth2() for the first measure to be added to the system.

This fix first appends the initial measure of a system during Score::layoutSystem() before performing the minWidth2() calculation. This ensures that Clef::layout() doesn't have an empty meas->system().

In order to accommodate the appending of the first measure earlier in the code, I first make sure to grab the state of system->measures().isEmpty() at the top of the loop, before possibly appending the first measure. This ensures that will not break out of loop when performing calculation if (systemWasNotEmpty && (minWidth + ww > systemWidth)).

Also need to keep track of the value of isFirstMeasure until lower in the loop so can only append measure if did not already append measure

added test clef_courtesy03.mscx which has a single measure section m1. When add clef to m2, should not display courtesy clef at end of m1, because courtesy clefs should not be displayed at end of a section.

Reply

• Oct 28, 2015 - 16:16

Fixed in branch 2.0.3, commit a2acf5eed0

fix #76006 append first measure before perform minWidth2() calculation

Previously, during Score::layoutSystem() the very first measure was not actually added to the system until after minWidth2() performed its calculation, resulting in Clef::layout() incorrectly determining that a courtesy clef should be added to all single-measure systems ending with a section break. This is because teh showClef calculation in Clef::layout would evaluate (meas->system() && meas != meas->system()->lastMeasure()) to be true since meas->system()->lastMeasure() would be null at the time of executing minWidth2() for the first measure to be added to the system.

This fix first appends the initial measure of a system during Score::layoutSystem() before performing the minWidth2() calculation. This ensures that Clef::layout() doesn't have an empty meas->system().

In order to accommodate the appending of the first measure earlier in the code, I first make sure to grab the state of system->measures().isEmpty() at the top of the loop, before possibly appending the first measure. This ensures that will not break out of loop when performing calculation if (systemWasNotEmpty && (minWidth + ww > systemWidth)).

Also need to keep track of the value of isFirstMeasure until lower in the loop so can only append measure if did not already append measure

added test clef_courtesy03.mscx which has a single measure section m1. When add clef to m2, should not display courtesy clef at end of m1, because courtesy clefs should not be displayed at end of a section.

Reply

• Dec 22, 2015 - 18:39

The fix for this seems to have created another problem - could you guys look at #91471: Crash on load of score with horizontal frame taking an entire system? Forcing a measure to be added to the system is not appropriate if it won't fit because of an initial horizontal frame. Or, I suspect, if this measure won't fit because previous systems on this row have taken all the room, although it's possible we'd have detected this and returned by now in that case.

Reply

• Dec 22, 2015 - 21:06

Addendum: with the fix applied, if you add a clef after a section break on a system with only one measure, you don't get the scourtesy clef, which is good. And if you then insert a measure before that one measure, you still don't get a courtesy clef, which is also good. But if you then undo the measure insert, the courtesy clef reappears until you force another layout via Ctrl+A. I believe that is because the changes made here need to be duplicated (in whole or in part) in layoutSystem1(), which is used instead of layoutSystem() during an undo operation.

Reply

• Jan 24, 2016 - 00:51

I'm looking at revisiting this fix, because of #91471: Crash on load of score with horizontal frame taking an entire system. And see my comments in my initial PR for that: https://github.com/musescore/MuseScore/pull/2323

Regarding the original fix here, adding the first measure to the system seemed like a good idea, but it led to one measure always being added to a system even if it was not really appropriate (because a horizontal frame at the beginning of the system takes up too much space to allow a measure to fit). Hence issue 91471. I see two basic options here. My proposed fix simply removed the tenatively-added measure at the end of the function if we decide it doesn't fit after all. The other possibility would be to really figure out what is going on with courtesy clef generation and re-fix this bug (76006).

I keep going back and forth on this. One thing I don't yet understand why this was not a problem for systems of >1 measure. Yes, meas->system()->lastMeasure() would return non-null in Clef::layout(), but it would never be equal to "meas" when called at this stage, so we'd always be allocating space for it. I think what happens is that after layoutSystem() finishes figuring out how many measures fit, we then do these calculations again, further down in layoutSystemRow() where we call minWidth2(), and this time we get the answer right. What I don't understand yet is why this wasn't working for systems of only one measure.

However, I am now think it is bad that we get the answer wrong the first time then right the next time. This is probably contributing to the occasional "ping pong" effect that some scores exhibit where each time the score gets laid out, the layout bounces back and forth between N measures on a given system and N+1. Some important issues were fixed regarding this a year or more ago, but I still see scores posted occasionally that demonstrate it. Maybe this is the cause? I need to investigate this more.

Anyhow, I think at this point I want to keep the current fix for this issue (76006) but simply extend my fix for 91471 to also do the same during undo in layoutSystem1(). But I'm posting my thoughts as of now in case by chance it triggers some memory or insight. Anyhow?

Reply