Skip to content
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

Note sorting algorithm rework #3881

Merged
merged 2 commits into from Oct 19, 2017
Merged

Conversation

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Oct 14, 2017

Addresses some of the issues in #3880

Sorting after position and key. Probably only needed with the arpeggiator. This works well and doesn't seem to use a whole lot cpu cycles. The notes are sorted on input from mouse or keyboard in Pattern::addNote(). Actions on the notes in the Piano Roll also triggers a sort via Pattern::rearrangeAllNotes() with a helper function in note.h . The sorting is duplicated when adding notes in the Piano Roll and this has its own post in #3880 but is probably a wontfix.

I've pulled these against stable-1.2 but they can just as well go into master or be split over the two.

@zonkmachine
Copy link
Member Author

@zonkmachine zonkmachine commented Oct 14, 2017

Note that old patches will open up like before and all will be fine. The next time you edit a track however it will be sorted with the new method and this will probably be only an advantage. Probably. Maybe you like the notes to shift around a bit, and you'll only really notice this in arpeggiated tracks with more than two simultaneous notes and under sort mode.
If this is not wanted you can just open/press a note in the piano roll to trigger a sort/save the project in 1.1.3 and it'll go wonky again.

@zonkmachine
Copy link
Member Author

@zonkmachine zonkmachine commented Oct 14, 2017

Got a random crash. I don't think it's related to this PR though. The project had been running for maybe an hour or so. I was looping two 3osc tracks with an automation track for the Master pitch, two notes detuned in the one track and an LFO detuning the other synth.
m_frequencyNeedsUpdate = true; LFO ... or Note detune ... or Master pitch?

bt / bt full

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x000000000062e0ea in NotePlayHandle::setFrequencyUpdate (this=0x81) at /home/zonkmachine/builds/lmms/lmms/include/NotePlayHandle.h:263
263			m_frequencyNeedsUpdate = true;
(gdb) bt
#0  0x000000000062e0ea in NotePlayHandle::setFrequencyUpdate (this=0x81) at /home/zonkmachine/builds/lmms/lmms/include/NotePlayHandle.h:263
#1  0x00000000006270a1 in InstrumentTrack::updateBaseNote (this=0x7fce29e77620) at /home/zonkmachine/builds/lmms/lmms/src/tracks/InstrumentTrack.cpp:543
#2  0x000000000062710a in InstrumentTrack::updatePitch (this=0x7fce29e77620) at /home/zonkmachine/builds/lmms/lmms/src/tracks/InstrumentTrack.cpp:552
#3  0x000000000063f89f in InstrumentTrack::qt_static_metacall (_o=0x7fce29e77620, _c=QMetaObject::InvokeMetaMethod, _id=6, _a=0x7fffe6625550)
    at /home/zonkmachine/builds/lmms/lmms/build/src/moc_InstrumentTrack.cpp:68
#4  0x00007fce35cf187a in QMetaObject::activate (sender=0x7fce29e78898, m=<optimized out>, local_signal_index=<optimized out>, argv=0x0)
    at kernel/qobject.cpp:3539
#5  0x0000000000642f4b in Model::dataChanged (this=0x7fce29e78898) at /home/zonkmachine/builds/lmms/lmms/build/src/moc_Model.cpp:102
#6  0x0000000000642e09 in Model::qt_static_metacall (_o=0x7fce29e78898, _c=QMetaObject::InvokeMetaMethod, _id=0, _a=0x7fffe66256b0)
    at /home/zonkmachine/builds/lmms/lmms/build/src/moc_Model.cpp:51
#7  0x00007fce35cf187a in QMetaObject::activate (sender=0x2ee0210, m=<optimized out>, local_signal_index=<optimized out>, argv=0x0)
    at kernel/qobject.cpp:3539
#8  0x000000000063b4ab in ControllerConnection::valueChanged (this=0x2ee0210)
    at /home/zonkmachine/builds/lmms/lmms/build/src/moc_ControllerConnection.cpp:104
#9  0x000000000063b354 in ControllerConnection::qt_static_metacall (_o=0x2ee0210, _c=QMetaObject::InvokeMetaMethod, _id=0, _a=0x7fcde40521b0)
    at /home/zonkmachine/builds/lmms/lmms/build/src/moc_ControllerConnection.cpp:52
@zonkmachine zonkmachine force-pushed the zonkmachine:arppatternsort branch from 8db7801 to 7922673 Oct 14, 2017
@PhysSong
Copy link
Member

@PhysSong PhysSong commented Oct 14, 2017

NotePlayHandle::setFrequencyUpdate (this=0x81) was called in

( *it )->setFrequencyUpdate();

and it may indicate an invalid reference of iterator occured.

@PhysSong
Copy link
Member

@PhysSong PhysSong commented Oct 14, 2017

m_processHandles is used in non-thread-safe way(i.e. without proper synchronization). (this=0x81) means *it == 0x81, which is an invalid address.
Anyway, I'm pretty sure it's unrelated with this PR. For this kind of crashes, I'll open a separate issue. :)

@gi0e5b06
Copy link

@gi0e5b06 gi0e5b06 commented Oct 14, 2017

#3879 seems related to this NotePlayHandle race condition.

@tresf tresf mentioned this pull request Oct 16, 2017
16 of 16 tasks complete
@zonkmachine
Copy link
Member Author

@zonkmachine zonkmachine commented Oct 16, 2017

This should go into master. I'll switch it over.

@zonkmachine zonkmachine force-pushed the zonkmachine:arppatternsort branch from 7922673 to 697d375 Oct 17, 2017
@zonkmachine zonkmachine changed the base branch from stable-1.2 to master Oct 17, 2017
@zonkmachine zonkmachine force-pushed the zonkmachine:arppatternsort branch from 697d375 to ab8db48 Oct 17, 2017
@zonkmachine zonkmachine changed the base branch from master to stable-1.2 Oct 17, 2017
@zonkmachine
Copy link
Member Author

@zonkmachine zonkmachine commented Oct 17, 2017

I switched back to stable 1.2 and this is ready for a review. Switching sorting order was a matter of flipping over two 'lesser than' signs and this is now less buggy than before. See the note on backward compatibility here.

@zonkmachine zonkmachine mentioned this pull request Oct 17, 2017
4 of 10 tasks complete
@zonkmachine zonkmachine force-pushed the zonkmachine:arppatternsort branch from ab8db48 to 8661780 Oct 17, 2017
@@ -213,7 +213,7 @@ Note * Pattern::addNote( const Note & _new_note, const bool _quant_pos )
}

instrumentTrack()->lock();
if( m_notes.size() == 0 || m_notes.back()->pos() <= new_note->pos() )
if( m_notes.size() == 0 || m_notes.back()->pos() < new_note->pos() )

This comment has been minimized.

@lukas-w

lukas-w Oct 17, 2017
Member

As far as I understand the code, all lines 216 to 245 do is insert the new note at the right position (sorted by the criteria in lessThan). If so, we can reduce this to a single line:

m_notes.insert(std::upper_bound(m_notes.begin(), m_notes.end(), new_note, Note:lessThan), new_note);

This reduces code complexity, improves performance by doing a binary search, and removes the duplicate compare code by using the lessThan method.

This comment has been minimized.

@zonkmachine

zonkmachine Oct 18, 2017
Author Member

As far as I understand the code, all lines 216 to 245 do is insert the new note at the right position (sorted by the criteria in lessThan).

Yes indeed. Oneliner, nice but I can't make it work.

Computer says no. 8)

/usr/include/c++/4.8/bits/stl_algo.h: In instantiation of ‘_FIter std::upper_bound(_FIter, _FIter, const _Tp&, _Compare) [with _FIter = Note**; _Tp = Note*; _Compare = bool (*)(Note*&, Note*&)]’:
/home/zonkmachine/builds/lmms/lmms/src/tracks/Pattern.cpp:216:90:   required from here
/usr/include/c++/4.8/bits/stl_algo.h:2543:31: error: invalid initialization of reference of type ‘Note*&’ from expression of type ‘Note* constif (__comp(__val, *__middle))
                               ^

This comment has been minimized.

@lukas-w

lukas-w Oct 18, 2017
Member

Ah, you'll have to change Note::lessThan to take const pointers:

static inline bool lessThan( const Note * lhs, const Note * rhs )

This comment has been minimized.

@zonkmachine

zonkmachine Oct 18, 2017
Author Member

Perfect, thanks!

@zonkmachine zonkmachine force-pushed the zonkmachine:arppatternsort branch from 8661780 to 81966fa Oct 18, 2017
@zonkmachine
Copy link
Member Author

@zonkmachine zonkmachine commented Oct 19, 2017

I took the opportunity to practice and updated the other sorting function to std::sort() as qSort() is obsolete.

Now I think I'm done. The notes are correctly sorted for both functions and there is no code changes needed to save/load the patterns correctly.

@lukas-w lukas-w merged commit fbfcb43 into LMMS:stable-1.2 Oct 19, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zonkmachine zonkmachine deleted the zonkmachine:arppatternsort branch Oct 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.