Introducing new options for ties, and a general overhaul of tie layout #19127

mike-spa · 2023-08-22T11:09:59Z

Resolves: #18685

This PR implements the feature requested in #18685. In order to do that, a deep overhaul of the tie layout code became necessary, as the existing code was too complex and inadequate to achieve this.

Here's a (probably incomplete) summary of the changes.

Endpoints position is (slightly) changed and, if available, approximates the optical center of noteheads using smufl cutouts.
Default curvature of ties (especially short and short-medium) is increased.
The logic for vertical adjustment, which checks for bad endpoint and arc intersections with staff lines, is improved and avoids a few undesirable situations we had before.
New style setting for inside/outside style (which defaults to "outside" as requested), as well as individual property override.
Whenever possible (and if short enough) ties will fit within a space without intersecting staff lines.
Very short ties are thinned.
The annoying glitch when drag-editing ties has been fixed.
New logic to deal with ledger lines.

src/engraving/libmscore/property.cpp

src/engraving/style/styledef.cpp

cbjeukendrup

Here's a first round of comments, mainly about the 'supporting code'. Now I'm going to look into the new layout code.

cbjeukendrup · 2023-09-21T14:11:21Z

src/engraving/dom/tie.cpp

@@ -312,6 +308,10 @@ void TieSegment::computeBezier(PointF shoulderOffset)

 void TieSegment::adjustY(const PointF& p1, const PointF& p2)
 {
+ /*****************************************
+ * DEPRECATED


Am I right that these deprecated methods are only preserved for use in the stable layout code? In that case, it might be desirable to remove them here and move them as static functions to stable.

That's quite a time consuming refactoring step, and I don't think it's worth doing it now for a method that is going to be deleted anyway. There were a few things (and there still is, to less extent) in Tie layout that were not moved to the dedicated layout class. Once we replace dev layout into stable, I've made a note to myself to go back and delete these methods that aren't used anymore.

src/engraving/types/typesconv.cpp

cbjeukendrup · 2023-09-21T14:22:49Z

src/engraving/types/typesconv.h

+ static String toXml(TiePlacement interval);
+ static TiePlacement fromXml(const String& str, TiePlacement def);


It looks like AsciiStringView is usually used for this kind of thing instead of String

cbjeukendrup · 2023-09-21T14:25:51Z

src/inspector/types/directiontypes.h

@@ -42,8 +42,15 @@ class DirectionTypes
 HORIZONTAL_RIGHT
 };

+ enum TiePlacement {


I'd say DirectionTypes is not the best place for this. I'd create a new SlurTieTypes class/file instead.

cbjeukendrup · 2023-09-21T14:27:47Z

src/inspector/view/qml/MuseScore/Inspector/notation/lines/SlurAndTieSettings.qml

@@ -70,4 +70,19 @@ Column {
 { text: qsTrc("inspector", "Below"), value: DirectionTypes.VERTICAL_DOWN }
 ]
 }
+
+ PlacementSection {


There is not much benefit in using PlacementSection here, since all its properties are overridden; better use FlatRadioButtonGroupPropertyView directly.

src/engraving/rendering/dev/chordlayout.cpp

src/engraving/dom/tie.h

cbjeukendrup · 2023-09-21T17:35:50Z

src/engraving/dom/tie.cpp

+ Note* startN = startNote();
+ Note* endN = endNote();
+
+ return (startN && startN->chord()->staffMove() != 0) || (endN && endN->chord()->staffMove() != 0);


Is this really the desired formula, or should it be

return startN && endN && startN->chord()->vStaffIdx() != endN->chord()->vStaffIdx();

or, like Slur::isCrossStaff

return startCR() && endCR() && (startCR()->staffMove() != 0 || endCR()->staffMove() != 0 || startCR()->vStaffIdx() != endCR()->vStaffIdx());

Yes, it's correct as it is (we consider it "cross" even if both endpoints are moved to the same staff). Actually, I think the last OR condition in Slur::isCrossStaff is redundant (it can't be true if at least one of the first two isn't) but I'll verify that another time

src/engraving/rendering/dev/slurtielayout.cpp

src/engraving/dom/tie.h

src/engraving/rendering/dev/slurtielayout.cpp

cbjeukendrup · 2023-09-21T22:52:45Z

Let's not worry about those unit tests; I'll just review #18722 asap so that we can merge that first and then we simply rebase this one. (I guess that was your plan already :) )

mike-spa · 2023-09-22T11:57:17Z

@cbjeukendrup besides my comment replies, all done, thanks!

cbjeukendrup

I found a couple more things, but decided to fix them my self in order not to bother you again :)

src/engraving/types/typesconv.h

src/inspector/CMakeLists.txt

src/inspector/view/qml/MuseScore/Inspector/notation/lines/SlurAndTieSettings.qml

cbjeukendrup · 2023-09-26T02:10:39Z

Hm, it looks like the tests still need one more update. @mike-spa could you take care of that? (you don't need to wait for a new approval after that, since it only seems to involve test reference files)

Move out piece of code to separate function Introduce TiePlacement property and property type correction Introduce inspector section for tie placement Created placeholder style widget for tie inside/outside Update calculation of inside/outside according to new settings Remove responsibility for setting pos out of adjustY Rename SlurPos to SlurTiePos Move computation of start and end system out of tie pos correction Temporarily deactivate adjustX and adjustY Rationalize calculation of default start/end points Force horizontal correction Re-implementation of adjustX correction Rationalize shoulder height formula and increase default curvature Decrease thickness for short ties Extract thickness calculation to dedicated method initial reimplementation of adjustY Implementing arc vs staffLine avoidance correction Better management of short ties more Don't do adjustments if autoplace is off Correct dragging behaviour cleanup Fix glitches when when dragging ties correction improvements Adjust for ledger lines corrected values Fix tie-to-tie collisions Tweak curve correction correction correction margins above and below correction move some constants to style correction Final corrections

Update template files

correction

Code review corrections - 2 Code review corrections - 3 Code review corrections - 4 Code review corrections - 5

mike-spa requested a review from oktophonie August 22, 2023 11:09

mike-spa changed the title ~~Cleanup ties~~ Introducing new options for ties, and a general overhaul of tie layout Aug 22, 2023

mike-spa force-pushed the cleanupTies branch 4 times, most recently from 21af4ee to 14076b4 Compare August 24, 2023 12:41

Jojo-Schmitz reviewed Aug 26, 2023

View reviewed changes

src/engraving/libmscore/property.cpp Outdated Show resolved Hide resolved

Jojo-Schmitz reviewed Aug 26, 2023

View reviewed changes

src/engraving/style/styledef.cpp Outdated Show resolved Hide resolved

mike-spa force-pushed the cleanupTies branch 3 times, most recently from ab0d6bc to dfa016c Compare August 29, 2023 15:56

cbjeukendrup force-pushed the cleanupTies branch from dfa016c to 1d2f134 Compare September 19, 2023 10:03

cbjeukendrup reviewed Sep 21, 2023

View reviewed changes

mike-spa force-pushed the cleanupTies branch from 1d2f134 to 40191d2 Compare September 21, 2023 15:20

cbjeukendrup reviewed Sep 21, 2023

View reviewed changes

src/engraving/dom/tie.h Outdated Show resolved Hide resolved

cbjeukendrup reviewed Sep 21, 2023

View reviewed changes

mike-spa force-pushed the cleanupTies branch from 4eeb87b to 8e619fb Compare September 22, 2023 11:53

cbjeukendrup force-pushed the cleanupTies branch from 8e619fb to 16499d9 Compare September 22, 2023 14:57

cbjeukendrup approved these changes Sep 22, 2023

View reviewed changes

src/engraving/types/typesconv.h Outdated Show resolved Hide resolved

src/inspector/CMakeLists.txt Outdated Show resolved Hide resolved

src/inspector/view/qml/MuseScore/Inspector/notation/lines/SlurAndTieSettings.qml Outdated Show resolved Hide resolved

cbjeukendrup force-pushed the cleanupTies branch from 16499d9 to a62d6f9 Compare September 26, 2023 01:30

cbjeukendrup mentioned this pull request Sep 26, 2023

New score and parts interactions #18722

Merged

mike-spa added 2 commits September 26, 2023 09:13

Increase version number to 4.2

41819b6

Update template files

mike-spa force-pushed the cleanupTies branch from a62d6f9 to 06ca4f2 Compare September 26, 2023 07:14

mike-spa added 2 commits September 26, 2023 11:01

Introduce style compatibility for old scores

f8f8835

added vtests

b2a40d3

correction

Code review corrections

3dfae78

Code review corrections - 2 Code review corrections - 3 Code review corrections - 4 Code review corrections - 5

mike-spa force-pushed the cleanupTies branch from 06ca4f2 to 3dfae78 Compare September 26, 2023 09:08

mike-spa merged commit 6f711ca into musescore:master Sep 26, 2023
10 of 11 checks passed

oktophonie mentioned this pull request Nov 28, 2023

Ties misplaced on slash noteheads #20227

Closed

mike-spa mentioned this pull request Dec 14, 2023

Fix disappearing ties when removing line break #20486

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introducing new options for ties, and a general overhaul of tie layout #19127

Introducing new options for ties, and a general overhaul of tie layout #19127

mike-spa commented Aug 22, 2023 •

edited

Loading

cbjeukendrup left a comment

cbjeukendrup Sep 21, 2023

mike-spa Sep 22, 2023

cbjeukendrup Sep 21, 2023

cbjeukendrup Sep 21, 2023

cbjeukendrup Sep 21, 2023

cbjeukendrup Sep 21, 2023

mike-spa Sep 22, 2023

cbjeukendrup commented Sep 21, 2023

mike-spa commented Sep 22, 2023

cbjeukendrup left a comment

cbjeukendrup commented Sep 26, 2023

		static String toXml(TiePlacement interval);
		static TiePlacement fromXml(const String& str, TiePlacement def);

Introducing new options for ties, and a general overhaul of tie layout #19127

Introducing new options for ties, and a general overhaul of tie layout #19127

Conversation

mike-spa commented Aug 22, 2023 • edited Loading

cbjeukendrup left a comment

Choose a reason for hiding this comment

cbjeukendrup Sep 21, 2023

Choose a reason for hiding this comment

mike-spa Sep 22, 2023

Choose a reason for hiding this comment

cbjeukendrup Sep 21, 2023

Choose a reason for hiding this comment

cbjeukendrup Sep 21, 2023

Choose a reason for hiding this comment

cbjeukendrup Sep 21, 2023

Choose a reason for hiding this comment

cbjeukendrup Sep 21, 2023

Choose a reason for hiding this comment

mike-spa Sep 22, 2023

Choose a reason for hiding this comment

cbjeukendrup commented Sep 21, 2023

mike-spa commented Sep 22, 2023

cbjeukendrup left a comment

Choose a reason for hiding this comment

cbjeukendrup commented Sep 26, 2023

mike-spa commented Aug 22, 2023 •

edited

Loading