Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix #285535, fix #284613: spacing of mid-measure clefs #5020

Merged
merged 1 commit into from May 21, 2019

Conversation

MarcSabatella
Copy link
Contributor

@MarcSabatella MarcSabatella commented May 15, 2019

See https://musescore.org/en/node/285535

The main issue here was a seemingly innocuous change to the alignment of clefs within their segment for the sake of palette display, but it ended up breaking mid-measure clef layout for scores with multiple staves. That's because previously clefs had negative positions and hence could overlap notes on other staves just as accidentals can - the segment shapes don't collide. But after that change, we ended up with bad spacing because layout would not allow the clefs to overlap the notes on other staves:

image

Fixing this in clef.cpp, however, uncovered another problem, where clefs would not longer tuck in over notes on the same staff:

image

versus the expected (in 3.0.5, anyhow, and ignore the collision with the beam in both cases, which is unrelated):

image

It's not as big a deal - and it was essentially the same in 2.3.2 - but since the whole "shape" scheme was supposed to allow for this, it did seem worth addressing. It turns out to have an interesting solution - Segment::minHorizontalDistance needs to be allowed to return negative values. It fixes this case, and I'm hoping it might turn out to also allow tighter spacing in other similar cases. So far I don't see any regressions from this, but more testing it definitely in order. [EDIT: see below, breaths needed to not return negative values, fixed now]

Here is how the original example now looks (more or less same as 2.3.2):

image

@MarcSabatella
Copy link
Contributor Author

Oh, also - we were using barline-to-note distance for the space after the mid0measure clef. And we had a style setting clef/key right margin we weren't using. Because people have have set that style to randomly bad values trying to figure out what it does, I introduced a new style setting and hooked it up to the same control in the style dialog, thus fixing https://musescore.org/en/node/284613

@MarcSabatella
Copy link
Contributor Author

MarcSabatella commented May 15, 2019

After more testing - including looking over the vests - I realize breaths shouldn't be allowed to "tuck in" so close, but my change made that happen. So I added a fix for that. BTW, there were already vtests that covered the clef case, they were "failing" before but work as expected now.

@MarcSabatella
Copy link
Contributor Author

A regression is identified in the issue report, investigating...

@MarcSabatella
Copy link
Contributor Author

Fixed the tie regression, and tweaked things to hopefully reduce chances of other regressions too - no longer messing with the setting of the return value of Segment::minHorizontalDistance() except for clefs. Although, I misspoke when I said I was changing the function to actually return a negative value, really it is only internally I am using negative values. There is still a floor of 0 on the return value.

@MarcSabatella
Copy link
Contributor Author

MarcSabatella commented May 20, 2019

I should mention that this was done at special request of S. Christian Collins, who listed it as the number one issue holding him back from moving to 3.0 himself. It is indeed a significant layout regression, especially bothersome since one of the original features implemented for 3.0 some years ago was supposed to be smarter/tighter spacing.

@anatoly-os anatoly-os merged commit 624b90d into musescore:master May 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants