fix #304353: melisma line overlaps next syllable | collect_artifacts #6081
Conversation
MarcSabatella
added
the
work in progress
libmscore/line.cpp
Outdated
| @@ -851,8 +857,10 @@ QPointF SLine::linePos(Grip grip, System** sys) const | |||
| extendToEnd = false; | |||
| } | |||
| } | |||
| ChordRest* right = extendToEnd ? cr : toChordRest(startElement()); | |||
| #endif | |||
There was a problem hiding this comment.
maybe better #else ... #endif?
There was a problem hiding this comment.
Will keep that in mind if I keep this code at all.
|
Looking at the vtests, lyrics-8 before: The lyrics-2 diff: |
|
For the dots, I guess the only reason the melisma appeared to (partially) cover them before was that extra overshoot. Probably I can extend the melisma line to cover the dots completely, but I note Gould does not do this in her examples (she doesn't discuss it explicitly), and doing so could possibly make differences like we see in lyrics-2 bigger / more common. So I am inclined not to cover the dot without a really good reason to. The difference in spacing in lyrics-2 is the sort of thing that is to be expected based on how this algorithm works - the whole point is to alter spacing. I wish it didn't affect cases where there was no overlap, but it does for pretty much the same reason that, for example, adding an accidental affects spacing even when it doesn't seem like it should. That said, I do think I can reduce the effect of this by not reserving quite so much horizontal space. I only need a thin barrier at the right edge, not the entire width of the note. I will be very interesting to know if you can try the artifact on one of your collections to see if you notice issues with spacing changes causing measure not to fit on a system, etc. Some amount of that may be unavoidable - there is no way to fix the overlap without adding space - but I am curious how big a difference it makes in practice. |
|
Fair enough. Now I'd just need to find a score of mine that does have this problem to check whether your PR solves it ;-) |
libmscore/chordrest.cpp
Outdated
| return false; | ||
| } | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Isn't this line unreachable? The only way the above while loop ends is by returning.
There was a problem hiding this comment.
Yes :-). This code is just temporary, I hope to replace it with code that doesn’t need to loop in this way at all, using the approach mentioned in the TODO
MarcSabatella
removed
the
work in progress
|
I have updated the code and removed the WIP label. The basic approach hasn't changed, but the way I identify whether a note ends a melisma or not has. Originally it was just a brute force search backwards for the previous lyric and seeing if it had a melisma that ends here. It got the job done but it's terribly expensive to do this each time we needed a chordrest shape. So I knew all along I'd need to replace it. Current version uses a flag _melismaEnd we set while laying out a lyric - find the end note, mark it. Then when we need to check if a note is melisma end, we can just check that flag. Only complication is clearing the flag when necessary. This can happen if we remove a melisma line or change its length. So I have code to do both. Along the way I fixed a couple other glitches that got in my way. In addition to the ones already mentioned in calculating the length of the melisma line, it turns out we weren't resetting the alignment of a lyric to centered when removing the melisma line (by editing the lyric and pressing space). Same for hyphenated melismas. This would fix itself on reload of the score. More testing and review definitely welcome, but as far as I know right now, this code works and is in the shape I would want it to be in. |
|
I'm still not understanding the process for tests. Last time I was able to see for myself which had diffs. Somehow I'm not seeing that info this time? But, I do see other test failures I need to investigate... |
|
The Travis CI mtest failures are due to crashes, not die to differences in output |
|
vtest "failures" are to be expected, the direct result of my improvements. |
MarcSabatella
force-pushed
the
304353-melisma-overlap
branch
from
7a88f42
to
a5876e7
Compare
|
Shouldn't these commits get squashed? |
|
Maybe, at some point it seems like we stopped doing that? |
|
@MarcSabatella we leave the commits separated if they solve different issues, or they are semantically separated. In your case, all commits relate to fixing one issue. So, please squash them. |
Resolves: https://musescore.org/en/node/304353 Melisma lines are spanners and therefore not laid out until well after note spacing and measure widths have been settled. This means there is no way for the melisma line to influence spacing. And yet, it needs to, if we want to avoid potential collisions. Because we cannot use the melisma line itself to affect spacing, this code borrows the approach used for lyrics themselves: calling addHorizontalSpacing() to reserve space for the melisma line when calculating the shape of the ChordRest. In order to do this, we need to know if a ChordRestr ends a melisma. So I have added a helper function, isMelismaEnd(), to check this. I also fixed a couple of issues in the calculation of the length of the melisma line. One was that we used the width of the start rather than end note (in cases where the next note also has a lyric, which is common). Another is that we were deliberately overshooting the right edge of the end note, whereas Gould says to end at the note.
MarcSabatella
force-pushed
the
304353-melisma-overlap
branch
from
a5876e7
to
9bb5f0c
Compare
|
Done. FWIW, I know that was always the policy in the past, but it seems lately sometimes I've seen comments indicating otherwise. |
Resolves: https://musescore.org/en/node/304353
Melisma lines are spanners and therefore not laid out until
well after note spacing and measure widths have been settled.
This means there is no way for the melisma line to influence spacing.
And yet, it needs to, if we want to avoid potential collisions.
Because we cannot use the melisma line itself to affect spacing,
this code borrows the approach used for lyrics themselves:
calling addHorizontalSpacing() to reserve space for the melisma line
when calculating the shape of the ChordRest.
In order to do this, we need to know if a ChordRestr ends a melisma.
So I have added a helper function, isMelismaEnd(), to check this.
I also fixed a couple of issues in the calculation
of the length of the melisma line.
One was that we used the width of the start rather than end note
(in cases where the next note also has a lyric, which is common).
Another is that we were deliberately overshooting
the right edge of the end note,
whereas Gould says to end at the note.
Currently a WIP. Functionally it is what I expect.
But I left TODO's in the code for clean up,
which I will do once there is buy-in on the results.