Reimplement ledger lines #24011
Merged
Reimplement ledger lines #24011
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
mike-spa
force-pushed
the
reimplementLedgerLines
branch
from
18fdbac
to
45af023
Compare
oktophonie
approved these changes
Aug 13, 2024
oktophonie
approved these changes
Aug 13, 2024
| @@ -437,8 +423,6 @@ void ChordLayout::layoutTablature(Chord* item, LayoutContext& ctx) | |||
| ldgLin->setVisible(item->visible()); | |||
There was a problem hiding this comment.
Looks like we're still allocating new ledger lines here. Probably needs the same changes as the dev version.
cbjeukendrup
approved these changes
Aug 13, 2024
RomanPudashkin
approved these changes
Aug 14, 2024
RomanPudashkin
added a commit
to RomanPudashkin/MuseScore
that referenced
this pull request
Aug 20, 2024
Reimplement ledger lines
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves: a crash which happens in a specific file under specific conditions @oktophonie. The crash is due to a read-after-free so it doesn't always crash. It can be reliably reproduced only by using the adress sanitizer, which shows that this is yet another instance of our well known invalid ledger lines problem. Hopefully this is the end of that @cbjeukendrup @miiizen.
To summarize
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.