From 18fdbac7fdaf7561d6b28a9c1cd9284f61cbc886 Mon Sep 17 00:00:00 2001 From: Michele Spagnolo Date: Tue, 13 Aug 2024 13:42:46 +0200 Subject: [PATCH] Reimplement ledger lines Two things are bad about the current way we do ledger lines. a) They are implemented as a linked list, with the Chord holding the pointer to the first LedgerLine of the list and each LedgerLine holding a pointer to the next. This makes zero sense: it makes the list slower to traverse, it wastes memory, and it makes the code uglier for no reason. I've now reimplemented it with the Chord holding a vector of LedgerLines* instead. b) They are deleted and recreated at every single layout. This is a waste of resources, as we keep deleting and reallocating things in the heap for no reason, but most importanly has been the source of countless (meaning that I've literally lost count) memory bugs as anything that holds pointers to these LedgerLines (especially Shapes and now Skylines too) get invalidated. Now ledger lines are deleted / created only if necessary, i.e. only if the total number of ledger lines needed by the chord has changed. This should be, hopefully, the end of those memory bugs. --- src/engraving/dom/chord.cpp | 41 ++++++++++++------- src/engraving/dom/chord.h | 9 ++-- src/engraving/dom/ledgerline.cpp | 1 - src/engraving/dom/ledgerline.h | 3 -- src/engraving/dom/scoretree.cpp | 6 +-- .../rendering/dev/accidentalslayout.cpp | 4 +- src/engraving/rendering/dev/chordlayout.cpp | 33 ++++----------- src/engraving/rendering/dev/slurtielayout.cpp | 8 ++-- .../rendering/stable/chordlayout.cpp | 27 +++--------- .../rendering/stable/slurtielayout.cpp | 8 ++-- 10 files changed, 54 insertions(+), 86 deletions(-) diff --git a/src/engraving/dom/chord.cpp b/src/engraving/dom/chord.cpp index 37e517023afc2..7046e19764e16 100644 --- a/src/engraving/dom/chord.cpp +++ b/src/engraving/dom/chord.cpp @@ -260,7 +260,6 @@ std::vector Chord::noteDistances() const Chord::Chord(Segment* parent) : ChordRest(ElementType::CHORD, parent) { - m_ledgerLines = 0; m_stem = 0; m_hook = 0; m_stemDirection = DirectionV::AUTO; @@ -285,7 +284,6 @@ Chord::Chord(const Chord& c, bool link) if (link) { score()->undo(new Link(this, const_cast(&c))); } - m_ledgerLines = 0; for (Note* onote : c.m_notes) { Note* nnote = Factory::copyNote(*onote, link); @@ -426,11 +424,7 @@ Chord::~Chord() delete m_stemSlash; delete m_stem; delete m_hook; - for (LedgerLine* ll = m_ledgerLines; ll;) { - LedgerLine* llNext = ll->next(); - delete ll; - ll = llNext; - } + muse::DeleteAll(m_ledgerLines); muse::DeleteAll(m_graceNotes); muse::DeleteAll(m_notes); } @@ -918,7 +912,7 @@ double Chord::maxHeadWidth() const // addLedgerLines //--------------------------------------------------------- -void Chord::addLedgerLines() +void Chord::updateLedgerLines() { // initialize for palette track_idx_t track = 0; // the track lines belong to @@ -1055,20 +1049,20 @@ void Chord::addLedgerLines() if (minLine < 0 || maxLine > lineBelow) { double _spatium = spatium(); double stepDistance = lineDistance * 0.5; - for (auto lld : vecLines) { - LedgerLine* h = new LedgerLine(score()->dummy()); + resizeLedgerLinesTo(vecLines.size()); + for (size_t i = 0; i < vecLines.size(); ++i) { + LedgerLineData lld = vecLines[i]; + LedgerLine* h = m_ledgerLines[i]; h->setParent(this); h->setTrack(track); h->setVisible(lld.visible && staffVisible); h->setLen(lld.maxX - lld.minX); h->setPos(lld.minX, lld.line * _spatium * stepDistance); - h->setNext(m_ledgerLines); - m_ledgerLines = h; } } } - for (LedgerLine* ll = m_ledgerLines; ll; ll = ll->next()) { + for (LedgerLine* ll : m_ledgerLines) { renderer()->layoutItem(ll); } } @@ -1155,7 +1149,7 @@ void Chord::processSiblings(std::function func, bool inclu func(m_tremoloSingleChord); } if (includeTemporarySiblings) { - for (LedgerLine* ll = m_ledgerLines; ll; ll = ll->next()) { + for (LedgerLine* ll : m_ledgerLines) { func(ll); } } @@ -1552,6 +1546,23 @@ Chord* Chord::next() const return nullptr; } +void Chord::resizeLedgerLinesTo(size_t newSize) +{ + int ledgerLineCountDiff = newSize - m_ledgerLines.size(); + if (ledgerLineCountDiff > 0) { + for (int i = 0; i < ledgerLineCountDiff; ++i) { + m_ledgerLines.push_back(new LedgerLine(score()->dummy())); + } + } else { + for (int i = 0; i < std::abs(ledgerLineCountDiff); ++i) { + delete m_ledgerLines.back(); + m_ledgerLines.pop_back(); + } + } + + assert(m_ledgerLines.size() == newSize); +} + void Chord::setBeamExtension(double extension) { if (m_stem) { @@ -1812,7 +1823,7 @@ void Chord::scanElements(void* data, void (* func)(void*, EngravingItem*), bool } const Staff* st = staff(); if ((st && st->showLedgerLines(tick())) || !st) { // also for palette - for (LedgerLine* ll = m_ledgerLines; ll; ll = ll->next()) { + for (LedgerLine* ll : m_ledgerLines) { func(data, ll); } } diff --git a/src/engraving/dom/chord.h b/src/engraving/dom/chord.h index ec9d045da8e9e..176800fa3757d 100644 --- a/src/engraving/dom/chord.h +++ b/src/engraving/dom/chord.h @@ -127,10 +127,9 @@ class Chord final : public ChordRest bool isUiItem() const { return m_isUiItem; } void setIsUiItem(bool val) { m_isUiItem = val; } - LedgerLine* ledgerLines() { return m_ledgerLines; } - const LedgerLine* ledgerLines() const { return m_ledgerLines; } - void setLedgerLine(LedgerLine* l) { m_ledgerLines = l; } - void addLedgerLines(); + const std::vector& ledgerLines() const { return m_ledgerLines; } + void resizeLedgerLinesTo(size_t newSize); + void updateLedgerLines(); double defaultStemLength() const { return m_defaultStemLength; } void setDefaultStemLength(double l) { m_defaultStemLength = l; } @@ -352,7 +351,7 @@ class Chord final : public ChordRest void processSiblings(std::function func, bool includeTemporarySiblings) const; std::vector m_notes; // sorted to decreasing line step - LedgerLine* m_ledgerLines = nullptr; // single linked list + std::vector m_ledgerLines; Stem* m_stem = nullptr; Hook* m_hook = nullptr; diff --git a/src/engraving/dom/ledgerline.cpp b/src/engraving/dom/ledgerline.cpp index 006af92a1dce2..d732d6c1bd685 100644 --- a/src/engraving/dom/ledgerline.cpp +++ b/src/engraving/dom/ledgerline.cpp @@ -40,7 +40,6 @@ LedgerLine::LedgerLine(EngravingItem* s) { setSelectable(false); m_len = 0.; - m_next = 0; } LedgerLine::~LedgerLine() diff --git a/src/engraving/dom/ledgerline.h b/src/engraving/dom/ledgerline.h index 81910875a0c9f..1c469f51e22eb 100644 --- a/src/engraving/dom/ledgerline.h +++ b/src/engraving/dom/ledgerline.h @@ -59,8 +59,6 @@ class LedgerLine final : public EngravingItem bool vertical() const { return m_vertical; } double measureXPos() const; - LedgerLine* next() const { return m_next; } - void setNext(LedgerLine* l) { m_next = l; } void spatiumChanged(double /*oldValue*/, double /*newValue*/) override; @@ -72,7 +70,6 @@ class LedgerLine final : public EngravingItem private: double m_len = 0.0; - LedgerLine* m_next = nullptr; bool m_vertical = false; }; } // namespace mu::engraving diff --git a/src/engraving/dom/scoretree.cpp b/src/engraving/dom/scoretree.cpp index 70e0116470575..a80867f907003 100644 --- a/src/engraving/dom/scoretree.cpp +++ b/src/engraving/dom/scoretree.cpp @@ -403,10 +403,8 @@ EngravingObjectList Chord::scanChildren() const children.push_back(m_stemSlash); } - LedgerLine* ledgerLines = m_ledgerLines; - while (ledgerLines) { - children.push_back(ledgerLines); - ledgerLines = ledgerLines->next(); + for (LedgerLine* ledg : m_ledgerLines) { + children.push_back(ledg); } for (EngravingObject* child : ChordRest::scanChildren()) { diff --git a/src/engraving/rendering/dev/accidentalslayout.cpp b/src/engraving/rendering/dev/accidentalslayout.cpp index 5b302482f9f91..7722ae9bd23f1 100644 --- a/src/engraving/rendering/dev/accidentalslayout.cpp +++ b/src/engraving/rendering/dev/accidentalslayout.cpp @@ -261,10 +261,8 @@ void AccidentalsLayout::createChordsShape(AccidentalsLayoutContext& ctx) for (const Chord* chord : ctx.chords) { PointF chordPos = keepAccidentalsCloseToChord(chord) ? PointF() : chord->pos(); - const LedgerLine* ledger = chord->ledgerLines(); - while (ledger) { + for (const LedgerLine* ledger : chord->ledgerLines()) { ctx.chordsShape.add(ledger->shape().translate(ledger->pos() + chordPos)); - ledger = ledger->next(); } for (const Note* note : chord->notes()) { SymId noteSym = note->ldata()->cachedNoteheadSym; diff --git a/src/engraving/rendering/dev/chordlayout.cpp b/src/engraving/rendering/dev/chordlayout.cpp index ae8e438fe517c..c23e9e14c6831 100644 --- a/src/engraving/rendering/dev/chordlayout.cpp +++ b/src/engraving/rendering/dev/chordlayout.cpp @@ -343,12 +343,6 @@ void ChordLayout::layoutTablature(Chord* item, LayoutContext& ctx) layoutTablature(c, ctx); } - while (item->ledgerLines()) { - LedgerLine* l = item->ledgerLines()->next(); - delete item->ledgerLines(); - item->setLedgerLine(l); - } - double lll = 0.0; // space to leave at left of chord double rrr = 0.0; // space to leave at right of chord Note* upnote = item->upNote(); @@ -434,20 +428,16 @@ void ChordLayout::layoutTablature(Chord* item, LayoutContext& ctx) // create ledger lines, if required (in some historic styles) if (ledgerLines > 0) { -// there seems to be no need for widening 'ledger lines' beyond fret mark widths; more 'on the field' -// tests and usage will show if this depends on the metrics of the specific fonts used or not. -// double extraLen = style().styleS(Sid::ledgerLineLength).val() * _spatium; + item->resizeLedgerLinesTo(ledgerLines); double extraLen = 0; double llX = stemX - (headWidth + extraLen) * 0.5; for (int i = 0; i < ledgerLines; i++) { - LedgerLine* ldgLin = new LedgerLine(ctx.mutDom().dummyParent()); + LedgerLine* ldgLin = item->ledgerLines()[i]; ldgLin->setParent(item); ldgLin->setTrack(item->track()); ldgLin->setVisible(item->visible()); ldgLin->setLen(headWidth + extraLen); ldgLin->setPos(llX, llY); - ldgLin->setNext(item->ledgerLines()); - item->setLedgerLine(ldgLin); TLayout::layoutLedgerLine(ldgLin, ctx); llY += lineDist / ledgerLines; } @@ -2550,19 +2540,10 @@ void ChordLayout::layoutChords3(const std::vector& chords, void ChordLayout::layoutLedgerLines(const std::vector& chords) { - auto redoLedgerLines = [] (Chord* item) { - while (item->ledgerLines()) { - LedgerLine* l = item->ledgerLines()->next(); - delete item->ledgerLines(); - item->setLedgerLine(l); - } - item->addLedgerLines(); - }; - for (Chord* item : chords) { - redoLedgerLines(item); + item->updateLedgerLines(); for (Chord* grace : item->graceNotes()) { - redoLedgerLines(grace); + grace->updateLedgerLines(); } } } @@ -3278,8 +3259,8 @@ void ChordLayout::layoutNote2(Note* item, LayoutContext& ctx) e->mutldata()->setMag(item->mag()); Shape noteShape = item->shape(); noteShape.remove_if([e](ShapeElement& s) { return s.item() == e || s.item()->isBend(); }); - LedgerLine* ledger = item->line() < -1 || item->line() > item->staff()->lines(item->tick()) - ? chord->ledgerLines() : nullptr; + LedgerLine* ledger = (item->line() < -1 || item->line() > item->staff()->lines(item->tick())) && !chord->ledgerLines().empty() + ? chord->ledgerLines().front() : nullptr; if (ledger) { noteShape.add(ledger->shape().translate(ledger->pos() - item->pos())); } @@ -3497,7 +3478,7 @@ void ChordLayout::fillShape(const Chord* item, ChordRest::LayoutData* ldata) shape.add(chordRestShape(item)); // add lyrics - for (const LedgerLine* l = item->ledgerLines(); l; l = l->next()) { + for (const LedgerLine* l : item->ledgerLines()) { shape.add(l->shape().translate(l->pos() - l->staffOffset())); } diff --git a/src/engraving/rendering/dev/slurtielayout.cpp b/src/engraving/rendering/dev/slurtielayout.cpp index 8b4ab37fcaf5f..f7631ac1438f3 100644 --- a/src/engraving/rendering/dev/slurtielayout.cpp +++ b/src/engraving/rendering/dev/slurtielayout.cpp @@ -1649,7 +1649,7 @@ void SlurTieLayout::adjustX(TieSegment* tieSegment, SlurTiePos& sPos, Grip start void SlurTieLayout::adjustXforLedgerLines(TieSegment* tieSegment, bool start, Chord* chord, Note* note, const PointF& chordSystemPos, double padding, double& resultingX) { - if (tieSegment->tie()->isInside() || !chord->ledgerLines()) { + if (tieSegment->tie()->isInside() || chord->ledgerLines().empty()) { return; } @@ -1660,7 +1660,7 @@ void SlurTieLayout::adjustXforLedgerLines(TieSegment* tieSegment, bool start, Ch bool ledgersAbove = false; bool ledgersBelow = false; - for (LedgerLine* ledger = chord->ledgerLines(); ledger; ledger = ledger->next()) { + for (LedgerLine* ledger : chord->ledgerLines()) { if (ledger->y() < 0.0) { ledgersAbove = true; } else { @@ -1697,7 +1697,7 @@ void SlurTieLayout::adjustYforLedgerLines(TieSegment* tieSegment, SlurTiePos& sP } Chord* chord = note->chord(); - if (!chord->ledgerLines()) { + if (chord->ledgerLines().empty()) { return; } PointF chordSystemPos = chord->pos() + chord->segment()->pos() + chord->segment()->measure()->pos(); @@ -1706,7 +1706,7 @@ void SlurTieLayout::adjustYforLedgerLines(TieSegment* tieSegment, SlurTiePos& sP int upSign = tie->up() ? -1 : 1; double margin = 0.4 * spatium; - for (LedgerLine* ledger = chord->ledgerLines(); ledger; ledger = ledger->next()) { + for (LedgerLine* ledger : chord->ledgerLines()) { PointF ledgerPos = ledger->pos() + chordSystemPos; double yDiff = upSign * (ledgerPos.y() - tiePoint.y()); bool collision = yDiff > 0 && yDiff < margin; diff --git a/src/engraving/rendering/stable/chordlayout.cpp b/src/engraving/rendering/stable/chordlayout.cpp index 43aad6766182b..3fccc06bd68fb 100644 --- a/src/engraving/rendering/stable/chordlayout.cpp +++ b/src/engraving/rendering/stable/chordlayout.cpp @@ -101,12 +101,6 @@ void ChordLayout::layoutPitched(Chord* item, LayoutContext& ctx) double chordX = (item->noteType() == NoteType::NORMAL) ? item->ldata()->pos().x() : 0.0; - while (item->ledgerLines()) { - LedgerLine* l = item->ledgerLines()->next(); - delete item->ledgerLines(); - item->setLedgerLine(l); - } - double lll = 0.0; // space to leave at left of chord double rrr = 0.0; // space to leave at right of chord double lhead = 0.0; // amount of notehead to left of chord origin @@ -169,7 +163,7 @@ void ChordLayout::layoutPitched(Chord* item, LayoutContext& ctx) // create ledger lines //----------------------------------------- - item->addLedgerLines(); + item->updateLedgerLines(); // If item has an arpeggio: mark chords which are part of the arpeggio if (item->arpeggio()) { @@ -335,12 +329,6 @@ void ChordLayout::layoutTablature(Chord* item, LayoutContext& ctx) layoutTablature(c, ctx); } - while (item->ledgerLines()) { - LedgerLine* l = item->ledgerLines()->next(); - delete item->ledgerLines(); - item->setLedgerLine(l); - } - double lll = 0.0; // space to leave at left of chord double rrr = 0.0; // space to leave at right of chord Note* upnote = item->upNote(); @@ -425,9 +413,7 @@ void ChordLayout::layoutTablature(Chord* item, LayoutContext& ctx) // create ledger lines, if required (in some historic styles) if (ledgerLines > 0) { -// there seems to be no need for widening 'ledger lines' beyond fret mark widths; more 'on the field' -// tests and usage will show if this depends on the metrics of the specific fonts used or not. -// double extraLen = style().styleS(Sid::ledgerLineLength).val() * _spatium; + item->resizeLedgerLinesTo(ledgerLines); double extraLen = 0; double llX = stemX - (headWidth + extraLen) * 0.5; for (int i = 0; i < ledgerLines; i++) { @@ -437,8 +423,6 @@ void ChordLayout::layoutTablature(Chord* item, LayoutContext& ctx) ldgLin->setVisible(item->visible()); ldgLin->setLen(headWidth + extraLen); ldgLin->setPos(llX, llY); - ldgLin->setNext(item->ledgerLines()); - item->setLedgerLine(ldgLin); TLayout::layoutLedgerLine(ldgLin, ctx); llY += lineDist / ledgerLines; } @@ -3499,8 +3483,9 @@ void ChordLayout::layoutNote2(Note* item, LayoutContext& ctx) e->mutldata()->setMag(item->mag()); Shape noteShape = item->shape(); noteShape.remove_if([e](ShapeElement& s) { return s.item() == e || s.item()->isBend(); }); - LedgerLine* ledger = item->line() < -1 || item->line() > item->staff()->lines(item->tick()) - ? item->chord()->ledgerLines() : nullptr; + LedgerLine* ledger = (item->line() < -1 || item->line() > item->staff()->lines(item->tick())) + && !item->chord()->ledgerLines().empty() + ? item->chord()->ledgerLines().front() : nullptr; if (ledger) { noteShape.add(ledger->shape().translate(ledger->pos() - item->pos())); } @@ -3729,7 +3714,7 @@ void ChordLayout::fillShape(const Chord* item, ChordRest::LayoutData* ldata, con shape.add(chordRestShape(item, conf)); // add lyrics - for (const LedgerLine* l = item->ledgerLines(); l; l = l->next()) { + for (const LedgerLine* l : item->ledgerLines()) { shape.add(l->shape().translate(l->pos())); } diff --git a/src/engraving/rendering/stable/slurtielayout.cpp b/src/engraving/rendering/stable/slurtielayout.cpp index cce133a85b2d5..1fc4a0ee76028 100644 --- a/src/engraving/rendering/stable/slurtielayout.cpp +++ b/src/engraving/rendering/stable/slurtielayout.cpp @@ -1766,7 +1766,7 @@ void SlurTieLayout::adjustX(TieSegment* tieSegment, SlurTiePos& sPos, Grip start void SlurTieLayout::adjustXforLedgerLines(TieSegment* tieSegment, bool start, Chord* chord, Note* note, const PointF& chordSystemPos, double padding, double& resultingX) { - if (tieSegment->tie()->isInside() || !chord->ledgerLines()) { + if (tieSegment->tie()->isInside() || chord->ledgerLines().empty()) { return; } @@ -1777,7 +1777,7 @@ void SlurTieLayout::adjustXforLedgerLines(TieSegment* tieSegment, bool start, Ch bool ledgersAbove = false; bool ledgersBelow = false; - for (LedgerLine* ledger = chord->ledgerLines(); ledger; ledger = ledger->next()) { + for (LedgerLine* ledger : chord->ledgerLines()) { if (ledger->y() < 0.0) { ledgersAbove = true; } else { @@ -1812,7 +1812,7 @@ void SlurTieLayout::adjustYforLedgerLines(TieSegment* tieSegment, SlurTiePos& sP } Chord* chord = note->chord(); - if (!chord->ledgerLines()) { + if (chord->ledgerLines().empty()) { return; } PointF chordSystemPos = chord->pos() + chord->segment()->pos() + chord->segment()->measure()->pos(); @@ -1821,7 +1821,7 @@ void SlurTieLayout::adjustYforLedgerLines(TieSegment* tieSegment, SlurTiePos& sP int upSign = tie->up() ? -1 : 1; double margin = 0.4 * spatium; - for (LedgerLine* ledger = chord->ledgerLines(); ledger; ledger = ledger->next()) { + for (LedgerLine* ledger : chord->ledgerLines()) { PointF ledgerPos = ledger->pos() + chordSystemPos; double yDiff = upSign * (ledgerPos.y() - tiePoint.y()); bool collision = yDiff > 0 && yDiff < margin;