Skip to content

Commit

Permalink
Reimplement ledger lines
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@mike-spa
mike-spa committed Aug 13, 2024
1 parent 611205a commit 18fdbac
Show file tree
Hide file tree
Showing 10 changed files with 54 additions and 86 deletions.
41 changes: 26 additions & 15 deletions src/engraving/dom/chord.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,6 @@ std::vector<int> Chord::noteDistances() const
Chord::Chord(Segment* parent)
: ChordRest(ElementType::CHORD, parent)
{
m_ledgerLines = 0;
m_stem = 0;
m_hook = 0;
m_stemDirection = DirectionV::AUTO;
Expand All @@ -285,7 +284,6 @@ Chord::Chord(const Chord& c, bool link)
if (link) {
score()->undo(new Link(this, const_cast<Chord*>(&c)));
}
m_ledgerLines = 0;

for (Note* onote : c.m_notes) {
Note* nnote = Factory::copyNote(*onote, link);
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -1155,7 +1149,7 @@ void Chord::processSiblings(std::function<void(EngravingItem*)> func, bool inclu
func(m_tremoloSingleChord);
}
if (includeTemporarySiblings) {
for (LedgerLine* ll = m_ledgerLines; ll; ll = ll->next()) {
for (LedgerLine* ll : m_ledgerLines) {
func(ll);
}
}
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
}
}
Expand Down
9 changes: 4 additions & 5 deletions src/engraving/dom/chord.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<LedgerLine*>& 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; }
Expand Down Expand Up @@ -352,7 +351,7 @@ class Chord final : public ChordRest
void processSiblings(std::function<void(EngravingItem*)> func, bool includeTemporarySiblings) const;

std::vector<Note*> m_notes; // sorted to decreasing line step
LedgerLine* m_ledgerLines = nullptr; // single linked list
std::vector<LedgerLine*> m_ledgerLines;

Stem* m_stem = nullptr;
Hook* m_hook = nullptr;
Expand Down
1 change: 0 additions & 1 deletion src/engraving/dom/ledgerline.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ LedgerLine::LedgerLine(EngravingItem* s)
{
setSelectable(false);
m_len = 0.;
m_next = 0;
}

LedgerLine::~LedgerLine()
Expand Down
3 changes: 0 additions & 3 deletions src/engraving/dom/ledgerline.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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
Expand Down
6 changes: 2 additions & 4 deletions src/engraving/dom/scoretree.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand Down
4 changes: 1 addition & 3 deletions src/engraving/rendering/dev/accidentalslayout.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
33 changes: 7 additions & 26 deletions src/engraving/rendering/dev/chordlayout.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -2550,19 +2540,10 @@ void ChordLayout::layoutChords3(const std::vector<Chord*>& chords,

void ChordLayout::layoutLedgerLines(const std::vector<Chord*>& 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();
}
}
}
Expand Down Expand Up @@ -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()));
}
Expand Down Expand Up @@ -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()));
}

Expand Down
8 changes: 4 additions & 4 deletions src/engraving/rendering/dev/slurtielayout.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -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 {
Expand Down Expand Up @@ -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();
Expand All @@ -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;
Expand Down
27 changes: 6 additions & 21 deletions src/engraving/rendering/stable/chordlayout.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()) {
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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++) {
Expand All @@ -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;
}
Expand Down Expand Up @@ -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()));
}
Expand Down Expand Up @@ -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()));
}

Expand Down
Loading

0 comments on commit 18fdbac

Please sign in to comment.