Ensure gap rest consistency - and several other fixes and improvements for situations with complext track mappings #23783
Conversation
mike-spa
force-pushed
the
ensureGapRestConsistency
branch
from
684cb6a
to
f7e865e
Compare
mike-spa
force-pushed
the
ensureGapRestConsistency
branch
3 times, most recently
from
6a27ad5
to
598524f
Compare
mike-spa
changed the title
| @@ -6039,359 +6046,353 @@ void Score::undoAddElement(EngravingItem* element, bool addToLinkedStaves, bool | |||
| ElementType::PEDAL, | |||
| ElementType::LYRICS | |||
| }; | |||
| voice_idx_t voice = track2voice(strack); | |||
| if (staff->isVoiceVisible(voice) && linkedTracks.empty() && muse::contains(VOICE1_COPY_TYPES, et)) { | |||
There was a problem hiding this comment.
It looks like VOICE1_COPY_TYPES is now unused. The comment above its definition suggests that it might have been important. Are you sure it's not necessary anymore?
There was a problem hiding this comment.
Oops, good catch, thanks
| <location> | ||
| <fractions>1/4</fractions> | ||
| </location> |
There was a problem hiding this comment.
Could you explain why these changes occur?
There was a problem hiding this comment.
Those changes occur because of the original commit from the comminity PR. I'll take this as an example. The second voice has 1/4 note and 3/4 of gap rest.
The gap rest isn't written in our files as an actual rest, but as a location move (which I don't like, but that's beside the point). In other words, we represent a 1/4 gap rest as a "move ahead by 1/4" instruction. Except, if the destination of the location move coincides with the end of the measure. In that case the location move isn't written because the start of the next measure resets the current tick anyway.
Now, the difference occurs because previously the gap rest was forced to always be a single rest, which means the underlying gap looks like this
but that gap of 3/4 doesn't get written in the file because the destination coincides with the measure end.
Now, however, we break gaps rests just like normal rests, which means the gap rests now look like this.
Now, the 1/4 rest gets written (that's what creates this diffs in the file), and the 2/4 will be skipped because it's end of the measure.
There was a problem hiding this comment.
Except, if the destination of the location move coincides with the end of the measure.
Ah, that explains it!
Of course, one could have an opinion about how much sense it makes that this causes the quarter rest to be "written" and the half rest not, but I think it doesn't really matter, since the whole concept of gaps and location tags probably won't exist for long anymore anyway.
* The removed block was creating a rest not necessarily corresponding to the gap, yield issues in the MEI exporter
mike-spa
force-pushed
the
ensureGapRestConsistency
branch
from
598524f
to
5547b4f
Compare
|
One final question: the VTests show some slight spacing difference. Perhaps that's not expected, is it? |
The horizontal spacing around invisible/hidden/gap rests is quite a complicated business (which also has some other problems of its own, see for instance #19850). The fact that splitting invisible rests can cause small spacing variation is expected, even though undesired. So at this stage I don't think it's a problem. |
Resolves: #21137
Resolves some long standing inconsistencies that we have with gap rests. There's more work to be done but I think we can be fine with this for now.
Substitute for #23555
Update: my initial solution turned out to be a bit too simplistic. In investigating the problem further, it turned out that we had some serious issues in
Staff::findLinkedStaffInStaff(which @miiizen had recently improved but not entirely fixed). Namely, the function was working correctly when searching for links in a part starting from the score, but could return completely wrong results when doing the opposite (searching for links in the score starting from a part). I ended up rewriting it, so it now hopefully returns correct results and is clearer too.