Undo/Operations etc

[spamatica]

Hey Tim,
I wanted to ask you about the undo/operations system.
I see myself coming up against it more and more and really feeling limited with not understanding how to bend it to my will.

I'm not sure if it was you who originally created it but you know most about it.
Could be it is simply HARD and my feeble mind can never grasp it enough.. but maybe you have some pointers on how it is supposed to work?

Matter at hand, I tried to understand why the recently reported bug with dragging parts outside tracks causes a crash.
I had a look and though I'm definitely not sure it has anything to do with undo, it's there, and confuses my understanding on exactly what is going on.
In this case I can fairly well understand the code of PartCanvas::moveItem where it creates a new track for the part to be put on. But then we get to the operations part and it seems more hairy.
I guess the first question is, will these only be executed if undo is performed?
Seems to me it does things directly, but what exactly?

[terminator356]

Hi. In PartCanvas::moveCanvasItems() we have this:

for(iCItem ici = items.begin(); ici != items.end(); ++ici)
{
CItem* ci = ici->second;
...
bool result=moveItem(operations, ci, newpos, dtype); // <<< Causes deletion of items.
if (result)
ci->move(newpos); // <<< Crashes here. Item was deleted.
}

Unfortunately the call to moveItem() above results in a new track being created,
which sends out SC_TRACK_INSERTED, which Arranger::songChanged() picks up
and causes a call to PartCanvas::updateItems(), which calls items.clearDelete()
which completely deletes the canvas item list. So next, the call to ci->move() above crashes
because that item is no longer valid (in fact the whole 'for' loop above is invalid).

Tricky. A lot of things going on there. Looking at solutions now...

[spamatica]

Cool, would have taken me a bit to figure out, I see the problem now. What about the general layout of the undo system? I guess it was not involved here but it is on many other things, any pointers? Den fre 27 mars 2020 kl 22:03 skrev Tim <[email protected]>:

…

Hi. In PartCanvas::moveCanvasItems() we have this: for(iCItem ici = items.begin(); ici != items.end(); ++ici) { CItem* ci = ici->second; ... bool result=moveItem(operations, ci, newpos, dtype); // <<< Causes deletion of items. if (result) ci->move(newpos); // <<< Crashes here. Item was deleted. } Unfortunately the call to moveItem() above results in a new track being created, which sends out SC_TRACK_INSERTED, which Arranger::songChanged() picks up and causes a call to PartCanvas::updateItems(), which calls items.clearDelete() which completely deletes the canvas item list. So next, the call to ci->move() above crashes because that item is no longer valid (in fact the whole 'for' loop above is invalid). Tricky. A lot of things going on there. Looking at solutions now... — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <https://github.com/orgs/muse-sequencer/teams/musedevteam/discussions/39/comments/1>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABCFAN6XMGC3BJBTSJNJX7DRJUHPZANCNFSM4LVDOMKA> .

[spamatica]

any pointers?

no pun intended :D

…

[terminator356]

Since it's a very complicated topic, I started a README.
https://github.com/muse-sequencer/muse/blob/master/muse3/README.developer.undo_system
Let me know if it looks OK. Anything to answer there? More specifics?
Yes, previously there was just the Undo system.
But it was very inefficient in the realtime stage, executing some real heavy code there.
So I created the lean-and-mean Operations system where things are executed as fast as possible,
in conjunction with the Undo system. Read more in the README.
Thanks.

[terminator356]

Hey guys sorry for the delay. Hope you are all well.

recently reported bug with dragging parts outside tracks causes a crash

Fixed now! With bonuses! And several other fixes! See ChangeLog.
@spamatica You had a comment when duplicating tracks with your smart '#' track naming code:

there is still a gaping hole in the logic, making multiple duplicates of multiple
tracks still does not produce valid results

Your logic was OK. It was the fact that the tracks are not added to the song yet.
This is fixed, and all new unique track naming is centralized now,
with a new class TrackNameFactory. The class embraces your smart code, modified slightly.
See the ChangeLog.

[spamatica]

Hey Tim, sorry back :D
I'm erratic at best and with the world upside down all bets are off..

Great with the fixes!

Back to the matter at hand, I've started to digest what you have written so far about Undo.
I will try to get back with some more hands on questions.

Reading your explanation about the RT execution having to have happened before we are back in GUI-land. It sounds a bit odd that it is theoretically possible for the GUI to continue even if the RT execution isn't done. Cannot this be handled with a spinlock? Basically the GUI waiting for a flag being cleared before it is allowed to continue.

Reading on.

[spamatica]

Hiya,
got an issue that might not be directly related to undo but it happens there.
I'm messing with resize of parts, specifically on the left side.
This involves rewriting the position of the events inside the part, to counter the new start position of the part.

I found out that it's the tick in the event multimap that is used by the painting (not the tick/frame in the event) so I managed to edit that.
But I can't get it to propagate to the painting in PartCanvas. I read out the tick pos of the events in the part being painted and it's still the old position (the event is however updated as it should, as I of course update that too).
Is the event multimap for the part duplicated somewhere and this information is lost, or is there something else I need to do?

Haven't committed the latest, I can provide source examples if that was hard to follow.

[terminator356]

Hey bro.
The description is a little vague.
Yes I see you're working in your branch these days.
Can you point me to the specific area(s) of trouble, or else provide code?
Hopefully it'll just be one of those "ahhh, you need to do this..." moments.
Thanks.

[spamatica]

Yeah, wanted to get it out there but didn't have the energy to explain clearly, here's a new try.
Lets start off with my latest checkin
d81a678

I added a debug print in pcanvas.cpp that prints the tick of the event in a part that is painted.
below in operations.cpp is the code that is supposed to change the tick position of the event when editing the start-position of the part.
The interesting bit is from line 1149 where every event is supposed to be updated with a new tick value.
Note that below the editing the tick is printed to show that it has been correctly set.

A testcase I had was the following:

1. add a part
2. add an event in the part at tick 6144
3. move the beginning of the part to the right until the event was 1536 ticks from the start

The printouts I get from the print statements is as follows.
Tickpos 6144 6144 // from pcanvas when it is originally drawn, both the multimap and the event tick is 6144 before 3. is performed
pos after =1536 // from Operations showing that the multimap has been changed to 1536
Tickpos 6144 1536 // from pcanvas when the part is redrawn after 3. is done, showing that the event tick has been updated but the multimap still has the old value.

Any ideas what could be wrong? I, of course, want both values to have been updated as I suppose both can be used at various places, I have only been looking at the gui painting.

[terminator356]

Hey mon! Hope your don't mind I pushed a couple of fixes to your code.

In your ModifyPartStart and ModifyPartLength Operations handlers, you had like this:
EventList eventList = _part->events();
You were modifying a copy of the list, which has no effect. You want:
EventList& eventList = _part->nonconst_events();

Also, watch out for auto and constant iterator stuff.
In your ModifyPartLength handler there's a harmless non-modifying loop:
for (auto ci = eventList.begin(); ci != eventList.end(); ++ci) {
But ci will be non-constant. So I changed it to:
for (auto ci = eventList.cbegin(); ci != eventList.cend(); ++ci) {

I researched the whole constant iterator thing a few weeks ago.
Yes, there are constant versions of begin() and end() etc.
but do not rely on them all the time unless you are really sure the compiler will treat the iterator as constant.
(Use your IDE's highlight feature to check, works well in KDevelop.)
I found cases where one would think the compiler would use the constant version in a particular context,
but it does not - the compiler uses the non-constant version.
So lately I've been changing everything in my travels to cbegin() and cend() etc. where warranted.
Those versions are the only way to guarantee a constant iterator is used.

Side note: Been looking at C++17 features and even C++20.
In C++17 you have access to all the 'nodes' of a container. One benefit is you can modify
a (multi-)map item's key in place, without having to remove the item and then reinsert it.
C++20 is quite radically different. New thing called 'Concepts'.

Cheers. Lemme know if any trouble...

[terminator356]

PS: When I pushed the changes, Github said I recently "pushed a branch rj_development".
I had clicked "checkout as a new branch" on the branch in Git Cola.
I hope that does not mean I overwrite it with a new version or something.
Eek. Sorry if I messed something up.

[spamatica]

Den tis 21 apr. 2020 kl 00:30 skrev Tim <[email protected]>:

Hey mon! Hope your don't mind I pushed a couple of fixes to your code. In your ModifyPartStart and ModifyPartLength Operations handlers, you had like this: EventList eventList = _part->events(); You were modifying a *copy* of the list, which has no effect. You want: EventList& eventList = _part->nonconst_events();

Oh damn. I was so sure that part was correct (thinking it was based on pointers somehow) so I didn't reflect further on it. Super obvious now, thanks!

Also, watch out for auto and constant iterator stuff. In your ModifyPartLength handler there's a harmless non-modifying loop: for (auto ci = eventList.begin(); ci != eventList.end(); ++ci) { But ci will be non-constant. So I changed it to: for (auto ci = eventList.cbegin(); ci != eventList.cend(); ++ci) { I researched the whole constant iterator thing a few weeks ago. Yes, there are constant versions of begin() and end() etc. but do not rely on them all the time unless you are really sure the compiler will treat the iterator as constant. (Use your IDE's highlight feature to check, works well in KDevelop.) I found cases where one would *think* the compiler would use the constant version in a particular context, but it does not - the compiler uses the non-constant version. So lately I've been changing everything in my travels to cbegin() and cend() etc. where warranted. Those versions are the only way to *guarantee* a constant iterator is used.

I might be to dense about this, what could potentially happen if the iterator isn't constant? I think these for loops could also be changed into: for (auto &ci : eventList) which is very attractive to me since it is much easier to read, but maybe it has potential issues. Side note: Been looking at C++17 features and even C++20.

In C++17 you have access to all the 'nodes' of a container. One benefit is you can modify a (multi-)map item's key in place, *without* having to remove the item and then reinsert it. C++20 is quite radically different. New thing called 'Concepts'.

I mentioned I've been watching the guy on youtube making the Serenity operating system from scratch, it has giving me inspiration to learn quite a bit about the new versions of C++, a lot of very cool stuff! ...though knowing myself I'm still not a great programmer, but still ;) Cheers. Lemme know if any trouble...

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/orgs/muse-sequencer/teams/musedevteam/discussions/39/comments/10>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABCFAN7MBL55IAI3PB6OOQTRNTEGJANCNFSM4LVDOMKA> .

[spamatica]

Hehe, we will see ;) Den tis 21 apr. 2020 kl 00:34 skrev Tim <[email protected]>:

…

PS: When I pushed the changes, Github said I recently "pushed a branch rj_development". I had clicked "checkout as a new branch" on the branch in Git Cola. I hope that does not mean I overwrite it with a new version or something. Eek. Sorry if I messed something up. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/orgs/muse-sequencer/teams/musedevteam/discussions/39/comments/11>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABCFAN46JZ6DIPZPOFM3N2LRNTEWHANCNFSM4LVDOMKA> .

[terminator356]

I might be to dense about this, what could potentially happen if the
iterator isn't constant?

If nothing else, it just looks good and is good habit when you don't really need a non-constant iterator..
There is talk that they may be faster than non-const iterators,
but even if not, they won't be slower, so you never lose by using them.