Suggestion for fixing/improving preset write operations.

[polluxsynth]

While discussing another issue (https://github.com/zynaddsubfx/zyn-fusion-issues/issues/329) , the discussion on how the preset write operation in the Zyn browser pane works. I've tried to extract the core of the discussion below.

Basically, I can't seem to do any consistent form of write operation in the browser pane. Read works fine.

First of all, I don't really understand what 'write mode' does. Whenever I want to replace an existing patch after an edit, I click on 'overwrite' which seems to replace it regardless of 'read mode' or 'write mode'. The only difference between read and write modes I can see is that in read modes, the patches are loaded when clicked on, but in write mode, nothing really happens.

Clicking on an empty patch slot (or any patch slot) in write mode doesn't seem to do anything in itself. If I first click on a slot and then click 'Overwrite', it sometimes overwrites the original .xiz file, and sometimes it creates a new one (called, -, e.g. 0001-test patch.xiz). In either case, it doesn't show up in the preset column, but if I click 'Rescan', the preset column is filled with entries named 'EMPTY PRESET', and in the case of a new .xiz file actually being written, the new patch file will be listed at the top. I can't really see any consistency as to under which conditions the patch file actually does get written, it seems to be arbitrary. In any case, when it does create a new .xiz file, it also seems to overwrite the one from which the patch originated.

Restarting zest brings back normality, and I then get the list of patches I have in the bank directory when I select the bank in question. Patches with a number (i.e. prefixed by 0001 etc) are shown without the patch number.

Then again, I realize now that the patch bank I'm working with doesn't have patch numbers in the file names, as I've saved them one by one from the file menu. Perhaps that confuses the backend that manages the bank directories.

Hm, I also realized that when I do get zest into the mode where it shows EMPTY PRESET, it seems to assume there are 128 (?) slots, and shows the existing patch files in their appropriate position in the list, see for instance the factory Guitar bank where the files are not numbered contiguously.

Is write mode perhaps supposed to list the presets that have been written with number prefixes, with unused preset numbers (no .xiz file prefixed with that nunber) displayed as EMPTY PRESET? This doesn't seem to happen, even in banks which have only numerically prefixed .xiz files, such as the factory banks.

Apparently, the intention in the code is that when clicking on a patch name in write mode it should immediately overwrite that patch with the current patch. I think that sounds rather dangerous, as it's easy to mis-click in the preset list, overwriting a patch that one didn't intend to overwrite.

TL;DR After a long dicsussion with @fundamental (https://github.com/zynaddsubfx/zyn-fusion-issues/issues/329) regarding how it is supposed to work, my suggestion is a follows:

1. Retain the current Read and Write modes, but the Overwrite button would be labelled Execute Write, or something to that effect.
2. Clicking on a slot in Write mode only selects that slot for potential writing; the write doesn't take place until Execute Write is clicked (and the button should be greyed out or in Zyn terms have a diagonal strike in Read mode to clearly show that it does not work in that mode).
3. Read mode would function like it seems to do today, listing the available presets, with the list of presets compressed into a contiguous list.
4. Going into write mode, by clicking on Write, should immediately additionally display all preset slots which are empty, filled with EMPTY PRESET, without regards to two search column tags. E.,g. in a trivial case, if there are two preset files labelled 000-something.xiz, 001-something-else.xiz, these will be listed at the top, followed by 126 EMPTY PRESETs (banks contain 128 patches). The Execute Write should by ungreyed, and clicking on it (and only by clicking on it) will overwrite the slot currently selected in the preset column, with the tags from the two center columns set appropriately.
5. Possibly once a preset has been written the UI should automatically revert to Read mode, both as an indication that the write operation took place, and also to avoid inadvertent overwrite of another preset. Since Read mode does doesn't display EMPTY PRESET, this will cause the preset list to change its appearance, which might be confusing.

At the basis of this is the fact that banks are groups of up to 128 patches, with the patch number normally being part of the filename in the shape of a numerical prefix, however, it is also possible to have patch files without numerical prefixes. The code will have to handle these in a consistent way, for instance assigning them numbers in alphabetical order. Not sure either how to deal with two preset files having the same numerical prefix.

A future issue is how to change the number of a preset in a practical way.

[fundamental]

and the button should be greyed out or in Zyn terms have a diagonal strike

Feel free to deviate from that behavior. IIRC the UI mockup didn't show disabled states, so that was a hack when active/deactive elements were required.

filled with EMPTY PRESET, without regards to two search column tags.

Perhaps there should be an additional visual indicator to say the slot ID? The idealized version would likely be the slot number in the lower left with a more muted text color, though I can't automatically say that would look the best or be the most usable.

this will cause the preset list to change its appearance, which might be confusing.

Likely the best behavior to avoid writing the same patch to a bunch of places, though unfortunately the best affordances to convey what's happening to the user would be moderately complex animations.

So, one modification I'd propose is at least keeping some of the overwrite behavior. Namely when transitioning to the write mode, if there is a patch that right now the overwrite button would target automatically have that patch selected. That should allow for the "continue to update the patch I'm working on" workflow to stay sane while communicating to users a more clear set of expectations before overwritting an unexpected patch.

Otherwise, sounds like a reasonable enough plan.

[polluxsynth]

filled with EMPTY PRESET, without regards to two search column tags.

Perhaps there should be an additional visual indicator to say the slot ID? The idealized version would likely be the slot number in the lower left with a more muted text color, though I can't automatically say that would look the best or be the most usable.

Yes that would be good. As it is currently, the slot id is hidden so it's not at all obvious to the user that it even exists.

So, one modification I'd propose is at least keeping some of the overwrite behavior. Namely when transitioning to the write mode, if there is a patch that right now the overwrite button would target automatically have that patch selected. That should allow for the "continue to update the patch I'm working on" workflow to stay sane while communicating to users a more clear set of expectations before overwritting an unexpected patch.

Yes, certainly. Normally that would be the last read patch, so when transitioning from read to write mode, the same patch as was highlighted in read mode should remain highlighted in write mode I would say.

Ok, I'll see if I can make heads or tails of the code that governs all this and see what I can come up with.

[polluxsynth]

I've been fiddling about a bit, trying to figure out what works and what doesn't and what messages are actually sent etc.

One thing I've discovered is that write mode actually does work to some degree, i.e. clicking on a slot in write mode causes that slot to be overwritten ... well, not exactly, for some reason it's always the first slot in the selected bank that is overwritten. The fault is in the UI, as the /bank/save_to_slot overwrites the correct slot when initiated from oscprompt.

Occasionally, I've gotten two patches with the same slot number in the file name, i.e. 0001 . I'm not sure yet under which conditions this happens. Most often it doesn't. When I turn my back and focus on something else, it does. :-)

The UI is however very reluctant to update anything, which is why I initially thought nothing was being written. For instance, if overwriting the first slot with a different patch name, the new name is not shown until another bank has been selected, and then the original bank selected again. (Incidentally, this results in an error if trying to immediately load the saved slot, if the file name has changed, as it tries to load the original file name, not the overwritten one).

Similarly, in write mode, the unused slots are not shown as EMPTY PRESET until another bank has been selected and then the original selected again. Same thing with Rescan, which again doesn't seem to do anything, but changing to another bank causes the rescanned slots to be shown.

So, so far, the two underlying issues seem to be in the UI: Slot 0 is always selected for write, and it's reluctant to update the preset list when things change.

Other issues are consequences of this. One interesting one is that when no bank is selected, selecting a slot in write mode attempts to write to //0001-patch-name.xiz, which is (normally) unwritable. In practice, this sortof makes sense as you can't obviously write to a non-bank.

Another oddity is that the /bank/save_to_slot OSC message is not shown when typing /bank in oscprompt. I'm assuming there's something missing in the OSC code somewhere in the engine, but I haven't understood enough about how this works to see what is missing.

I think I'm going to try and fix the issues first before attempting to change anything.

[polluxsynth]

It seems a lot can be fixed by adding bank.doBank and in some cases bank.doRescan at various points in ZynBank.qml. For instance, after having done /bank/save_to_slot, the list of patches needs to be updated by doing bank.setBank. The thing that bothers me is that I'm guessing this must have worked flawlessly at one point, so how come it has broken down? Did some of the OSC actions actually do more than was immediately required of them at one point? I.e. if /bank/rescan would actually return an updated list, and not just do the rescan, that would explain why the rescan button only calls doRescan and not additionally doBank.

It's one of those "...but how did this ever work before?" moments which indicates that there's something I'm overlooking.

Or perhaps some refactoring of the engine Bank class was done without fully considering the UI consequences?

[fundamental]

The fault is in the UI, as the /bank/save_to_slot overwrites the correct slot when initiated from oscprompt.

Heh, guess that explains how I missed some of the bugs. Most of my quick testing is through oscprompt.

Occasionally, I've gotten two patches with the same slot number in the file name, i.e. 0001 .

Interesting. Bank write should trigger Bank::clearslot() which calls remove(). Not sure how that's happening unless the bank thinks that the slot is already clear.

The UI is however very reluctant to update anything, which is why I initially thought nothing was being written. For instance, if overwriting the first slot with a different patch name, the new name is not shown until another bank has been selected, and then the original bank selected again.

Is this in the multi-UI sense or the single-UI sense? Generally for the multi-UI case (e.g. oscprompt+fusion) there have to be /damage calls in the right places when multiple things are affected or otherwise just a few broadcasts. If it's just fusion there can be a few .refresh() style calls here and there to update components.

Similarly, in write mode, the unused slots are not shown as EMPTY PRESET until another bank has been selected and then the original selected again.

Guess I missed the preconditions for that behavior. There should be an on-click style handler for the read/write buttons which should handle the setup going into the query being sent to the backend. Must be something missing there.

Another oddity is that the /bank/save_to_slot OSC message is not shown when typing /bank in oscprompt. I'm assuming there's something missing in the OSC code somewhere in the engine, but I haven't understood enough about how this works to see what is missing.

Here's one of the fun weird bits of how the OSC API works. More or less there's realtime messages and non-realtime messages. The distinction is hidden from users as best as we can, but essentially there's different dispatching happening in the different threads. The reflection code happens on a version which is similar to the realtime dispatch (with some non-realtime bits hacked in). So, you'll find in MiddleWare.cpp the 'snoop ports' Those should include a few bank operations as well as global xiz/xmz saving/loading.

It seems a lot can be fixed by adding bank.doBank and in some cases bank.doRescan at various points in ZynBank.qml.

Sounds about right with where I'd expect the bugs to lurk.

Did some of the OSC actions actually do more than was immediately required of them at one point?

It's possible, as there are some times that OSC behavior gets written for the fusion GUI, is found to break the fltk GUI and then a compromise is made. I'm not seeing anything obvious with git log on MiddleWare.cpp and Bank.cpp, though there have been a lot of changes since bank search was added around '16.

Or perhaps some refactoring of the engine Bank class was done without fully considering the UI consequences?

Very possible. I try to review changes/PRs/etc when they're occurring, but it's easy to let a few bugs slip by.

[polluxsynth]

The fault is in the UI, as the /bank/save_to_slot overwrites the correct slot when initiated from oscprompt.

Heh, guess that explains how I missed some of the bugs. Most of my quick testing is through oscprompt.

Ah, good, yes that would definitely explain some of the holes in the current behavior.

I've noticed however that this issue seems to have been automatically fixed when I added more doBank calls to ZynBank.qml . I need to look into it more, but I'm wondering if it wasn't an update problem - the UI didn't request updates to the bank lists and so was using old data.

Occasionally, I've gotten two patches with the same slot number in the file name, i.e. 0001 .

Interesting. Bank write should trigger Bank::clearslot() which calls remove(). Not sure how that's happening unless the bank thinks that the slot is already clear.

In at least several of the cases, the filename has been something like 0001-.xiz, indicating it's the result of a state where the UI didn't actually know which file to write, so possibly a corner case somewhere.

The UI is however very reluctant to update anything, which is why I initially thought nothing was being written. For instance, if overwriting the first slot with a different patch name, the new name is not shown until another bank has been selected, and then the original bank selected again.

Is this in the multi-UI sense or the single-UI sense? Generally for the multi-UI case (e.g. oscprompt+fusion) there have to be /damage calls in the right places when multiple things are affected or otherwise just a few broadcasts. If it's just fusion there can be a few .refresh() style calls here and there to update components.

I'm not sure what you mean by multi-UI vs single-UI. In this particular case I was using fusion.

I haven't yet got my head around what /damage actually does or is used for. My gut feeling so far is that it is something that is emitted by the engine when things have changed in a way that might not be immediately obvious to the UI (fusion), and so it's some sort of 'dirty' message, saying for instance "the banks have changed; reload them to get updated".

I hadn't noted the refresh functions in some of the .qml files. It looks like the paradigm is use to update things shown in the UI as a result of something else changing, like for instance the reverb parameters when the reverb preset is changed. Actually in this case, ZynBank:doBank() seems to have this function.

BTW, what's with the () in the .qml files? Sometimes they are used in function calls, sometimes not. For instance, in ZynBank, SetColumn for bank_name contains a lambda which has { bank.doBank } without the (), whereas the rescan trigger button has a lambda with { bank.doRescan() } with the parentheses.

Another oddity is that the /bank/save_to_slot OSC message is not shown when typing /bank in oscprompt. I'm assuming there's something missing in the OSC code somewhere in the engine, but I haven't understood enough about how this works to see what is missing.

Here's one of the fun weird bits of how the OSC API works. More or less there's realtime messages and non-realtime messages. The distinction is hidden from users as best as we can, but essentially there's different dispatching happening in the different threads. The reflection code happens on a version which is similar to the realtime dispatch (with some non-realtime bits hacked in). So, you'll find in MiddleWare.cpp the 'snoop ports' Those should include a few bank operations as well as global xiz/xmz saving/loading.

So does this mean that it's expected behavior or that something is missing somewhere?

Or perhaps some refactoring of the engine Bank class was done without fully considering the UI consequences?

Very possible. I try to review changes/PRs/etc when they're occurring, but it's easy to let a few bugs slip by.

Testing doing oscprompt would certainly explain the current state of affairs.

BTW, is the fltk UI still supported? The question came up in the Zynthian forum, as there was criticism that fusion was consuming inordinate amounts of CPU on that platform, and the fltk UI might be more appropriate. My spontaneous reaction was that the reason might be the interaction between fusion and the engine, with continual UI updates for envelopes, VU-meters and the keyboard, and that a solution might be to have the updating frequency adjustable. But that's just a wild guess.

One final question, I've found one thing in the engine (Bank.cpp) in that when save_to_slot is executed, the db is not updated, so that subsequent /bank/search calls return the old state. It can be fixed on the UI side by calling /bank/rescan, but it seems more logical to me that save_to_slot() does db->scanBanks() before returning to keep the db consistent.

[fundamental]

the UI didn't request updates to the bank lists and so was using old data.

Unsurprising. The UI very aggressively caches information to keep the overall experience more responsive. Clunky as bits of the final layers may look I'm pretty happy with what's under the hood.

something like 0001-.xiz, indicating it's the result of a state where the UI didn't actually know which file to write, so possibly a corner case somewhere.

Possible, though I don't see a problem when looking at BankDb::processXiz() based on a source code reading. I could be totally missing it though.

I'm not sure what you mean by multi-UI vs single-UI.

The backend basically acts as a server with the UI being a client. You can have multiple clients bound to the same server, e.g. the fusion GUI, a fltk GUI, an oscprompt, etc can all simultaneously be connected and kept in sync using the messaging to the backend (server)

I haven't yet got my head around what /damage actually does or is used for.

It's the ugly but necessary "everything that starts with the damaged path has been updated and if the GUI has a cached copy of those values it needs to discard them and request a new up-to-date copy". Bit of a mouthful, but roughly equivalent to 2D graphics where a rectangular bounding box of pixels is declared to be out of date and the current render buffer is damaged within that region.

Sounds like you've got the gist for it though.

BTW, what's with the () in the .qml files?

The () is optional in Ruby when no ambiguities are introduced so there's some allowable variance:

class Foo
  def method_name()
    return :result
  end
end

f = Foo.new
puts f.method_name #result
puts f.method_name() #result
puts(f.method_name) #result
puts(f.method_name()) #result

I'm sure there's an official style guide, but usually I add () when I want to emphasize a method is getting called and otherwise omit where possible.

BTW, is the fltk UI still supported? The question came up in the Zynthian forum, as there was criticism that fusion was consuming inordinate amounts of CPU on that platform, and the fltk UI might be more appropriate.

Yep, the FLTK UI is still supported, but if there's a new feature I only ensure that the new feature is in the fusion GUI and that it doesn't break the FLTK GUI.

that a solution might be to have the updating frequency adjustable.

Already implemented. I forget the name of the parameter off the top of my head, but if you git grep getenv() in the fusion code you should see a few parameters for adjusting animation update rates. All of the animations should react accordingly to a different timestep.

One final question, I've found one thing in the engine (Bank.cpp) in that when save_to_slot is executed, the db is not updated, so that subsequent /bank/search calls return the old state. It can be fixed on the UI side by calling /bank/rescan, but it seems more logical to me that save_to_slot() does db->scanBanks() before returning to keep the db consistent.

Interesting. I guess that must be a regression I introduced with the Bank cache. Since saving to slots is a rare event I'd say it's fine to trigger a bank scan in those cases.

[polluxsynth]

the UI didn't request updates to the bank lists and so was using old data.

Unsurprising. The UI very aggressively caches information to keep the overall experience more responsive. Clunky as bits of the final layers may look I'm pretty happy with what's under the hood.

As well you should, I'm honestly really impressed by ease of defining the highest level of the UI, how easy it is to add a new parameter etc. It's not surprising that it gets complex when implementing things like patch writing, because that type of operation is a different paradigm and requires a different type of interaction (both with the user and with the engine) than the bulk of the editor.

I'm not sure what you mean by multi-UI vs single-UI.

The backend basically acts as a server with the UI being a client. You can have multiple clients bound to the same server, e.g. the fusion GUI, a fltk GUI, an oscprompt, etc can all simultaneously be connected and kept in sync using the messaging to the backend (server)

Got it. I was going to ask how valid a usecase that is, but I just thought of an example: running ZynAddSubFX in a Zynthian, with the Zynthian engine VNC enabled, while at the same time using a remote Zest to edit patches.

I'm thinking though, that in this specific case (i.e. the UI requesting updates to the bank list after writing to a slot), if the UI actually performing the write emits a /bank/blist after performing a write, then the resulting /bank/search_results message from the engine will be picked up by all clients woudln't it? Although I guess this is a special case, in that the UI performing the write "knows" what it expects in terms of change and can specifically request it.

I haven't yet got my head around what /damage actually does or is used for.

It's the ugly but necessary "everything that starts with the damaged path has been updated and if the GUI has a cached copy of those values it needs to discard them and request a new up-to-date copy". Bit of a mouthful, but roughly equivalent to 2D graphics where a rectangular bounding box of pixels is declared to be out of date and the current render buffer is damaged within that region.

Ok, I think I understand; more concretely, when the engine emits a damage message, is the UI expected to request updated information, e.g. send a new /bank/blist message in order to update the relevant structures? Because if so, the problem with the UI not showing the updated bank contents when writing to a new slot, for instance, is not due to lack of explicit /bank/blist calls when the UI knows it has written something and thus needs to get an update from the engine, but rather a failure to properly automagically request such updates upon receiving damage messages.

The () is optional in Ruby when no ambiguities are introduced so there's some allowable variance: ...

Right. Need to update my (nonexistant (read: need to take a crash course)) Ruby skills. Or even sheepishly admit that I didn't really know that the .qml files are actually Ruby, because of the apparent simplicity in defining the top layers of the UI, I was thinking it was some form of Ruby-inspired but purposely designed description language.

BTW, is the fltk UI still supported? The question came up in the Zynthian forum, as there was criticism that fusion was consuming inordinate amounts of CPU on that platform, and the fltk UI might be more appropriate.

Yep, the FLTK UI is still supported, but if there's a new feature I only ensure that the new feature is in the fusion GUI and that it doesn't break the FLTK GUI.

So, in practice, slowly running out of steam as new things get added to fusion then, but definitely not actively deprecated.

that a solution might be to have the updating frequency adjustable.

Already implemented. I forget the name of the parameter off the top of my head, but if you git grep getenv() in the fusion code you should see a few parameters for adjusting animation update rates. All of the animations should react accordingly to a different timestep.

Great, I'll have a look at that and see what difference it makes.

Possibly getting ahead of myself here, I'm wondering how feasible for the engine to return a success state after a write operation, which could then be displayed by the UI. Something like sending a message like /bank/write_success or /bank/write_failure, which could be picked up and displayed in the appopriate place (like the user_value line in fusion).

[fundamental]

e.g. send a new /bank/blist message in order to update the relevant structures?

Yeah, that's the general idea. If it's only one thing which is 'damaged' then the backend usually just broadcasts out the new value, but if it's a lot of things, then damage acts as a wider wildcard sort of "if you really need these things, then go get them yourself" message.

... then the resulting /bank/search_results message from the engine will be picked up by all clients woudln't it?

Yeah, this is a case where it's easier to get the UI proposing the write to just manually refresh() the search query. That would leave other secondary GUIs minorly out of sync, but not in a way that anyone (I think) would end up complaining.

Right. Need to update my (nonexistant (read: need to take a crash course)) Ruby skills. Or even sheepishly admit that I didn't really know that the .qml files are actually Ruby, because of the apparent simplicity in defining the top layers of the UI, I was thinking it was some form of Ruby-inspired but purposely designed description language.

The summary is that Qt defined the core concepts of QML. Qt doesn't embed into plugins well and also the implementation of methods in QML are in javascript with some clunky linkages to C++. A partial reimplementation is needed to fix the plugin embedding problem and in terms of scriptable languages that a) I know b) I like and c) think weren't esoteric enough to drive away contributions.

So, what is a QML file in terms of the zest stuff? Well, it's basically a really fancy way of just defining a single ruby class. All the callbacks/function/methods are plain old ruby code and the nested instances of objects are turned into code run in the class's initializer. Any methods defined on subclasses are monkey patched on and parameters are turned into a reactive programming setup where changes propagate through callbacks based upon learned dependencies. It's a bit weird, but you can see all of the classes which get spat out of the system in the fcache.rb file which you likely have seen a few exceptions appear from.

Overall ruby is pretty good for some DSL building work.

So, in practice, slowly running out of steam as new things get added to fusion then, but definitely not actively deprecated.

Yeah, basically. There's some users that will certainly prefer the older GUI out of either familiarity or in terms of information density. So, I'm not trying to boot anyone off of it, but if we're not going to favor the new GUI, then why make it in the first place? :p

Possibly getting ahead of myself here, I'm wondering how feasible for the engine to return a success state after a write operation, which could then be displayed by the UI.

Pretty easy. I'd say having a more generic /user_message:s response sent out would make sense. The idea sounds familiar, so there might already be one of those in place? I'm 70% sure there's an /alert:s which the FLTK GUI does something with, but the fusion GUI ignores (missing feature, not me intentionally avoiding it).

[polluxsynth]

Merged in https://github.com/zynaddsubfx/zynaddsubfx/pull/181 and https://github.com/mruby-zest/mruby-zest-build/pull/75 .