musescore
Spacing should not kern over chords with articulations or chords which start/end slurs
This PR is based on #13218, so that should be merged first. I've split the two things to collect vtests separately.
Before:
After:
Maybe I've found a bug:
- Create a situation with chords and accidentals where kerning would occur
- Add a slur -> kerning disabled
- Flip slur -> kerning still disabled, but I would expect it to be enabled again.
Ha! Great catch. That's because in #13218 I hadn't extracted the full direction logic, but only the "auto" portion of it. I've corrected that now
I found it's still not 100% "waterproof" when multiple slurs are used. After some fiddling with adding/removing/flipping slurs, I managed to create this situation (it's somewhat artificial, but I think it could happen with non-artificial multi-slur situations too):
It must be that the _startSlur of the first chord becomes the downwards slur, and the big upwards slur gets overlooked.
It must be that the
_startSlurof the first chord becomes the downwards slur, and the big upwards slur gets overlooked.
Yeah, the chord is storing only one _startSlur and one _endSlur. To make it general, I would have to make a vector of startSlurs and a vector of endSlurs and I wasn't sure it would be worth it, that's why I didn't (and I actually expected this to possibly happen). I could try though, maybe the performance hit isn't significant
It must be that the
_startSlurof the first chord becomes the downwards slur, and the big upwards slur gets overlooked.Yeah, the chord is storing only one
_startSlurand one_endSlur. To make it general, I would have to make a vector of startSlurs and a vector of endSlurs and I wasn't sure it would be worth it, that's why I didn't (and I actually expected this to possibly happen). I could try though, maybe the performance hit isn't significant
I was thinking about another alternative solution: instead of vectors of slurs, we could store: bool _startsOrEndsUpSlurs and bool _startsOrEndsDownSlurs. That probably requires that we call chord->computeIsSlurStartEnd after LayoutMeasure::preComputeSlurUp has been done. That's a bit more dangerous because we need to ensure that preComputeSlurUp and computeIsSlurStartEnd are done in the correct order and both are done before computing kerning, but I can imagine the performance will be better. Would that be possible?
(Also something I'd like to hear @vpereverzev's feedback on first 🙂)
For the rest, it seems unnecessary to me to have different vectors for start and end slurs; we can just throw them all together in one vector.
instead of vectors of slurs, we could store:
bool _startsOrEndsUpSlursandbool _startsOrEndsDownSlurs.
I was curious to try this, but I've found that in fact the performance cost of using the vectors is literally not measureble. It even seems slightly faster with the vector implementation (probably because stuff gets optimized when they're empty? which means most of the time) . Also, I can think of several situations (and I've encountered a few already) where it would be quite convenient to have an easy access from the chord to a slur that starts or ends on it, so I'd like to keep the vectors.
In terms of performance, this current implementation at costs a 7.5% increase in the total layout time (tested on a huge file with hundreds of slurs though) and nearly all of it is spent in Note::doComputeKerningType. That's too much I think @vpereverzev. I'm gonna try to better optimize this.
Yeah, the cost of constructing/filling those vectors may well be low, but then you need to iterate over them in Note::doComputeKerningType for every note of the chord. By computing the boolean values once for the chord, you can trivially retrieve them in Note::doComputeKerningType.
I've optimized it further and made sure that the computation is performed only once per chord. The measured performance cost (i.e. the time taken to perform Score::doLayoutRange()) is now +5.5% on a huge piano concerto, +5.7% on the Start Wars, +3.6% on a huge file but with not so many articulation and slurs.
By the way, when #13466 is merged, the performance cost of this PR will become much smaller, maybe even negligible. I'll wait for the other to be merged and the rebase and see.
@mike-spa please rebase it
@RomanPudashkin sorry, still working on it to reduce performance cost. Will be probably ready tomorrow.
@vpereverzev @RomanPudashkin I've eliminated the performance cost of this PR. I've also optimized some small things around it, and now it even seems to be some 1-2% faster than master.
Master:
PR:
If the vtest don't show any problem, this one is good to go for me