surge-synthesizer / tuning-workbench-synth

A simple JUCE synth which uses our tuning-library
GNU General Public License v3.0
48 stars 8 forks source link

VST3 does not display GUI in Qtractor #124

Closed tank-trax closed 4 years ago

tank-trax commented 4 years ago

When loading TWS VST3 in Qtractor it does not display the default GUI and only allows for a generic GUI.

Linux Debian Buster 10.3 TWS Version 1.0.0 VST 3.6.13 Qtractor Version 0.9.14.4git17d707

baconpaul commented 4 years ago

That is true

@rncbc we have another few VST3 things in the surge family which are built using JUCE6. TWS is one of them and it basically doesn't show the UI in qtractor but does in bitwig, reaper, etc...

I just tried pulling to head of juce6 and rebuilding and that didn't fix it (TWS will build with a submodule a few behind). But may be a useful thing for you to look at?

We can look too of course, but TWS is really just standard juce6 plugin spits out linux vst3 with their cmake.

tank-trax commented 4 years ago

Same situation for SurgeEffectsBank VST3

baconpaul commented 4 years ago

Wonder if it is because juce uses gtk or some other such dependency?

rncbc commented 4 years ago

dunno but apparently IEditController::createView("editor") is returning a nullptr on those plugins... seems to be JUCE6 interface that doesn't like being hosted under Qt/X11 ? :/

baconpaul commented 4 years ago

Interesting. These plugins work in reaper/bitwig/juce audiopluginhost but I'm not sure what code path those use. Lemme peek.

baconpaul commented 4 years ago

So here's what I think the problem is. When you call createView I don't have a reference to my plugin instance. Nothing to do with X11 or GTK.

From reading the JUCE code I have a theory.

Surge is a 'single component' plugin. That is, the editor and the processor are the same class. VST3 has a 'two component' plugin model which they recommend and which JUCE components eject.

In order for that to work you need to connect the editor to the plugin and vice versa in the host.

So what JUCE does is it creates the plugin and creates the editController, just like you do, but then (in juce_VST3PluginFormat) it interconnects them (the function is 'interconnectComponentAndController') with this code

    /** Some plugins need to be "connected" to intercommunicate between their implemented classes */
    void interconnectComponentAndController()
    {
        componentConnection.loadFrom (holder->component);
        editControllerConnection.loadFrom (editController);

        if (componentConnection != nullptr && editControllerConnection != nullptr)
        {
            warnOnFailure (componentConnection->connect (editControllerConnection));
            warnOnFailure (editControllerConnection->connect (componentConnection));
        }
    }

where componentConnection and so on are instances of VST3::IConnectionPoint. I couldn't find where qtractor does that and my theory (perhaps wrong) is that it is missing?

So if all the components you've tested up until now are single-class components (and surge is), it may be that the juce6 ones are the first split-class ones, and are missing this connection.

I couldn't quite figure out how to plumb this in, but my guess is you probably want something like that somewhere around line 875 or 880 of qtractorVst3Plugin.cpp?

Thoughts?

baconpaul commented 4 years ago

oh and loadFrom is a JUCE-ism to make sure that queryInterface works (that's actually a ComObject.

baconpaul commented 4 years ago

Yeah OK that's definitely part of it. If I do a connection I get the UI opening but not drawing.

baconpaul commented 4 years ago
@@ -1103,6 +1105,16 @@ bool qtractorVst3PluginType::Impl::open ( unsigned long iIndex )

            if (controller) m_controller = owned(controller);

+           Vst::IConnectionPoint *c1, *c2;
+                        if( m_component->queryInterface( Vst::IConnectionPoint::iid, (void **)&c1 ) != kResultOk ) c1 = nullptr;;
+                        if( m_controller->queryInterface( Vst::IConnectionPoint::iid, (void **)&c2 ) != kResultOk ) c2 = nullptr;;
+                        std::cout << "C1/2 = " << c1 << " " << c2 << std::endl;
+                        if( c1 && c2 )
+                        {
+                                c1->connect(c2); c2->connect(c1 );
+                        }
+
+
            Vst::IUnitInfo *unitInfos = nullptr;
            if (m_component->queryInterface(
                    Vst::IUnitInfo::iid,
@@ -1120,7 +1132,7 @@ bool qtractorVst3PluginType::Impl::open ( unsigned long iIndex )
            }

            if (unitInfos) m_unitInfos = owned(unitInfos);
-
+ // connect
            return true;
        }

that qtractor diff gets the window showing up right size no contents.

so I think: qtractor has to do a bit of work for split-model VST3s but is super close.

hope that helps some!

rncbc commented 4 years ago

after digging into some JUCE6 code I come to realize in despair that it's not going to work in (my) qtractor VST3 host implementation so soon... :/

hold and behold: JUCE6 still suffers from its ugly and dang old JUCE1 VST2 implementation that assumed (and still assumes as a matter of fact as it seems deep hard-coded in both host and plugin wrappers) that the GUI is always to be brought up and splatted to the screen, always, no matter if you're only querying or scanning for inventory and what not isn't a full-blown plugin instance and audio processor up and running.

all that to say that qtractor plugin inventory scan reports no GUI for any JUCE6 based plugins... and that it won't get fixed anytime so soon. so sorry.

tank-trax commented 4 years ago

the TWS and SurgeFX built using JUCE5 load their GUI's in Qtractor, although I am getting black screens with other Instruments (OB-XD and Synister) built using JUCE5

rncbc commented 4 years ago

hat qtractor diff gets the window showing up right size no contents.

so I think: qtractor has to do a bit of work for split-model VST3s but is super close.

good catch, we're getting somewhere... after some tests I could bring up the (empty) window alright.

thanks anyway

tank-trax commented 4 years ago

I should add that both of those builds are VST2 instruments. And they appear instantaneously.

rncbc commented 4 years ago

updated in https://github.com/rncbc/qtractor/commit/40b9a8e

the empty window issue remains though.

thanks again

baconpaul commented 4 years ago

Cool thank you!

Hey where in your code do you connect your xcb loop with the runloop? I wonder if there's an instance issue there also. (I can look but figure if you have 10 seconds you could point me at it).

rncbc commented 4 years ago

Hey where in your code do you connect your xcb loop with the runloop? I wonder if there's an instance issue there also. (I can look but figure if you have 10 seconds you could point me at it).

look for#ifdef CONFIG_VST3_XCB in qtractorVst3Plugin.cpp; basically the core business is handled in qtractorVst3PluginHost::processEventHandlers(), here https://github.com/rncbc/qtractor/blob/master/src/qtractorVst3Plugin.cpp#L708

tank-trax commented 4 years ago

after retrying can confirm that a number of VST2's built using various versions of JUCE all load well in Qtractor, this list includes:

it looks like this is specific to VST3

baconpaul commented 4 years ago

OK closer. I've now gone from a white screen to a black screen. Progress!

Qtractor wasn't calling any of the registered event handlers in the run loop because the g_hostContext timer wasn't running; so you were never polling the xcb loop. (using my patented "debug with printfs" this was pretty clear)

Here's a patch which started the timer so that in split mode TWS shows up as a black screen and you are calling into me to at least try and do something with the FD. Will keep plugging away.

@ -2344,6 +2352,7 @@ bool qtractorVst3Plugin::Impl::openEditor (void)

 #ifdef CONFIG_VST3_XCB
        g_hostContext.openXcbConnection();
+       g_hostContext.startTimer( 20 );
 #endif

        Vst::IEditController *controller = pType->impl()->controller();
@@ -2360,6 +2369,7 @@ void qtractorVst3Plugin::Impl::closeEditor (void)

 #ifdef CONFIG_VST3_XCB
        g_hostContext.closeXcbConnection();
+       g_hostContext.stopTimer();
 #endif
 }
baconpaul commented 4 years ago

Ahh and now i see why it doesn't work In this split mode I get two registrations for the same has with different FDs. But your internal data structure is QHash<IEventHandler *, int> so you collide lemme quickly change that to a std::vector<pair<>> and see if it works

baconpaul commented 4 years ago
Screen Shot 2020-06-03 at 5 04 22 PM

Got it

You won't want to apply my patch but it shows you the fixes. Basically the event handlers should be a hash on the object/fd pair not just the object. I used std::vector and std::pair to test it because I don't know your data structures but you get the idea.

vst3.patch.txt

baconpaul commented 4 years ago

oh and this is the same bug that made it not work in bitwig 3.1 that got fixed in 3.2 beta @tank-trax - basically event handler data structure with the wrong key.

tank-trax commented 4 years ago

for Qtractor 0.9.14.9git.a52b86 built on Debian Buster both TWS and SurgeFX now display the black screen however they generally segfault if closing and cannot go back to generic GUI

Update: applied the patch and the GUI for TWS appears and am able to play different patches.

Screenshot_20200603_230935

tank-trax commented 4 years ago

this may be of interest here is a list of the VST2 instruments, all have GUI,EXT,RT

with Dexed, OB-Xd, SurgeEffectsBank, synister, and tuning-workbench-synth all built using JUCE (not version 6)

Screenshot_20200603_235551

the VST3 plugins built using JUCE6 only have EXT,RT

Screenshot_20200603_235148

rncbc commented 4 years ago

You won't want to apply my patch but it shows you the fixes. Basically the event handlers should be a hash on the object/fd pair not just the object. I used std::vector and std::pair to test it because I don't know your data structures but you get the idea.

vst3.patch.txt

adapted and merged in https://github.com/rncbc/qtractor/commit/40d41926

yes, it shows up now and seems functional; only thing left now is sporadic random and otherwise rare crashes on plugin de-instantiation and spotted on the segfault back-trace as on "JUCE Timer thread" ... a batle for another day :)

thanks again

baconpaul commented 4 years ago

Thank you! I'll pull now

@tank-trax @rncbc the reason it doesn't show up with the GUI tag is that qtractor calls createView twice; once to show the view and once to see if it can. The second one which sets m_boolEditor doesn't do the connect thing.

If I apply this patch against head and rescan the tws shows up as GUI,EXT,RT

guiflag.patch.txt

Good stuff!

baconpaul commented 4 years ago

Oh and @rncbc if i had just looked at the QHash header and seen insertMulti I could have saved us both a lot of time - chuckle. Sorry bout that. Thanks for merging the corrected version of the fix.

baconpaul commented 4 years ago

oh and in my patch you probably want to disconnect after you create also I just realized otherwise the scan could leave a reference around.

rncbc commented 4 years ago

oh and in my patch you probably want to disconnect after you create also I just realized otherwise the scan could leave a reference around.

i believe that's already been taken care of before in https://github.com/rncbc/qtractor/commit/40b9a8e

baconpaul commented 4 years ago

oh in the mainline yeah it is working great. in the little patch I just posted that also connects before you do the throwaway createView in scan, you will want to disconnect after the scan createView I think.

tank-trax commented 4 years ago

tested Version: 0.9.14.11git.7ee91f and both SurgeFX and TWS displayed their GUI's however under Modes are still listed as EXT,RT

rncbc commented 4 years ago

rescan? being out-of-process you probably have an outdated inventory cache; it does not get invalidated if you replace binaries with very same pathname.

tank-trax commented 4 years ago

yes re-scanning added the GUI, thanks!

baconpaul commented 4 years ago

Cool! I’m so excited that this all works. Thanks so much @rncbc