Add laissez vibrer ties #25435

miiizen · 2024-11-05T15:19:56Z

Resolves: #16726
This PR adds laissez vibrer ties. They are added via a toggle in the toolbar.

Screen.Recording.2024-11-05.at.15.03.42.mov

Laissez vib ties are drawn as normal ties, with all the same style options plus a minimum length available in the properties panel.

Screen.Recording.2024-11-05.at.15.04.51.mov

When multiple LV ties are added to a chord, the 'minimum length' for whichever tie starts furthest to the right is honoured, and then the right endpoints of all the other ties are extended to match.

Screen.Recording.2024-11-05.at.15.07.21.mov

There is also the option to use SMuFL symbols instead of MuseScore's own ties. When added to a chord, these will all end at the rightmost point any tie reaches and will start at the same point as their length is not variable.

Properties:

Style settings:

This element is implemented as a child of the Tie class which makes it a Spanner without an end element. The relevant methods have been overriden to enforce this. The spanner will also only ever have a single segment.

In old scores, laissez vibrer articulations will be converted to the new ties.

rettinghaus

Looks pretty nice. Thank you so much. One comment on my side for the MusicXML export.

NB: the export to MEI also has to be adjusted. I can do that, if you like.

rettinghaus · 2024-11-06T09:40:22Z

src/importexport/musicxml/internal/musicxml/export/exportmusicxml.cpp

        const Tie* tieFor = note->tieFor();
-        if (tieFor && ExportMusicXml::canWrite(tieFor)) {
+        if (tieFor && !laissezVib && ExportMusicXml::canWrite(tieFor)) {


This way all styling of lv ties gets lost. Better to adjust the previous clause to just exchange type="start" with type="let-ring".

rettinghaus · 2024-11-06T19:31:07Z

Another thing that I noticed during testing: lv styles are not being saved, so direction, color and line styles are lost.

XiaoMigros · 2024-11-07T08:39:28Z

Should l.v. ties get a tie placement style option too?

oktophonie · 2024-11-07T08:44:08Z

@XiaoMigros They follow that of normal ties, which can also be overridden in Properties.

miiizen · 2024-11-08T10:10:35Z

@rettinghaus I'd be really grateful if you could take a look at the MEI side of things when this is merged!

bkunda · 2024-11-11T12:29:06Z

Really loving this PR @miiizen!

Just one nit-picky UI point:

We should use the label "Laissez vibrer" in these places, instead of "Laissez vib"

Add laisser vib class Add styles Add lv layout methods Create smufl symbol layout Refactor laisser vib to be a child of Tie Add MuseScore tie layout Create properties panel for lv ties Add style settings Add icons and input toolbar button Fix layout & file reading/writing

mike-spa

Outstanding work!

mike-spa · 2024-11-12T09:42:53Z

src/engraving/dom/laissezvib.cpp

+    initElementStyle(&laissezVibStyle);
+}
+
+mu::engraving::PropertyValue mu::engraving::LaissezVib::getProperty(Pid propertyId) const


mu::engraving shouldn't be necessary here, as you have the using namespace statement above. Actually, I think all of this should be done inside the engraving namespace . Meaning that instead of having the using namespace you'd just open namespace mu::engraving { ... } and put all your code inside it (that's how we do it for all the other items if I'm not mistaken).

mike-spa · 2024-11-12T09:46:26Z

src/engraving/dom/laissezvib.cpp

+    return up() ? SymId::articLaissezVibrerAbove : SymId::articLaissezVibrerBelow;
+}
+
+static int compareNotesPos(const Note* n1, const Note* n2)


This is a function that I can see being useful in many other contexts. In fact, I suspect that there may be something similar to this already, I can definitely remember having to use similar logic before, but I may be wrong. In any case, a better place for this could be in the utils file.

mike-spa · 2024-11-12T10:01:16Z

src/engraving/dom/laissezvib.cpp

+    }
+}
+
+void LaissezVib::calculateDirection()


Couple of thoughts (referred to all three of these functions, namely calculateDirection, calculateIsInside, isOuterTieOfChord).

I wonder if it would be worth trying to factor out some of the reused code here. I can see that the logic is quite similar to normal ties, with the obvious difference that we aren't looking at the end chord. The answer may well be no: sometimes avoiding repetitions at all costs can result in messier code. But do give it a thought, if you can see any easy way of doing it. A pattern that I've found useful sometimes when dealing with overrides like this is to separate the part that's common from the part that's overridden (see for instance Spanner::computeEndElement and Spanner::doComputeEndElement to see what I mean).

Independently from the previous point: perhaps calculateDirection, which is the more strictly layout-dependend one, is worth moving out of the DOM and into the layout module?

This and the point above became immediately obvious when beginning partial ties! I'll tidy these up and just have single methods in Tie which handle both start & end or only one or the other.

mike-spa · 2024-11-12T10:08:27Z

src/engraving/dom/spanner.h

@@ -220,31 +220,31 @@ class Spanner : public EngravingItem
    void setNoteSpan(Note* startNote, Note* endNote);

    EngravingItem* startElement() const { return m_startElement; }
-    EngravingItem* endElement() const { return m_endElement; }
+    virtual EngravingItem* endElement() const { return m_endElement; }


Is it necessary to override all of these methods? Does it become too annoying if you just ensure that m_endElement is always null for LV ties and let the other methods (which work with m_endElement) deal with it?

I thought this was pretty ugly, glad to get rid of it. It does work when only overriding setEndNote and setEndElement to which I've added a stronger assertion.

mike-spa · 2024-11-12T10:09:35Z

src/engraving/rendering/score/slurtielayout.cpp

@@ -1350,48 +1351,6 @@ void SlurTieLayout::avoidPreBendsOnTab(const Chord* sc, const Chord* ec, SlurTie
    }
 }

-TieSegment* SlurTieLayout::layoutTieWithNoEndNote(Tie* item)


THANK YOU, I couldn't wait to get rid of this horror

mike-spa · 2024-11-12T10:16:39Z

src/engraving/rendering/score/slurtielayout.h

@@ -19,12 +19,13 @@
 * You should have received a copy of the GNU General Public License
 * along with this program.  If not, see <https://www.gnu.org/licenses/>.
 */
-#ifndef MU_ENGRAVING_SLURTIELAYOUT_DEV_H
-#define MU_ENGRAVING_SLURTIELAYOUT_DEV_H
+#pragma once


Nothing bad about this, but since we use the #ifndef / #endif for every other header file in our repo, I'd rather keep it consistent.

This is changing; we're now gradually switching to #pragma once everywhere. New files should use #pragma once, and existing files could/should be updated when they are touched anyway.

miiizen force-pushed the lv-ties branch from e65b1c9 to 659c1de Compare November 5, 2024 16:25

rettinghaus suggested changes Nov 6, 2024

View reviewed changes

miiizen force-pushed the lv-ties branch 3 times, most recently from 80b7faa to 014794c Compare November 7, 2024 15:41

oktophonie requested a review from mike-spa November 8, 2024 09:08

miiizen force-pushed the lv-ties branch from 014794c to e9f6a84 Compare November 11, 2024 13:52

miiizen added 5 commits November 11, 2024 14:18

Add MusicXML laissez vib support

9bafca1

Convert lv articulation to new laissez vib ties

8adac79

Add tests

36f7f38

Fix horizontal spacing

567299a

miiizen force-pushed the lv-ties branch from e9f6a84 to 567299a Compare November 11, 2024 14:18

mike-spa reviewed Nov 12, 2024

View reviewed changes

Review changes

52ff468

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add laissez vibrer ties #25435

Add laissez vibrer ties #25435

miiizen commented Nov 5, 2024

rettinghaus left a comment •

edited

Loading

rettinghaus Nov 6, 2024

rettinghaus commented Nov 6, 2024

XiaoMigros commented Nov 7, 2024

oktophonie commented Nov 7, 2024

miiizen commented Nov 8, 2024

bkunda commented Nov 11, 2024 •

edited

Loading

mike-spa left a comment

mike-spa Nov 12, 2024

mike-spa Nov 12, 2024

mike-spa Nov 12, 2024

miiizen Nov 12, 2024

mike-spa Nov 12, 2024

miiizen Nov 12, 2024

mike-spa Nov 12, 2024

mike-spa Nov 12, 2024

cbjeukendrup Nov 12, 2024

Add laissez vibrer ties #25435

Are you sure you want to change the base?

Add laissez vibrer ties #25435

Conversation

miiizen commented Nov 5, 2024

rettinghaus left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rettinghaus commented Nov 6, 2024

XiaoMigros commented Nov 7, 2024

oktophonie commented Nov 7, 2024

miiizen commented Nov 8, 2024

bkunda commented Nov 11, 2024 • edited Loading

mike-spa left a 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

rettinghaus left a comment •

edited

Loading

bkunda commented Nov 11, 2024 •

edited

Loading