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

Cross-staff beam regressions #5046

Merged
merged 2 commits into from May 22, 2019
Merged

Conversation

@MarcSabatella
Copy link
Contributor

MarcSabatella commented May 22, 2019

See https://musescore.org/en/node/289492 and https://musescore.org/en/node/289498. The first issue contains a fairly thorough description of the underlying issue: bad layout resulting from the fact that with cross-staff beams, we make an initial guess as to stem direction, do some layout based on that, then correct the stem direction guess much later after it's too late to fix the layout based on the original guess.

We've fought some version of this for years and tried to address it by improving our guess, or being smarter about trying to correct things after realizing we guessed wrong. for this PR, I took a simpler approach I think is both more likely to succeed and less likely to cause regressions elsewhere. Instead of acting on the guess, instead I recognize that all we have is a guess, so I explicitly don't act on the guess, but only on things I know won't change during the course of the layout.

In the case of slurs, we were using stem direction to layout to either the notehead or the stem dpeneding on stem direction. I've changed this so if the note is on a cross-staff beam, we always layout to the notehead.

In the case of beams, the issue was actually not so much about the beam layout as the layout of potentially overlapping chords (multiple voices on same segment), where changes of opinion about stem direction changes the horizontal offset of the chords. So, again, in cases where a chord is on a cross-staff beam, I'm changing the algorithm to note base the offset on the guessed stem direction, but on things like, was it moved to another staff, what voice is it in, etc. Some of the same things used in Chord::computeUp, but here I consider only the factors specifically relevant to cross-staff beams.

These changes should only affect cross-staff beams, so they are safe in that sense, but will no tdoubt mean some differences in layout compared to 3.0.5 and/or master without these changes. There may be cases where the guess was actually right and produced the more correct result than my new code, but with my code we should actually produce good layout either way, no weird glitches liek the ones in the issues here.

I plan to inspect the vtests before going to bed, but need to deal with a couple of other last minute regressions first.

@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

MarcSabatella commented May 22, 2019

vtest cross-2 shows a regression caused by this change, I see a fix, will try it, then going bed.

Never mind, it's unlikely to work right off the bat, need more time, will deal with this later.

The slur commit should be good, but the beam one probably causes as many problems as it solves.

@MarcSabatella MarcSabatella force-pushed the MarcSabatella:cross-regressions branch from 7b954f7 to f2073af May 22, 2019
@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

MarcSabatella commented May 22, 2019

Simple and safer solution found for the beam issue: if a cross-staff beam is found, just treat it the same way we do if actual cross-staff notes are found: don't offset the chords at all. There's no way to do so reliably anyhow. In cases where the notes on the cross cross-staff beam don't conflict with other notes, there is no change, all is well:

image

In cases where there is conflict, user will just have to resolve it manually:

image

This is the same result as we have always produced (going back to MuseScore 1) if there were no cross-staff beam but instead just the note itself were cross-staff:

image

Obviously not ideal, but at least the user can fix this through manual offsets, whereas the messed up beam cannot. And we've lived with this collision for years already: https://musescore.org/en/node/114141

I think this is the best we can do for now, and really the foreseeable future.

@anatoly-os

This comment has been minimized.

Copy link
Contributor

anatoly-os commented May 22, 2019

Vtests are ok, right?

@anatoly-os anatoly-os merged commit ff184e1 into musescore:master May 22, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

MarcSabatella commented May 22, 2019

Yes, iccluding the one that clued me in that my original approach on the beams was not workable - you can't separate out choices about offsetting chords from choices about notehead placement within each chord (which has to be correct for actual final stem direction).

I should add another vtest then...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.