MuseScore
MuseScore copied to clipboard
musescore
Make sure gap rests correspond to the `<location>` gap duration
Make sure that filling gaps uses only rest with valid DurationType. This means sometimes adding more than one rest (still flagged as gap). Adjust the MEI exporter accordingly.
These are the images of the visual test failing
I am not sure what the difference is?
To give a little more background to the PR. When deleting rests when there is more than one voice, a location gap is added in the MuseScore file. A gap cannot always be represented with one single valid duration type (e.g., a half-note and a 16th note will be a gap of 9/16. This change makes sure that when the gap is filled, this is done with the number of rests needed to avoid invalid duration types.
Don't worry about thise vtests, the difference is minimal, see
and
But what about those failing unit tests?
But what about those failing unit tests?
Sorry, I missed these. I'll have a look.
I'm not 100% sure about those VTests. This looks like it's not just a rounding error. It might mean that gap rests can somehow affect horizontal spacing, which is of course not desirable, but also wouldn't be in scope of this PR to fix, in my opinion. Anyway, let's wait for a more official comment from the Engraving team.
I don't understand the reason for this change. What issue is it solving? If it isn't solving any specific issue, I would prefer it to stay as it is, because a) We are representing something that's doesn't exist as a musical element, it's just there to fill a gap, so it's better to have a single gap-filling object, why have more than one? b) There could be other logic in the program that relies on there being a single gap-rest. If we were solving an issue, it would be worth the risk of changing it, but if we are not why take that risk? c) If we split the gap rest into several rests, it also creates more stuff for the horizontal spacing system to compute (it's not as trivial as "just ignore them": they create segments, segments have a duration, duration corresponds to spacing, etc, there's some math in place to deal with that). That is likely the reason for the vtest differences, but it's also a waste of performance. Not big, but still a waste.
It solves problems (such as in the MEI exporter) where the gap is filled with a rest that is actually not representing the duration of the gap. This seems like problematic data to me. Making sure that rests used to fill gaps actually represent the duration of the gaps seems to be of higher priority than the performance impact of the extra processing yielded, which I expect to be not significant.
Also, one thing that I noticed is that MuseScore is not consistent when creating these gaps. If you take the following situation:
And delete the two invisible rests at the beginning of the second voice, MuseScore will create one location gap:
<location>
<fractions>9/16</fractions>
</location>
if you delete the 16th note rest first and the half-note rest after, but two location gaps:
<location>
<fractions>1/2</fractions>
</location>
<location>
<fractions>1/16</fractions>
</location>
if you delete the half-note reset first.
Loading the file with two location gaps will create two rests, so I assume that creating two rests when there is one location gap should be fine too.
(PS as it stands, MuseScore inserts a dotted half note rest for the 9/16 gap, which is one 16th note too long)
Gap rests have been known in the past to cause a lot of problems (score corruptions, most prominently), so forgive me if I am extremely hesitant to change anything related to them, especially now as we're entering a stabilization phase.
In general, my impression has always been that gap rests are a horrible hack to work around the limitations of our representation model, so I'd prefer to work out a comprehensive solution to improve the model (and if possible get rid of them entirely) rather than keep working with them.
@lpugin I do see the merit of the change you're proposing, and it may indeed be a good thing to do until we have a better solution to replace them. We'll come back to this after the release.
@mike-spa thank you for the feedback. I understand the need to be careful with this since score corruption is very annoying. I think the proposed change remains very minimal and should make things more consistent, which I would expect to be helpful. But of course, no problem for waiting until after the release.
@lpugin while I was working on a different issue, it has become clear to me that there is at least one scenario (and not even very rare) where gap rests need to be "promoted" to real rests. In which case, they are not anymore a "virtual" item that's there just to fill a space, but they become a notated element, which I think makes your work here now much more important (as it may improve/correct the actual notation, rather than just the underlying data). May I ask you to update your PR once more to fix the unit tests? I'd be happy to then merge it right away.