Implement partial & repeat ties #25745

miiizen · 2024-12-05T14:35:44Z

Resolves: #25426

This PR introduces partial ties to MuseScore, enabling ties to be connected across repeats. When adding a tie before a repeat item (repeat barlines, markers, jumps, and voltas) if any note on the other side of the repeat destination can be tied, a partial tie will be added.
When more than one endpoint is available, clicking the outgoing tie will open a menu of possible endpoints. These can be toggled on and off.

Screen.Recording.2024-12-05.at.14.33.29.mov

Playback implementation to follow.

Add separate style option for hanging ties

mike-spa · 2024-12-19T09:03:15Z

src/engraving/compat/midi/compatmidirender.cpp

@@ -1080,7 +1080,7 @@ std::set<size_t> CompatMidiRender::getNotesIndexesToRender(Chord* chord)
    }

    auto noteShouldBeRendered = [](Note* n) {
-        while (n->tieBack() && n != n->tieBack()->startNote()) {
+        while (n->tieBack() && !n->incomingPartialTie() && n != n->tieBack()->startNote()) {


I've seen this check repeated several times: every time you are looking for a tie-back-but-not-incoming-partial. Perhaps this is worth adding a tieBackNonPartial() method?

mike-spa · 2024-12-19T09:04:50Z

src/engraving/dom/edit.cpp

+static Tie* createAndAddTie(Note* startNote, Note* endNote)
+{
+    Score* score = startNote->score();
+    const bool createPartialTie = !endNote;


I actually find it more readable without this additional bool that's just the negation of another

mike-spa · 2024-12-19T09:10:20Z

src/engraving/dom/excerpt.cpp

        Tie* tie = toTie(originalNote->tieFor()->linkedClone());
        tie->setScore(score);
        newNote->setTieFor(tie);
        tie->setStartNote(newNote);
        tie->setTrack(newNote->track());
        tieMap.add(originalNote->tieFor(), tie);
    }
-    if (originalNote->tieBack()) {
+    if (originalNote->tieBack() && originalNote->tieBack()->type() == ElementType::TIE) {


Same as above (kind of). Maybe we could modify tieBack and tieFor to return only "real" ties, and not LV and/or partial ties?

I considered this, but it seemed to me that there were far more scenarios where we want tieFor and tieBack to return LVs & partial ties than when we didn't.

mike-spa · 2024-12-19T09:29:19Z

src/engraving/dom/measure.cpp

+
+    Segment* seg = first(SegmentType::ChordRest);
+    while (seg) {
+        for (track_idx_t track = startTrack; track < ntracks; track++) {


Since you're checking all the available tracks, I think you can more compactly do
for (EngravingItem* item : seg->elist()) {}

mike-spa · 2024-12-19T09:59:02Z

src/engraving/dom/note.cpp

        tieFor()->endNote()->setTieBack(tieFor());
    }
 }

+bool Note::followingJumpItem()


From the name of this function I expected it to return an EngravingItem. You could either rename it into hasFollowingJumpItem, or it seems like it could also be useful to return the items themselves rather than bools?

mike-spa · 2024-12-19T10:12:50Z

src/engraving/dom/note.cpp

+    return false;
+}
+
+String Note::precedingJumpItemName()


I think this function really should not be here. At a first glance, I thought this was the symmetric of followingJumpItem but it looks like it has a completely different use and the fact that it returns a String left me quite confused.

From what I understand, the string returned by this function has a UI-related purpose which is exclusively referred to partial ties, so it definitely should be in the PartialTie class (perhaps as a static method)

mike-spa · 2024-12-19T10:28:05Z

src/engraving/dom/tie.h

+    void toggleEndPoint(const String& id);
+
+    void addTie(TieEndPoint* endPoint);
+    void removeTie(TieEndPoint* endPoint);


The fact that there is a public add method but also an addTie is a bit odd. Someone reading the public interface of your class wouldn't know why they are different and which one they should use. Could there be a more descriptive name?

mike-spa · 2024-12-19T10:30:50Z

src/engraving/dom/tie.cpp

+{
+    Note* note = endPoint->note();
+    Score* score = note ? note->score() : nullptr;
+    Tie* _startTie = startTie();


mike-spa · 2024-12-19T11:04:25Z

src/engraving/dom/tie.cpp

+        if (std::find_if((*tieEndPoints()).begin(), (*tieEndPoints()).end(), findEndTie) != (*tieEndPoints()).end()) {
+            continue;
+        }
+        score()->undoRemoveElement(tie);


A method of the Tie class should not be removing other ties from the Score, it could be a bit dangerous. Also, from a function named collectPossibleEndPoints I'd probably expect something which returns me a list of TieEndPoint*, not something which actively modifies the current tie and even the score. Here I would try to do a combination of (not necessarily all of them)

Giving this method a more descriptive name

Moving out the code which removes other ties into a separate function

Given that this method performs an undoable action on the score, it looks more like the kind of methods we usually put in edit.cpp, that's also an option

Yes, this code to remove ties is absolutely in the wrong place - it doesn't even cover all the situations I was hoping.
I think we need to remove stale endpoints when the repeat structure is recalculated - either inside or just after RepeatList::unwind.

mike-spa · 2024-12-19T11:09:15Z

src/engraving/dom/tie.h

@@ -25,6 +25,64 @@
 #include "slurtie.h"

 namespace mu::engraving {
+class TieEndPointList;
+class TieEndPoint


Side question: would TieJumpPoint be perhaps a better name for this class? I know what you mean by TieEndPoint, but it feels to me like it doesn't communicate the main purpose. Also because we often call "end point" the graphical end points too.

mike-spa · 2024-12-19T11:18:51Z

src/engraving/dom/note.cpp

+    case ElementType::PARTIAL_TIE: {
+        PartialTie* pt = toPartialTie(e);
+        pt->setTick(tick());
+        if (pt->partialSpannerDirection() == PartialSpannerDirection::OUTGOING) {


pt->itOutGoing() ?

Remove unneeded endEdit

aabd837

miiizen requested review from bkunda and oktophonie December 5, 2024 14:36

miiizen added 2 commits December 5, 2024 14:45

Implement partial ties

e1f5602

Add tests

05d09b7

miiizen force-pushed the partial-ties branch from 34d3c6c to 05d09b7 Compare December 5, 2024 14:45

oktophonie mentioned this pull request Dec 6, 2024

[TIED NOTES] One note is missing in repeats right after adding/deleting tie (tie between repeat barline) #25748

Open

4 tasks

miiizen added 7 commits December 6, 2024 16:20

Review fixes

da7d094

Fix inconsistencies with ties before segnos

7cb4a9d

Enforce min hanging tie distance at end of systems

1f54187

Add separate style option for hanging ties

Fix horizontal spacing for partial ties

48ab1f6

Note pad interaction fix

28e6ad9

Disallow ties between 2 different voltas

82ede19

Fix partial tie focus issues

8b6d621

oktophonie approved these changes Dec 13, 2024

View reviewed changes

Cover more tie clean up cases

f2976ac

miiizen mentioned this pull request Dec 18, 2024

Implement partial slurs #25883

Open

mike-spa reviewed Dec 19, 2024

View reviewed changes

miiizen added 2 commits December 19, 2024 16:02

Review fixes

ad95d2c

Remove partial ties on repeat structure change

3c25b0a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement partial & repeat ties #25745

Implement partial & repeat ties #25745

miiizen commented Dec 5, 2024

mike-spa Dec 19, 2024

mike-spa Dec 19, 2024

mike-spa Dec 19, 2024

miiizen Dec 19, 2024

mike-spa Dec 19, 2024

mike-spa Dec 19, 2024

mike-spa Dec 19, 2024

mike-spa Dec 19, 2024

mike-spa Dec 19, 2024

mike-spa Dec 19, 2024

miiizen Dec 19, 2024

mike-spa Dec 19, 2024

mike-spa Dec 19, 2024

Implement partial & repeat ties #25745

Are you sure you want to change the base?

Implement partial & repeat ties #25745

Conversation

miiizen commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment