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 cross staff tie layout #21192

Merged
merged 1 commit into from
Feb 11, 2024
Merged

Conversation

miiizen
Copy link
Contributor

@miiizen miiizen commented Jan 24, 2024

Resolves: #20747

Fix layout of cross staff ties on the same system and between systems.
Screenshot 2024-01-24 at 17 42 24

@miiizen miiizen force-pushed the 20747-cross-ties branch 3 times, most recently from 3863949 to 6afac52 Compare January 25, 2024 08:53
@oktophonie oktophonie added the vtests This PR produces approved changes to vtest results label Jan 25, 2024
Copy link
Contributor

@mike-spa mike-spa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job!


return (startN && startN->chord()->staffMove() != 0) || (endN && endN->chord()->staffMove() != 0);
return (startChord && (startChord->staffMove() != 0 || startChord->vStaffIdx() != staff))
|| (endChord && (endChord->staffMove() != 0 || endChord->vStaffIdx() != staff));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain why we need to add the chord->vstaffIdx() != staff condition? At a first glance it looks redundant to me, cause it can only be true if at least one chord has staffMove() != 0. Or is there a case that I'm not thinking of?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed when the tie starts on a note on staff 0 (the tie's staff is 0) and ends on a note on staff 1. In this case neither chord is currently moved but the tie should still be cross staff. (eg. the first case in the attached screenshot)

@@ -1556,6 +1558,10 @@ TieSegment* SlurTieLayout::tieLayoutBack(Tie* item, System* system, LayoutContex
segment->setSystem(system);
segment->resetAdjustmentOffset();

if (chord) {
segment->setStaffIdx(chord->vStaffIdx());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels slightly hacky (and slightly dangerous) cause you are changing the staffIdx of only one of the TieSegments, so you end up with mismatching tracks between the Tie item and its two TieSegments. Anyway, this is such an edge case that, if it works properly, I think we can keep the hack, for once, as it does make it all quite simple. Maybe do write a small hack-warning comment to explain that though :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah, having a vStaffIdx field for tie segments would absolutely be safer - I can try to make that work

@oktophonie oktophonie requested review from cbjeukendrup and removed request for RomanPudashkin February 9, 2024 16:44
@cbjeukendrup cbjeukendrup merged commit c63f04e into musescore:master Feb 11, 2024
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vtests This PR produces approved changes to vtest results
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crossstaff ties create a small unwanted tie elsewhere on the page or point in the wrong direction
4 participants