Fix #293785: Fix #302590: Fix #294890: Chord symbols jump when user goes into edit…

[njvdberg]

… mode

Resolves: https://musescore.org/en/node/302590
Resolves: https://musescore.org/en/node/293785
Resolves: https://musescore.org/en/node/294890

When looking into #293785 it was found that placement of Harmonies above a <code<FretDiagram was not consistent.
o) Align::HCENTER aligns the horizontal centers of bot Harmony and FretDiagram.
o) Align::RIGHT aligns the right edge of the Harmony roughly to the centre of the <code<FretDiagram.
o) Align::LEFT aligns the left edge of the Harmony to the left edge of the FretDiagram.

So every alignment uses it own reference. This has been corrected first.

The root cause of #302590 was the difference of bounding box between NORMAL and EDIT mode. And this was caused because of the different formatting between NORMAL and EDIT. In EDIT mode the horizontal alignment was using both different algorithm and reference. Also the height of the bounding box where different in EDIT and NORMAL mode
To solve this the bounding box while in EDIT mode

1. is aligned exactly are in NORMAL mode
2. has the same height in both EDIT and NORMAL mode.

• I signed CLA
• I made sure the code in the PR follows the coding rules
• I made sure the code compiles on my machine
• I made sure there are no unnecessary changes in the code
• I made sure the title of the PR reflects the core meaning of the issue you are solving
• I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
• I made sure the commit message title starts with "fix #424242:" if there is a related issue
• I created the test (mtest, vtest, script test) to verify the changes I made

[njvdberg]

It was reported this PR also solves #294890, added to comments.

[AntonioBL]

This PR changes the vtests, please have a look here: https://github.com/musescore/MuseScore/actions/runs/65207454
They seem a lot of minor changes, except for these vtests:
harmony-2
harmony-13
harmony-14
where the positioning of some chord symbols is shifted.
[Please note that maybe the previous behavior was not correct and this PR could have corrected the relative positioning according to the parameters saved in those files.]

[Jojo-Schmitz]

harmony-2 indeed seems to be a real issue, esp. for that D7 chord symbol:

harmony-13 esp. for the C7 (not as bad as the D7 above though):

for harmony-14 it is the increased staff distance that seems to be an issue

[njvdberg]

Thanks, I will have a look.
And just for my curiosity, how could I catch these issues myself?

[AntonioBL]

If you update the PR and force push, you can see the results of vtests actions here:
https://github.com/musescore/MuseScore/actions?query=workflow%3ACI_vtests
If you want to check before making a PR to MuseScore master, the simplest way is to push to your git repository and make a PR targeting your master branch: the Github action is triggered and you can see the results in the Actions tab in your Github repository web page.
You could also run vtests locally, but the scripts in the repository require a Linux installation, see:
https://github.com/musescore/MuseScore/blob/master/vtest/gen
https://github.com/musescore/MuseScore/blob/master/vtest/gen_compare
and the script used by the Github action:
https://github.com/musescore/MuseScore/blob/master/.github/workflows/vtests.yml

[Jojo-Schmitz]

Thanks, I will have a look.
And just for my curiosity, how could I catch these issues myself?

I believe you even get a notification about those failed vtests. Not sure why not in this case though?

[njvdberg]

I believe you even get a notification about those failed vtests

That's what I was hoping for ;-). But the hints of Antonio should make it possible for me to run these test locally (I only have a Linux installation :-))

[njvdberg]

I had a quick look at the differences. harmony-14 detects a real problem, the shapes of the Harmony are too low. Also, when creating the a Harmony this edit bounding box is at a wrong location. This might be related with #5889 from @SKefalidis.
When looking at D7 in harmony-2, I see a 3.4 x-shift and when I calculate the position by hand, the current position is now indeed nicely centered around the reference while in the 3.4 it wasn't. This shift looks correct unless I understood the alignment wrong. It seems the alignment for harmonies is different as for e.g. staff texts (in 3.4):

With this PR now the alignment of both harmonies and texts are the same:

If this isn't correct, the alignment of harmonies can easily changed to what it used to be (but in consistent with alignment of e.g. texts).

[Jojo-Schmitz]

the 3rd ones are to far to the left, they should end with the stem or the right edge of the notehead, but surely not to its left edge, just like the 1st ones are not aligning to the right edge, but to the left.
the 2nd ones should center align to the notehead center IMHO.
That D7 does look left aligned (to the stem or right edge), but indeed may well be center aligned to the left edge. This IMHO is wrong, see above, it should be center aligned to the noted head center.

[njvdberg]

The 3rd ones are too far to the left indeed because they were, like the texts, aligned to the fixed reference point, being the left edge of note head. Now I changed a alignment of the harmonies:
Center: Centre of the harmony is aligned with the centre of the note head
Right: Right edge of the harmony is aligned with the right edge of the note head.
Left: Left edge of the harmony is aligned with the left edge of the note head.
Please note this is different as texts which use a fixed reference, being the left edge of the note head!
With this change most of the vtest differences are solved. However there are still shifts. The reason of these shift is the original calculations of the centre alignment wasn't correct. Especially the shift of the last harmony (C7sus4) was quite large. Since the placement of the harmonies is slightly different, the position of the notes also changed.

[Jojo-Schmitz]

Should be fine then.
I wonder though whether other texts (esp. Lyrics) should follow the same rules?

[AntonioBL]

@njvdberg : so the new behavior is the one in the upper figure, right? I mean this figure. Because in the other one the reference for both text and chord symbol is the left edge of the note head.
(I can't directly check at the moment 😅 )

[MarcSabatella]

I can't say that the exact position of centering a chord symbol over a note is crucially important to me, because normally chord symbols are left-aligned anyhow. The current proposed alignment seems good. What's more important is to check the alignment in the presence of fretboard diagrams, because that's where centering matters. They need to center to the same reference point as the diagram, whatever that is.

[njvdberg]

I agree the alignment of a harmony over a note isn't that crucial since the note head is pretty small. But a defined position is crucial because we have both a Harmony and a TextBase, depending whether in EDIT or NORMAL mode, which should placed on top of each other to prevent the jumping.
In the current situation the position wasn't that clear at all, different location were mixed up. Therefor I have to come with a clear definition to make sure both objects can be placed on top of each other. So far alignment in MuseScore is at least confusing since alignment definition is different as I'm used to and also not consistent.
The proposal is to align the harmonies above notes like this:

Alignment of the harmonies above fretboard diagrams will be like:

[Jojo-Schmitz]

Fine by me

[MarcSabatella]

Looks great!

[njvdberg]

I finally managed to to get all cases right (horizontal alignment, vertical alignment, above/below staff, normal/edit mode).
vtest (@AntonioBL , thanks for pointing me to this!) still shows some difference. Most of them are minor. All these changes can be explained by the (slightly) changed reference of the harmonies.
harmony-13 looks still very bad but, since this example is very "dense" the position of the notes (and therefor also note at the right) is effected by it. In less dense examples, e.g. harmony-11, this effect is much less.

[Jojo-Schmitz]

Harmony-13 before

after:

diff:

IMHO after looks better

[AntonioBL]

In Harmony-13 with this PR the horizontal spacing in the second measure takes into account the presence of the chord symbol on the first note. I prefer this with respect to the original behavior: I think it becomes more readable. 👍

As I said, a change in vtests is not necessarily a change to the worse.
Vtests have been somehow "neglected" for some time, so probably some of them (or their results) are not optimal.

[MarcSabatella]

As I said, I don't really care too much how the alignment works out for centered chords with no fret diagram, but I like the idea of centering over notehead. It's not super immediately obvious to me there is any noticeable difference in the before/after on harmony-13, though. When I look at the diff, it actually seems most of it is about the spacing of the notes, not the position of the chord symbols relative to the notes. So I guess that is what I'd want to be sure I understood.

My first thought was that this is due to a change in the bbox/shape calculation, but now I think maybe it's something rather different, and directly related to the actual purpose of the PR. If I load the file into a current master, there is a layout shift on the next relayout, akin to the ones we see with fingerings. Without actually building this PR, my guess is that the changes have eliminated that layout shift, and thus the vtest is now seeing the "true" layout. In which case, it's a great change all around.

If the layout shift is not fixed by this PR, then that is something to still look at I think, as it probably involves some of the same code.

[MarcSabatella]

And actually, no that I read @AntonioBL 's last comment more carefully, I think he has nailed the source of the diff. This is exactly the change that happens in current master after the initial layout - the first note of the second measure moves to the right a bit in order to enforce that space to the first note with the chord symbol included in the shape.

Personally, I'm not convinced that's better than not counting the chord symbol, but it's definitely better to not shift after first layout, an since this is only going to be an issue with centered chords, again, I really don't care one way or the other. Not shifting on relayout is a win.

[njvdberg]

A side effect of this PR is that a relayout will not cause any shift. In the existing code, a relayout will casue a shift:

This relayout causes a serious shift.

If we now compare the result of this PR with the result of current master after a relayout the differences are not that alarming anymore:

Now we see a clear shift of the harmonies which is as expected because the origin is changed (a halve note width). Also the spread of the notes is slightly more evenly but this is more or less the same as in the original version without a relayout.

[anatoly-os]

@AntonioBL do we need to update the vtest refs in the PRs that change the vtests results? Do the CI_vtests job compare vtest results with master or the ref files?

[AntonioBL]

@anatoly-os : sorry, I had not seen the notification.
It is not necessary to update the vtest ref for the CI vtests: they build both the ref branch and the PR on the fly and compare their vtest results.
Comparison against the ref png is done only in http://vtest.musescore.org/index.html (but most of the vtests already have outdated ref files).