undoable instrument-changes - #1262

[grindhold]

I started working on #1262 for a couple of time now.
I added a journal-checkpoint when an element is being dropped on the instrument-slot of an instrument track. the current result: i can ctrl-z back until i get the last instrument that was on that track but only after a few times ctrl-z-ing.
my guess: the plugin that is being loaded makes some more journalcheckpoints on load.
i made some debugoutput. the elements getting "undone" before the instrument itself are mostly analogmodels.

Will continue solving the riddle, but hints are appreciated ;)

P.S.: while unexpanding the file edited by me, i stumbled across some codestyle-violations (spaces instead of tabs).

[diizy]

Hm, I think we should figure out the possible problems (leaks?) with the journaling code before merging this.

[grindhold]

i think so, too. that's why i was asking for hints, parallel to investigating further by myself :) maybe there is some dev out there who knows where exactly this problem may originate. i will push further changes into this PR until it's good

[grindhold]

The problem: every AutomatedModel::setValue leads to a new undo-checkpoint. This is not correct, as we only want user-triggered value-changes to generate a new checkpoint. setValue is called when loading instrument settings several times wich leads to the "leaking" checkpoints.
My approach on solving that: AutomatedModel::setValue gets a new bool parameter "ignoreJournal" which determines wheter the value should enter its change into the journal.
Further rework the codebase and decide for every call to setValue wheter the value-change should be recorded or not.

It's pretty much to do, so i would like a little 'go' before i start doing it. Does anyone have a better idea?

[diizy]

On 11/10/2014 02:24 AM, grindhold wrote:

The problem: every AutomatedModel::setValue leads to a new
undo-checkpoint. This is not correct, as we only want user-triggered
value-changes to generate a new checkpoint. setValue is called when
loading instrument settings several times wich leads to the "leaking"
checkpoints.
My approach on solving that: AutomatedModel::setValue gets a new bool
parameter "ignoreJournal" which determines wheter the value should
enter its change into the journal.
Further rework the codebase and decide for every call to setValue
wheter the value-change should be recorded or not.

It's pretty much to do, so i would like a little 'go' before i start
doing it. Does anyone have a better idea?

Well it's a tricky situation. I don't think it's a very good idea to
overload setValue() with more functionality. Morelike we need separate
functions for 1) values set by software and 2) values set from the GUI.

However there's a literal metric shitton of work required to do this so
I have to question whether this is a good use of your or anyone's time
at this point... might be better to consider it as a part of the 2.0
effort where things get turned over anyway.

It's likely that there's going to be some changes to how models work,
particularly wrt. automations - we're probably going to move away from
automations pushing data to models, and instead convert to a system
where models pull data directly from automations, controllers, etc.

[tresf]

Morelike we need separate functions for 1) values set by software and 2) values set from the GUI.

Ideally, if each knob change has event data, we can examine the event data for human-interaction, no? This may require the AutomatableModel to accept events instead of raw values, but it seems like it could be added in addition to current functionality (hypothetically proposed here as I'm not currently looking at the code).

-Tres

[diizy]

On 11/10/2014 02:40 AM, Tres Finocchiaro wrote:

Morelike we need separate functions for 1) values set by software
and 2) values set from the GUI.

Ideally, if each knob change has event data, we can examine the event
data for human-interaction, no?

With events, there's always the problem that the events have to be
stored somewhere for processing, ie. they have to be gathered into an
array. If there are multiple places where events can be coming from (ie.
multiple threads), this means that the array structure the events are
gathered in must be thread-safe, and structures that are both
thread-safe and RT-safe are notoriously tricky to implement.

We're going to have to tackle this hurdle when it comes to live
Midi-events, but I don't think there's really cause for the same
complexity in AutomatableModels, since these models are "singular" in
that they only have one value at any one time - unlike Midi, where there
can be anywhere from 0-?? Midievents happening during a period...
multiple value-change-events during a period wouldn't make sense for
AutomatableModels, they'd just cancel each other out, or we'd have to
pick one and discard others, anyway, since only one value can be set at
one time.

(Yes there's sample-exact models but the same logic applies to them...
they just have one value per frame, instead of one value per period.)

[grindhold]

under the given circumstances i don't think, i can provide a proper solution to the problem. i am going to withdraw this PR and look for another problem to solve.
someone could cherry-pick those tab-space-fixes nevertheless.

[diizy]

On 11/10/2014 03:26 AM, grindhold wrote:

under the given circumstances i don't think, i can provide a proper
solution to the problem. i am going to withdraw this PR and look for
another problem to solve.
someone could cherry-pick those tab-space-fixes nevertheless.

Yeah, it's best to come back to this later.

The other problem with journaling AutomatableModel is, even if you
separate the GUI-originating events into their own function, every knob
tweak is still going to call that function several times, spamming the
undo stack... so if we want there to be any sense in the feature, we
should only save checkpoints once every mouse click, so only when the
mouse button is released. To do this, the knob has to somehow
communicate to the model when it's being fingered and when it isn't.
That's something that probably will be done in the future, so maybe this
needs to wait until that feature is done first...