christofmuc / KnobKraft-orm

The KnobKraft Orm - The free modern cross-platform MIDI Sysex Librarian
GNU Affero General Public License v3.0
205 stars 27 forks source link

Patch numbers in the Library don't correspond to the ones in the patch (for non consecutive patch numbers in dump - e.g. Behringer Pro 800) #259

Open Andy2No opened 1 year ago

Andy2No commented 1 year ago

I started trying to make a partial adaptation for the Behringer Pro 800, initially intended to allow patches to be named, which can't be done from the synth. Most of the sysex details have been kept secret, so far, so it can't do everything that an adaptation should. Patches can only be exported from the synth by asking for a full dump, from the front panel Global Settings menu.

I found that the patch numbers shown in the Library tab don't correspond to the ones in the patch, and can't because friendlyProgramName(program) is only ever passed an arbitrary number during the import from file process, which is just a count of where the import is up to, not actually a real patch number. Blank / init patches are not included in the dump, so the count is wrong as soon as there's a gap.

What I would need, to make this work, is a way of being passed the whole patch (message), not just the potentially wrong number, so the python code can extract the actual patch number. Perhaps the best way to approach this would be to allow an optional function to do that, which is called in preference to friendlyProgramName() if defined, e.g.

def friendlyProgramNameFromPatch(message)

I started this as a discussion, and there are more details there - #258

christofmuc commented 1 year ago

I have to check - normally, when you have implemented numberFromDump() it should take precedence over the running number during an import. Can you post your adaptation and maybe a dump file so I can give it a try?

christofmuc commented 1 year ago

I think you're on to something bigger here :-)

Some intermediate results:

What different type of numbers do we have?

We have different import methods, which count differently as well.

To fix this mess, we need to find all places where the PatchHolder class is created (this is the class that determines what is stored in the database).

Andy2No commented 1 year ago

Thanks. I only just read that, as I was about to add the dump and adaptation you asked for... Maybe I should put those in a discussion thread instead, since it's a universal behaviour, not specific to what I've done - just to any situation where there isn't a full set of consecutively numbered patches from the start up to the final one in a dump.

I think this is also probably going to apply to the Waldorf Pulse II, if I ever get any further with that one. At best, I expect patches missing where the init ones are. On mine that's a block of 80 or 90, towards the end of the range of patch numbers, with some non-blank ones at the high end.

I'll start a new discussion thread for the Pro-800 adaptation (such as it is) soon - I'm a bit tired now.

christofmuc commented 1 year ago

Wasp's nest. But I finally need to clean this up:

  1. The old C++ adaptations (well, before there were adaptations actually) did split the patch data that is location independent from the patch location, So e.g. for the Virus in the database there are only the parameter values, plus a position determined at import time. The Orm generates edit buffers or program dumps from these parameter values as it needs them, which is now only when a patch is stored in a bank at a certain position.
  2. The Python adaptation are "simpler" and rather store the data from the synth as they received it, as one or more MIDI messages. This naturally includes a program position. This can lead to ambiguity when putting a patch in a Synth bank, because now there are two numbers.
  3. To add to the complexity, we also have a running number used during the import. This was mainly built for synths that have no stored patch number, but as you discovered this currently takes precedence, which is a bug.

The fun really starts when looking at the names as well:

  1. When the synth has no name storage, it calculates the name via friendlyProgramName. This is based on the patch number. But when I move e.g. Patch 371 from the Matrix 1000 into a new bank 2 at position 11, it becomes Patch 211.
  2. The number is also used to initialize a patch name for those synths, but the name can be changed. So you could change the name of the patch 211 back to 371 (as text).
  3. When the synth has name storage as well, the program number is not used for the name. Still you can switch the UI to display patch numbers. Patch numbers are generally extracted via numberFromDump() which is part of the ProgramDumpCapability

What is the expected behavior?

  1. The stored program number is shown everywhere, if it is available. In case a patch is placed in a bank, so new number should be shown.
  2. A patch can actually have multiple numbers, but those are related to its position in a list. But not all lists have positions: a) Imports. Currently imports are implemented differently from user lists, and the original idea of retaining all imports in which a patch had been encountered is not fully implemented. Once imports are migrated into lists, we could easily add a patch into all imports in which it had been discovered. But that is a different feature. b) User lists. User lists cannot be sent to the synth, and they are made for collecting patches from muliple synths (e.g. to use in a song). So a position in a list needs to be stored, but it is not a storage place. c) Finally, synth and user banks. Here the position in the list is the same as the program place. The complexity here is that not all program numbers also contain the bank, some synths only store a position inside a bank, but the bank is unknown by the patch.

Uff.

Andy2No commented 1 year ago

I'd suggest let the adaptation decide how to display it, whenever that's an option. If it's in a list which doesn't display a number, then there's no need to ask the adaptation what to do, but otherwise I think it's best to pass the full "message" - as much of the database entry as an adaptation function can be shown, plus a context flag, to determine different cases.

For example, if friendlyProgramName() was passed the same "message" as numberFromDump(message), I would be able to show the actual bank and program slot number as appropriate for the synth - or any other. If it needs to display different things in different circumstances, that's where the context flag comes in - e.g. an enumerated type (or Python equivalent) or a short string constant or single character.

If the adaptation doesn't care about the context, then maybe it should be allowed to always show the same thing, if it wants to - if "message" is a program dump or an edit buffer dump then the adaptation will know what it wants to show, with the option of changing that behaviour based on the context.

Since existing adaptations already use friendlyProgramName(program), keep that as it is, but keep it optional and add another optional function with more flexibility, e.g.

def flexibleProgramName(program, message, context)

As for some synths having a program number within a bank, but no bank name / number, I think it's better for the adaptation to be able to decide what to do for that particular synth - e.g. just show the program number, without a bank.

So, to summarise, give the adaptation programmer the ability to do what he or she wants, for that particular adaptation, where appropriate. If there are circumstances where the Orm needs to override that, then fair enough, do that instead.

christofmuc commented 1 year ago

@Andy2No Good idea with the new program function taking the message. This is a bigger change - I tried to fix the original problem that for adaptations numberFromDump() was never called in the 2.0.6 release. Please give it a spin and see if it makes a difference for you!

Andy2No commented 1 year ago

Fantastic! It worked first time, without any modification to my adaptation code :)

The ones that have blank names here are supposed to be like that - that's how an edited patch looks when you import the dump. All the patches are correctly numbered. A0-99 are the stock ones, but can all be overwritten. I might add a leading zero to the single digit ones, to match the display on the synth, otherwise it does all it needs to do, for now, for that synth. Renaming still works too.

Knobkraft Orm 2 0 6 test