zynthian / zynthian-issue-tracking

Centralized Issue Tracking for Zynthian Project
https://github.com/orgs/zynthian/projects/1
11 stars 3 forks source link

Do not restart Pianoteq when changing instrument #803

Closed riban-bw closed 1 year ago

riban-bw commented 1 year ago

Is your feature request related to a problem? Please describe. When changing instrument, Pianoteq is restarted which takes a long time and is prone to xruns.

Describe the solution you'd like It would be advantageous to change preset and instrument without the need to restart the engine.

Additional context It may be that Painoteq is being restarted because it was the only way to invoke a different engine. Maybe use of the JSON-RPC api (introduced in 7.5) would provide a (partial?) answer. This may need some logic to handle versions before 7.5.

jofemodo commented 1 year ago

In the old 6.0 times, the only way of changing remotely the preset was using Pianoteq's MIDI mappings + Prog.Change. This limited to 127 the number of presets that can be accessed without restarting. On every start, a new MIDI mapping is generated with the 127 presets near the current one (+-64). Of course, this is highly suboptimal, but we have no other option in the time this was implemented. Moving to the JSON-RPC API should be high priority after releasing stable.

riban-bw commented 1 year ago

I've been looking at using the JSON-RPC interface for Pianoteq. Some observations (so we don't forget):

def rpc(method, params=None, id=0): if params is None: params=[] payload = { "method": method, "params": params, "jsonrpc": "2.0", "id": id} try: result=requests.post(url, json=payload).json() except: return None if 'result' in result: return result['result'] return None

rpc('getInfo')

riban-bw commented 1 year ago

I have created pianoteq_rpcjson_api git branch and committed a partially working implementation. It looks at the piantoeq version installed and chooses the old (unchanged) or new engine class. (I think it cleaner to separate the classes as they work very differently.) I have implemented most of the RPC calls and preset selection works well with no restarts and fairly prompt change between instruments (although sustained notes can interfere with the patch change).

As mentioned above there are issues with the RPC-JSON implementation in 7.5.4 so we may need to wait until a later release of Pianoteq before enabling all features but this is a good start. Pianoteq is looking like a more integrated instrument.

linuxnow commented 1 year ago

I was about to ask for tips to contribute this work but you've been way too fast.

I have tested it and Pianoteq is not restarted anymore, I've tested several instruments from different packs, so this seems to be working. Thank you very much!!!

There are some small things:

-First issue: the saved subsnapshots don't work anymore because the name of the presets is different. This is no problem, I've reprogrammed them all.

-Second issue: dynamic is not mapped, it works in the staging-2210 branch but not now

-Third issue: this is important for the user experience. In the staging-2210 version, the original preset value for dynamic is not mapped to the zynthian-ui values, so when you save a subsnapshot you always change the dynamic value to 64. This is an error because you lose the original value which, supposedly, should be the best and the one you keep unless you decide to change it. In this patch, as you cannot change them, they are not saved, so you preserve the preset values. In any case, the values should be correctly copied from the original preset to the ui value and then correctly kept in the subsnapshot.

-Fourth issue: if you change the output value via VNC and create a user preset, when you load it, the output value is not preserved. This works in staging-2010

-Fifth issue: volume is not mapped to the ui either

riban-bw commented 1 year ago

Odd! I posted a response earlier but it didn't make it here!

  1. For some reason we call pianoteq by its specific name, e.g. stage, demo, etc. I think this isn't needed and could be rationalised. The presets are saved as native presets now so things may have changed, specifically they are saved to pianoteq's "user" bank. I will take a look at how to handle migration of presets and snapshots.
  2. I had not implemented controllers but have now.
  3. This should be resolved by new implementation of controllers.
  4. Odd! I will take a look.
  5. This should be resolved by new implementation of controllers.
riban-bw commented 1 year ago

I fixed 4. Pianoteq saves presets in banks. Its native presets are stored in (what appears to be a protected) bank '', i.e. without a name. Zynthian now uses the bank and preset name to identify the preset and stores any presets created by Zynthian in a bank called, "Zynthian". Presets saved in Pianoteq's native GUI should appear in Zynthian and load correctly now. Note that there is not a mechanism to update Zynthian UI with values changed in Pianoteq GUI. This is subject of another ticket for all engines.

riban-bw commented 1 year ago

I have observed that Pianoteq does not seem to cache its instruments. The first time you load a preset for an instrument it is loaded with a short delay (maybe a few seconds). The next time a preset is recalled for that instrument it is fast. I don't know the resource usage in loading instruments. Maybe they could be preloaded / cached which would increase initial startup time but make access to any preset fast. I also don't know if instruments get unloaded after a period of inactivity.

linuxnow commented 1 year ago

Wow, I have no words. Thank you very much!

I have tested the changes and everything works as expected, everything is fixed. -Pianoteq user presets now work, although you don't really need them because you can fine tune everything from zynthian. -original preset values are correctly loaded and we don't overwrite them, which makes the user experience a lot better, everybody had a modified dynamic set to 64 which made it sound awkward. -you can change any parameter, save it in a subsnapshot and it's rightfully recalled. -not restarting pianoteq makes the experience fast and usabla on stage

Just a little issue that I've noticed: -the last parameter that you can access through the zynthian interface is dynamic . You can change it using the controller that goes from 0 to 1 in 1/100th steps. But the values are discrete (mono, stereo, binaural....). If you don't have access to the VNC interface you can't know what you are changing, although it works.

It would be nice to be able to read the discrete list and, somehow, display it in the zynthian's ui.

linuxnow commented 1 year ago

And please pull the branch into stable, the user experience is incomparably better :))

riban-bw commented 1 year ago

A little empirical experimentation shows each instrument loaded seems to take a few MB so loading all the KIViR instruments, Bells and Carillons and the instruments from two instrument packs (Steinway D & Electric Pianos) adds about 30MB and about 8% extra CPU usage when idle. It also seems to take a little longer to load a preset from a previously loaded but recently unused instrument but still less than a second. It may be worth adding an option to cache instruments or maybe add some intelligence to cache the instruments within a snapshot (ZS3).

Currently I have just used the normalised values for all parameters, i.e. every parameter appears as a number between 0..1. This is sub-optimal and needs resolving. I will take a look at improving that. The API offers the ability to get/set the normalised value or the textual representation but does not offer the ability to retrieve the enumerated textual values so this will require some manual empirical work. I think that might be the next thing to resolve.

This won't be merged into stable for a while. The API is unstable - not finalised and capable of crashing Pianoteq (and Zynthian - I have had a reboot!). The implementation requires more work and full testing. It is only supported by Pianoteq versions 7.5 and above. I wonder whether we should deprecate support for V6 at some point. It may be appropriate (from a Zynthian support effort point of view) to only support the RPC-JSON API but I suspect there are many users with a V6 license who may not want to pay to upgrade. A business approach would be to say they can stick with the last version of Zynthian that supports V6.

linuxnow commented 1 year ago

I've tested it cycling through most of the instruments and presets in Steinway D, Electric Pianos and KIViR, using subsnapshots for fast changes, and I haven't had a single glitch. If you can guide me on how to run formal tests I would volunteer for the task. Meanwhile I intend to use it and I would report any issues that might arise,

Thanks for the nice work!

riban-bw commented 1 year ago

I have added the discrete values (including the awkward difference in Output Mode list for different instruments). I have also enabled default MIDI CC mapping for sustain (type) controllers.

I think that is all done as far as I can.

Formal testing would be good! If only we had someone with the time, experience and enthusiasm to write such tests. It has been attempted several times before (I have done it a couple of times) but requires continual effort to keep the test scripts and plans up to date. My preference is to document the product and test against the documentation. If something is not documented a user may file an issue that they don't know how to do something and it gets added to the docs. The product should behave as documented. If a feature is not documented it is not supported.

riban-bw commented 1 year ago

ZS3 does not work! It looks like only the parameters are applied and the preset is not recalled. This is bad! [Edit] It is working fine now! Must have been finger trouble.

linuxnow commented 1 year ago

I'm reinstalling from scratch the stable image to make a thorough test again with this branch enabled. I'll let you know ASAP

linuxnow commented 1 year ago

I have added the discrete values (including the awkward difference in Output Mode list for different instruments). I have also enabled default MIDI CC mapping for sustain (type) controllers.

AFAICT everything works. I've been updatiing my ZS3 subsnapshots modifying parameters and they are correctly kept and duely recalled. I can see that many of the parameters now have on/off values and the enumerated list, fine!

I think that is all done as far as I can.

* The potential to crash has been reported but shouldn't happen with the wrapper I have written which avoids sending invalid RPC-JSON.

It hasn't crashed a single time, in my case,

* The lack of delete in the API actually seems to match the lack of this feature in the application. I can't find where you can delete presets in the native GUI!

You are prompted for a file name when creating personal presets in Pianoteq, problably the only way is to remove the file.

* I haven't (fully) tested saving and restoring presets, snapshots and ZS3. ZS3 is one of the drivers for this ticket so it should be tested.

I'll keep on testing it a few more days, but until now all the params are saved and restored in ZS3 and in spanpshots.

Formal testing would be good! If only we had someone with the time, experience and enthusiasm to write such tests. It has been attempted several times before (I have done it a couple of times) but requires continual effort to keep the test scripts and plans up to date. My preference is to document the product and test against the documentation. If something is not documented a user may file an issue that they don't know how to do something and it gets added to the docs. The product should behave as documented. If a feature is not documented it is not supported.

I know that writing tests is boring, I wouldn't know how to start!!

Thanks again for the great work, I'0m gong to stick to this branch until merged!

riban-bw commented 1 year ago

I implemented delete/rename of user presets and show the user presets at top of list with separator indicating user/factory presets. (Separator only shown if user presets exist for an instrument.)

Zynthian now uses bank "My Presets" (as it used to) and not "Zynthian" as I implemented in a recent change. This is because the backup/restore/upload/download of presets is configured to use this bank. Presets in other banks cannot be modified by Zynthian so will need to be manipulated with the native UI.

Upstream have said they will fix the crashing and implement preset delete in the API in a future version but I have worked around this by direct access to filesystem. I think we have a fully featured integration now.

If no more issues arise I plan to merge with testing branch. I wll discuss with @jofemodo how and when we update stable.

linuxnow commented 1 year ago

I can see the User Presets and Factory Presets separators. And I can see the presets that I gave created. So everything works as expected.

There's one small glitch, delete does not seem to work, I can't get rid of the preset I've created.

riban-bw commented 1 year ago

That is by design - sorry! The previously created presets were put in bank "Zynthian". We have not gone back to using the default bank "My Presets". You will need to use the Pianoteq UI to move those presets to bank "My Presets":

riban-bw commented 1 year ago

I've improved the integration so now you can manipulate presets saved in any user bank, not just "My Presets". This should allow manipulation of the presets added in the Pianoteq UI or to the Zynthian bank that we used for a short time.

linuxnow commented 1 year ago

Tested, it works flawlessly. After a rename you get the new name in the VNC interface. Love it!

jofemodo commented 1 year ago

Closing this!