Let SurgeSynthProcessor::getProgramName actually return the name of the currently loaded patch

[ZonderP]

It currently returns an empty string (at least when SURGE_JUCE_PRESETS is undefined which by default is the case in source code of Surge XT 1.0.0)

Some hosts use 'effGetProgramName' to be able to display the current patch name. E.g. energyXT.

I would like that Surge XT behaves like a well implemented VST and react to 'effGetProgramName' (which in Juce causes SurgeSynthProcessor::getProgramName to be called, if I'm not totally wrong) In it's current state, it should return the name of the actual loaded patch if called with index 0.

Since Surge patch names may be rather long, it might be necessary to cut the end part of the patch name to fit into the length which the VST2 specification allows.

[mkruselj]

However, we don't support VST2, even if you can build it yourself. Pull request is welcome if you want to tackle this particular issue, though!

[ZonderP]

Okay, so for VST3 this function isn't even used... (I've never bothered with VST3 so far.) Yep, might try to create a MR, if I do not have to dig too deep into the code to find out how to retrieve the name of the currently loaded patch. (Any hint welcome ;-) ) For a MR, it would be best, if I have most actual commit in master in my clone?

[baconpaul]

The reason this isn’t implemented is I had all sorts of state and synch problems with the au and vst3. You can see a pretty complete implementation is ifdefed out for preset changes but it didn’t work properly in logic

returning the name seems pretty harmless but the name invalidation is something I couldn’t figure out. If you can that’s great

and we use standard GitHub pr mechanism. There’s a git howto in doc which explains it

thanks!

[ZonderP]

Since @mkruselj said 'we don't support VST2', I thought this function ('SurgeSynthProcessor::getProgramName') would only be called for VST2 . If that's not the case, then I guess I can't do anything here, because I have no means to test au and other formats. But in case it is called only for VST2, then it should really be very simple (unless I miss something, since I didn't yet really check the Surge code...) Surge reports - for VST2 at least -, that it would support 1 program, therefore it should just return the name of the actual loaded patch when this function is called with index 0 (for all other indices it just can return an empty string) Juce will even properly handle too long names in 'handleGetProgramName' in the VST2 wrapper. If Surge would have a 'standard' banks handling (as initially intended for VST2) with an fixed amount of patches, then it would need to report 'NumPrograms' as 64/128 or whatever and then it would need to return the correct patch name for any supported index < NumPrograms, but since it only has one program, I think the actual loaded program can be assumed to be at index 0 and thus its name can be returned. It's a bit late here, and I programmed quite a bit already, thus bare with me, in case the above was complete nonsense...

[baconpaul]

It is really simple to make it return the patch name when called

I couldn’t figure out how to invalidate the patch name when the patch changes

If you can make it do that with the juce api I can rest the au. I never could. Moreover it would stay stale in several daws and I thought empty was better than wrong

[baconpaul]

But I would love to fix it if someone can figure that out! Just it is more than a strcpy of synth storage getpatch getname

[ZonderP]

Ah, now I see what you mean with invalidate the patch name. That's not something this function has to do. This function should always just return the name of a patch at the passed in index at the time when it gets called by the host. A host may call it after it loaded the instance and probably should also do so, when it was told by the VST, that something changed. In Juce there is a method to inform a host about a possible change. Due to 'bad' VST2 specification, this method isn't very detailed, but usually hosts call all kind of VST callbacks, in case they've been told about a change in a VST. I think the function Surge would need to use, after the patch got changed internally, should be 'AudioProcessor::updateHostDisplay' - which now since some versions of Juce is a little bit more advanced, since it now allows to specify 'programChanged'. This is how I understand this whole thing is supposed to work. (Edit: If hosts do not do what they are supposed to do when the receive 'audioMasterUpdateDisplay', then this isn't a problem of Surge or any other VST. Just returning the actual patch name at a given position is all a VST can - and should - do to conform to the VST2 specification.)

[baconpaul]

Yeah that sounds right, the editor has to force thst api call when the patch change is detected or some such and the. We run around daws and see if it works.

[ZonderP]

Just not to speak of different things. The editor knows, when a user selected a patch and then - after it has loaded that patch - should (must) call 'updateHostDisplay'. That's all. And 'getProgrammName' should just return the name of that loaded patch when called with index 0. That's all. To let 'getProgrammName' return that name is never bad. What hosts do with the notification 'audioMasterUpdateDisplay' is up to them, and if they - for whatever reason - do not send 'effGetProgramName' to the VST, then there is nothing a VST can do and that host will not correctly display the name of the newly loaded patch. But at least for hosts, which react properly by sending 'effGetProgramName' to the VST from which they received 'audioMasterUpdateDisplay', they will be able to display that actual patch name.

[baconpaul]

Yup sounds right. And I guess you have to call the update from the right thread. But I’m assuming that’s the main thread not the audio thread.

[ZonderP]

Yep, I think I also would prefer the main thread. In my project, if a user renames a patch, then I use 'AudioProcess::updateHostDisplay' from the main thread and at least energyXT seems to handle it and updates its display of that changed patch name.

[baconpaul]

Ahh now I remember. Bitwig throws a (valid) exception if you all updateHostDisplay from a thread other than the audio thread. OK but we have a mechanism to do that. Hold on.

[ZonderP]

Oh, really! It's not that easy to please all hosts! But Bitwig for sure is one you want to support properly. Need to check that for my project then. From time to time I tried it in Bitwig, but maybe I didn't test that scenario... Thanks for the tip.

[baconpaul]

Yup and now I remember the problem

name.patch.zip

Apply this patch and when the patch changes update will get called. The getProgramName returns the name. Here's the behavior I see

1. Reaper VST3 - name doesn't change ever, because it's not in the factor list.
2. Logic Pro - name changes sometimes. If you go quickly it doesn't though
3. Bitwig - doesn't display patch names, but param name changes make it through

so basically "it doesn't really work" which is why I didn't merge it

[ZonderP]

Thanks, I will try that out, when I find time and report back how it behaves in energyXT and maybe some other hosts too.

My personal opinion on this is, that it also "doesn't really work" if Surge reports an empty patch name for hosts that would otherwise deal correctly with it and display that patch name somewhere on their GUI.

Also I know tons of VSTs which are written properly regarding this callback for the program name and just report back the name of the patch, thus I assume they all do not really work properly in those hosts (Logic Pro, ...) which somehow don't handle this right. (I'm writing a VST wrapper which works best with VSTs which implement that handful of 'required' VST specific functions and therefore do tests with many, many free VSTs). If I was to write a VST, I would implement those few functions that make it a real 'VST' instead of just letting it be an audio processor. Don't get me wrong - I think Surge XT is fabulous, therefore I care! And if then users would say, hey your VST shows the wrong patch name in HostXYZ, I then would just answer: Go ask HostXYZ devs to make this work, my VST does all it can do. (You know that when Bitwig was released, that there was no possibility to load and save programs and banks for VSTs, although this is 'originally' the duty of a host when it wants to properly support VST - I guess this might be one reason, why Surge and others now have their own patch handling, apart from obvious limitations with VST's original approach).

At the end, it's your decision which "doesn't really work" way you want to go.

[baconpaul]

We can't make a VST2 anyway and distributing it violates a variety of licenses, as you know, so VST2 isn't really a format we can or do support.

The fact that it doesn't work with VST3 and AU in our mainline hosts is, indeed, something I would like to solve. I would start by asking the juce team what I'm doing wrong, honestly. It really seems like these APIs should work in juce.

We used to be hand rolled wrappers. It was terrible.

But also we happily take pull requests for any improvements! But my view has been I would rather show nothing than show something wrong in the hosts most of our users use. So fixing this really is fixing the full built in preset advertising mechanism.

[ZonderP]

Hmmm, maybe there might be a bug in Juce in these APIs, but I would rather guess, that it is more likely to be a problem of the hosts. I know VSTs written with Juce where this 'AudioProcess::updateHostDisplay -> 'AudioProcess::getProgramName' just seems to work - in hosts where this also works for VSTs not written with Juce.

'So fixing this really is fixing the full built in preset advertising mechanism.' That would really be fantastic! (Although that possibly would only work for VST3 and maybe other formats but unlikely for VST2?)

I'll see, how it goes with your patch and if it works for me, I'll just apply it to my 'private' Surge XT VST2 builds. But still will make a report, how it works in energyXT and others.

P.S.: Just came to my mind: I think one could also use '#if JUCE_PLUGINHOST_VST' in 'getProgrammName' and only then return the patch name, otherwise still return an empty string?

[baconpaul]

That ifdef is not active at that compile time (the same processor links to all the plugins) but the dynamic wrapper type would be there and yea you could if it. Heck you could even only make it work in ext

[ZonderP]

Hmmm, wanted to report back about how it is going with the name.patch applied, but have some strange issue now with Surge XT 1.0.1. First need to find out if it has something to do with the name.patch applied or with the upgrade from 1.0.0 to 1.0.1. So far I used 1.0.0 build, and the Surge XT built in Patch Browser worked nicely whatever I did. But now with patched 1.0.1 build, the Juce-menu-based Patch Browser very soon stops to work properly and I cannot select a patch from the menu then. Need to build either 1.0.0 with the name.patch applied and/or 1.0.1 without that name.patch applied. In energyXT this is...

[baconpaul]

The 10 to 101 changes didn’t change patch loading fyi.

[baconpaul]

Oh there was one change to use c++17 std thread in one spot where we used to use windows threads directly - are you building with an old runtime or something? That could do it.

[ZonderP]

Thanks for the hint. I believe I built with a new runtime and also that it should be the same one I used when I first built 1.0.0, which I didn't do too long ago (maybe a month or so). Strange thing is, that it's really hard to reproduce and find a pattern when the patch browser menu stops working. Since I have a similar problem with my project from time to time with the Juce menu, might be related and might be it only occurs in energyXT. For my project the observation so far was that it probably has something to do with how the editor window is hosted, but so far I didn't find out the exact reason. I try now hard to reproduce with the 1.0.0 build, because I suspect, that the buggy behavior might also be there if I try long enough...

[ZonderP]

Yep, just managed to get the same buggy behavior also with the 1.0.0 build without the name.patch applied. So all good here! I wouldn't worry too much about it, because I think it might only happen in energyXT (hopefully, but I guess otherwise people using other hosts would already have reported.) IIRC, for me the menu-problem for my project started to show up in energyXT, when I upgraded to Juce 6.1.2 (which is also the version Surge is currently using) Edit: As soon as the Patch Browser bug occurs, also the 'Menu' menu doesn't work anymore... (All menus stop to work properly) Edit 2: And funny thing is, I can even close the Surge XT editor and open it again, but the menus still don't work. (Exactly the same as for my project!)

[mkruselj]

Yup that def sounds like eXT being a bit of an a-hole. Maybe?

Are you saying it doesn't happen pre-JUCE 6.1.2?

[ZonderP]

Yes. but only if I really remember correctly... I'm working on my project since years and used Juce 3.2.0 for a long time, but finally managed to go to Juce 6.x nearly 1 year ago. I had to do quite some work, when I moved to Juce 6.0.7 and when I finally got it running, I can't remember that I saw this menu problem. I used Juce 6.0.7 roughly 1/2 year between May and November last year, mostly just testing in energyXT. Then I switched to Juce 6.1.2 in November and from that moment on I saw this behavior. I investigated a bit and at least found out that there were some fixes in Juce related to menu between 6.0.7 and 6.1.2... Stopped investigating at some point, because I really couldn't find out what is going on under the hood and since it only occurs in energyXT. Though I really would like, that my stuff also works in energyXT. Maybe later, when I'm hopefully closer to a releasable version, I might try to upgrade to then newest Juce and hope that this issue just gets fixed by doing so. If not, maybe I'll try to involve Juce devs...

[ZonderP]

Sorry, I did NOT remember correctly. I found an old version of my DLL, which was built with Juce 6.0.7 and tried it out just now. That same menu related bug was also already there with 6.0.7 - but as said - so far I could only reproduce it in energyXT. And I am sure, that I didn't have this problem with Juce 3.2.0. Could also be that I made to many changes to make my project work with Juce 6.x, but in this case I doubt that this is the reason, because in other hosts I cannot reproduce this issue and also that now Surge menu has the same problem in energyXT makes me think it's a combination of Juce menu implementation and how energyXT hosts the VST editor (or something similar).

[ZonderP]

Something completely different and sorry if I post it here, but it's the fastest way to tell you: The download link 'https://github.com/surge-synthesizer/releases-xt/releases/download/1.0.1/surge-xt-win32-1.0.1-setup-x86.exe' offered on 'https://surge-synthesizer.github.io/downloads' to download 'Windows 32-bit VST3' doesn't work. Edit: I think the correct link would be: 'https://github.com/surge-synthesizer/releases-xt/releases/download/1.0.1/surge-xt-win32-1.0.1-setup.exe' (This one worked for me)

[baconpaul]

Thanks. Fixed by hand and added to #5784

guess that tells us a lot about 32 bit demand. Maybe we should switch it to self build only at this point.

[ZonderP]

Okay, tested Surge XT 1.0.1 VST2 32 bit with the name.patch applied in some hosts. For me it looks very good :+1:

SAVIHost (32 bit): Displays (and updates properly) the currently selected patch in its Window's title bar and also when you choose 'PlugIn' menu -> 'Name program'

VSTHost (32 bit, v1.54): Displays (and updates properly) the currently selected patch in the title bar of the windows which hosts Surge XT's editor.

MuLab (32 bit, v8.6.24): Displays (and updates properly) the currently selected patch in the 'header' of the window which hosts Surge XT's editor.

Reaper (32 bit, v6.47): Displays (and updates properly) the currently selected patch in dropdown displayed on 'header' of the window which hosts Surge XT's editor.

Cakewalk (64 bit, v2021.12.102): Displays (and updates properly) the currently selected patch in dropdown displayed on 'header' of the window which hosts Surge XT's editor.

energyXT (32 bit, v3.0): Displays (but does not update properly) the currently selected patch in dropdown displayed on 'header' of the window which hosts Surge XT's editor and in another location on its GUI. But the problem with this updating of the display of the patch name also happens equally for other VSTs. One has to move over and click or do some other action on the energyXT GUI so that the GUI then updates the display of the actual patch name, but then it will correctly display the actual selected one.

For Cantabile (32 bit, v3.0.3694), Podium Free (32 bit, v3.2.1), Waveform (64 bit, 11.5.17) and Bitwig Studio (64 bit, v4.1.6): I couldn't find a GUI item anywhere which would display the current patch name, but patched Surge XT 1.0.1 seemed to behave well with minimal testing...

[baconpaul]

These are all tests with a VST2 build?

[ZonderP]

Yepl, only VST2. I've absolutely no clue about VST3, never used, don't know what additional features it offers, don't know anything about hosts which support it and in which way. Even absolutely don't know, how bank or patch handling is done with VST3. Okay, I know that Reaper, Studio, Cakewalk and probably also a few others of the above support it, but to be honest, I more or less only know how to start those DAWs and then even have a hard time to find out, how to load a VST2 and display the GUI for it.

In my opinion the name.patch is very good for VST2 and in all tested hosts caused no problems but instead now does what it should do for VST2 - and since you mentioned above 'but the dynamic wrapper type would be there and yea you could if it', I think that one could, to be on the save side for VST3 and au, additionally just do this (== make it work this way for VST2 only).

[mkruselj]

I tested the patch in Reaper with VST3. It doesn't change the preset name straight away, but if you close and reopen the plugin GUI, or select the preset from the preset menu it updates. This one is on Reaper to fix, I would say.

[ZonderP]

I also would say, that this is more a Reaper issue - seems to be similar to the one in energyXT with the VST2, where it more or less just seems to be a repaint issue of the affected control.

[baconpaul]

Well we can apply the patch and chase the daw makers Or apply the patch with an if vst2 If press merge on either. We can always back it out if it breaks We know it’s inconsistent in logic thoigh which is a bummer

[baconpaul]

Ahh now I remember the problem with logic and this is NOT au mergable.

And I have lots a bit of music I was working on perhaps.

Basically the name being non-empty triggers some change at patch load time I could never debug and it reset one of my tracks to init sine.

So if we want to merge this we definitely need to condition it to VST2 and 3 only for now.

[baconpaul]

Ahh yeah I lost a bunch of tracks - ahh well I can recreate them but basically if you return a name it does the wrong thing in logic unless you have the whole preset mechanism working which we don't

So don't merge this patch.

[ZonderP]

'Heck you could even only make it work in ext' -> Heck you could even make it work everywhere except in Logic! :-)

Although when I first read your post, I was tempted to say something like "I would probably never want to add 'per host' code."

[baconpaul]

I suggest we condition the patch on vst2 only.

[ZonderP]

So it doesn't behave bad in Logic, if VST2 would be used?

[ZonderP]

I mean, if you're willing to add host related code then it might be something like: if (VST2 && !Logic) {return patch.name} else {return ""}

[baconpaul]

Logic can’t load vst2

[ZonderP]

Oh, really! First DAW I hear of which completely abandoned VST2.

[baconpaul]

It never supported it.

[ZonderP]

Oh, yeah - well, it's Apple since nearly 20 years now. But I think I remember it once - long, long time ago - supported VST, but that was probably VST1 at the early 2000s where it was still available under Windows - Logic 5 or 6... Does it even natively support VST3 or must some bridging/wrapping happen? (A quick internet research gave me the impression it would only support AU)

[baconpaul]

it only supports au. and has since the emagic days even before the apple acquisition if i recall. (I didn't use it in VST1 days).

[mkruselj]

I think this should be added for both VST2 and VST3. I'll chase Reaper devs.

[baconpaul]

So i think we should be careful what happens with logic is not a logic bug its a surge bug and i can't find it if you save a track with track name init sine, when you reopen the project it streams your item on then sets the patch name and that forces a load of init sine so saving a project means it won't re-load

the au does that

i don't know why

you sure you want to change this for the vst3 when its not working in reaper and we have that behavior? I think we should not.

[mkruselj]

Let me check if anything like that even happens with VST3 in Reaper first!

EDIT: Yeah nothing of the sort seems to happen there. It just doesn't refresh the prest name in the preset menu in the header. That's all.

Why would track name have anything to do with which patch is loaded in the instrument plugin? So weird.

[baconpaul]

I think it is trying to send a “set to init sine” message improperly. Like I said I can’t debug it. But it gives me pause of adding a half assed implementation of the preset mechanism…

[baconpaul]

Another explanation could be something like if the name is not empty and not in the preset list then reset the synth. That would also match what I’m seeing. So basically the result of getpresetname has to be one of the presets returned by getpreset otherwise it goes nutty.

I can’t quite find it but that also matches what I’m seeing.

like I said, I really don’t think we should merge this change. While I wouldn’t go as far as to say no, as long as you knock out AU, I am very worried that we don’t understand it and the consequences of it in all our contexts.