Skip to content

Refactor PianoRoll::getSelectedNotes() #7818

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
regulus79 opened this issue Mar 26, 2025 · 2 comments
Open

Refactor PianoRoll::getSelectedNotes() #7818

regulus79 opened this issue Mar 26, 2025 · 2 comments

Comments

@regulus79
Copy link
Contributor

Currently, PianoRoll::getSelectedNotes loops over all notes to find the ones where isSelected is true, adds them to a NoteVector, and returns that. This is done every time the function is called, which is not efficient. @szeli1 brought this to my attention in #7759.

This is mostly a TODO issue to remind me to do this, but if anyone else wants to try to tackle it, feel free!

Implementation Ideas

  • The selected notes should probably be cached in some kind of vector.
  • Since the piano roll is afaik the only class which actually interacts with the isSelected state of the notes, perhaps for all the occurances of a note being selected/deslected, we could add a line of code which adds/removes that note from the vector. We would have to be very careful to address all the occurances, and also repopulate the vector whenever a new clip is opened in the piano roll (Since selections of notes persist within each MidiClip; selecting some notes in one clip, opening another clip, and opening the old clip will retain the selection).
  • Or, we could implement that logic within Note::setSelected. However, this would probably require either giving Note access to the piano roll or having some kind of static vector, which may not be desirable.
@messmerd
Copy link
Member

In places where we only need to loop over certain elements that meet some condition and don't need to store the results, we could make use of C++20 ranges, specifically std::ranges::filter_view.

I recently used it with success to remove some dynamic allocations in real-time code here:
messmerd@f2637d4

Maybe I could refactor PianoRoll::getSelectedNotes as well, then open a PR.

@musikBear
Copy link

This is done every time the function is called, which is not efficient.

That is of cause true, but it is also not in a efficiency-critical situation, because it does not happen during playback. What i mean is that this is not broken, maybe a bit clonky but only under the 'hood' and it works.
There are a lot that is missing where some are crippling and severe. One such example could be "Cut-your-self" for overlapping notes, a feature-miss that causes disharmony and make it difficult to get clean mixes. Eg important features that is missing, and make LMMS perform inferior compared to other DAWs.
Imho it would be better to spend dev-time on issues like thát and leave non-critical efficiency features that also works fine, until the 2.0 total rewrite.
Well just imo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants