lv2-porting-project / JUCE

The JUCE cross-platform C++ framework
https://juce.com
Other
15 stars 4 forks source link

Surge XT LV2 stuck at 100% #6

Open tank-trax opened 3 years ago

tank-trax commented 3 years ago

Hello. I am a member of the Surge Synth team and tried out the LV2 build for Surge XT and found what appears to be a bug with the LV2. This does not happen with the Standalone nor the VST3.

Version: Surge XT 0.9.main.49f99b4d Build: 2021-08-19 @ 17:11:52 on 'LinuxDAW/local' with 'GNU-10.2.1' using JUCE 60008 System: Linux 64-bit LV2 on Intel(R) Core(TM) i5-3470 CPU @ 3.20GHz

Surge XT will load properly at 100% but the container seems stuck at that dimension for 100%.

This is how it looks at 100%

image

And here at 125%

image

This behavior happens in jalv.select, Reaper and Ardour

There is no way to stretch or maximize. Here is another example if set to 150% in Reaper, the GUI container remains at 100%.

image

KottV commented 3 years ago

Hi, I saw that too. It happens with the Surge XT only, other plugins do resize correctly. Maybe it uses it's own method than JUCE's setSize?

baconpaul commented 3 years ago

nope. just calling juce editor->setSize

tank-trax commented 3 years ago

Another Synth that resizes that uses JUCE and is LV2 is Odin2 and it does the same thing.

Here at 100% image

And here at 150% image

tank-trax commented 3 years ago

this may be LV2 specific as Samplv1 has a similar response

When opened via jalv.select image

If the bottom right hand corner is dragged...

image

KottV commented 3 years ago

Odin2 2.3.1 works fine in jalv.gtk

odin2

KottV commented 3 years ago

samplv1 has a little odd behavior, if i drag it by it's resize corner - it doesn't change window size/borders if i resize it with WM (dragging the border on Meta+MRB) - it works

tank-trax commented 3 years ago

Surge Synthesizer System: Linux 64-bit LV2 (experimental) on Intel(R) Core(TM) i5-3470 CPU @ 3.20GHz Surge (not XT) Build Info: Built on 2021-05-06 at 00:07:28, using local host 'LinuxDAW' with 'GNU-10.2.1' Version: 1.9.HEAD.72778f62

not built using JUCE is OK, resize feature works well

KottV commented 3 years ago

not built using JUCE is OK, resize feature works well

yes it's true, but it's is not related to this Surge XT - JUCE-lv2 issue Surge code is huge for me, if you point me where it does resizing - i'll try to find the problem

baconpaul commented 3 years ago

SurgeGUIEditor::setZoomFactor in src/gui/SurgeGUIEditor.cpp

at head line 1912 calls juceEditor->setSize()

juceEditor is a reference to the AudioProcessorEditor subclass SurgeSynthEditor in src/surge_synth_juce

We call setZoomFactor on the UI thread from a user introduced action. It's bound to the various menus.

But if I were debugging this i'd start in setZoomFactor at that setSize call and debug down from there

KottV commented 3 years ago

Thank You! I'll take a look.

baconpaul commented 3 years ago

one thought is i also use juceEditor->setScaleFactor and maybe LV2 is responding to that incorrectly? very first thing I would try is to call that before the setSize not after and see if it fixes the LV2. If it is you know your bug at least?

baconpaul commented 3 years ago

on and that setScaleFactor is 2 lines down from setSize

KottV commented 3 years ago

Hm, interesting, putting smth hardcoded in juceEditor -> setSize() do resize in jalv.gtk. I'll play with debug build a bit later;

tank-trax commented 3 years ago

Odin2 2.3.1 is OK now. Had an old build. Thanks for the heads up @KottV

KottV commented 3 years ago

Ok, i've put:

        DBG("x: "<< currentSkin->getWindowSizeX()<<" y: " << currentSkin->getWindowSizeY() + yExtra << "\n");
        juceEditor->setSize(currentSkin->getWindowSizeX(), currentSkin->getWindowSizeY() + yExtra);

in SurgeGUIEditor.cpp:1912

running any version (standalone, vst3, lv2) and switching the zoom from menu gives always the same values:

run at default 125% x: 904 y: 569 switch to 150% x: 904 y: 569 switch to 100% x: 904 y: 569 etc

so, why it works in vst3 and standalone? o_O or am i doing smth stupidly wrong?

baconpaul commented 3 years ago

Ahh that answers it The scale factor on the core editor grows the window in vst3 and standalone I bet

baconpaul commented 3 years ago

Put a print on the scale factor below the set size

so rather than a 100% window at 125size we are using a 125scake window at 100size

baconpaul commented 3 years ago

So the answer is make the lv2 also send the resize events when scale factor is increased on the editor component somehow. Probably?

KottV commented 3 years ago

I don't understand how https://docs.juce.com/master/classAudioProcessorEditor.html#a2b80bdfc3ba742fed44fe834a8293eb9 works from plugin's side.

Anyway, this is works for sure:

juceEditor->setSize(currentSkin->getWindowSizeX() * zoomFactor * 0.01, currentSkin->getWindowSizeY() * zoomFactor * 0.01 + yExtra);

yes it's better to cache, but it works :) whoops, but it breaks VST3 and standalone then

KottV commented 3 years ago

I think I got it. Surge tell itself to change scaleFactor as if it done by the host. VST3 has support for that, but LV2 wrapper - seems to doesn't. I asked falkTX. As dirty workaround it can check for the wrapper type then use setSize with recalculated values.

KottV commented 3 years ago
index f7fa40aa..f0ab663c 100644
--- a/src/gui/SurgeGUIEditor.cpp
+++ b/src/gui/SurgeGUIEditor.cpp
@@ -1808,7 +1808,14 @@ void SurgeGUIEditor::setZoomFactor(float zf, bool resizeWindow)
         {
             yExtra = SurgeSynthEditor::extraYSpaceForVirtualKeyboard;
         }
-        juceEditor->setSize(currentSkin->getWindowSizeX(), currentSkin->getWindowSizeY() + yExtra);
+        if (juceEditor->processor.wrapperType == juce::AudioProcessor::wrapperType_LV2)
+        {
+            juceEditor->setSize(currentSkin->getWindowSizeX() * zoomFactor * 0.01, currentSkin->getWindowSizeY() * zoomFactor * 0.01 + yExtra);
+        }
+        else
+        {
+            juceEditor->setSize(currentSkin->getWindowSizeX(), currentSkin->getWindowSizeY() + yExtra);
+        }
     }
     juceEditor->setScaleFactor(zoomFactor * 0.01);
     setBitmapZoomFactor(zoomFactor);

LV2 and VST3 at 125% scale

изображение

baconpaul commented 3 years ago

Right OK so we know what the bug is. I've been able to avoid putting any wrapper type specific stuff into the code so far and would rather hold off for a bit until you can fix the branch. If we get close to the XT production release in dec and you can't fix the LV2 branch I can (somewhat reluctantly) merge a change like that or something similar.

KottV commented 3 years ago

sounds like you put conditions on me :)

baconpaul commented 3 years ago

No! My point is: our strategy with bugs in juce through the cycle has been get them fixed upstream and store the patch in case that doesn’t happen before release. We did that with several things as we approach 6.1 with the juce group.

basicalky if it’s dec and this still isn’t fixed we know what to do. But I don’t want to do it since it’s not “right” in some purist sense, so let’s try and fix the core bug

KottV commented 3 years ago

I was joking! But truth is that it can be challenge for me. I'll ask @falkTX again, because this method seems "legal". For example, Vital uses it too https://github.com/mtytel/vital/blob/c0694a193777fc97853a598f86378bea625a6d81/src/plugin/synth_editor.cpp#L60

baconpaul commented 3 years ago

the method is totally legal yes. And with vector assets it means JUCE takes care of all the scaling for you. It's really handy!

KottV commented 3 years ago

I see. And Vital has the same issue with LV2 version too.

baconpaul commented 3 years ago

Anyway here's how it works with vst3

setScale -> sets the transform and calls setTransform setTransform -> sets the transform and calls sendMovedResizedMessages sMRM -> triggers the component listener componentMovedOrResized event juce_VST3PluginFormat componentMovedOrResized sends the appropriate message

It seems the LV2 doesn't implement a componentMovedOrREsized callback.

paul:~/dev/music/JUCE-lv2$ grep -r componentMoved modules/ | grep LV2
paul:~/dev/music/JUCE-lv2$ grep -r componentMoved modules/ | grep VST3
modules//juce_audio_processors/format_types/juce_VST3PluginFormat.cpp:    void componentMovedOrResized (bool, bool wasResized) override
modules//juce_audio_processors/format_types/juce_VST3PluginFormat.cpp:    using ComponentMovementWatcher::componentMovedOrResized;
modules//juce_audio_processors/format_types/juce_VST3PluginFormat.cpp:        componentMovedOrResized (true, true);

so seems that's the work to be done.

falkTX commented 3 years ago

hmm? maybe a new thing? I remember the scaling with vitalium working fine. still works fine here, let me see if I can attach a quick recording... (cant attach recording, damn) but you can see it working at https://falktx.com/data/vitalium-lv2-2021-08-24_13.26.04.webm

falkTX commented 3 years ago

Anyway here's how it works with vst3

setScale -> sets the transform and calls setTransform setTransform -> sets the transform and calls sendMovedResizedMessages sMRM -> triggers the component listener componentMovedOrResized event juce_VST3PluginFormat componentMovedOrResized sends the appropriate message

It seems the LV2 doesn't implement a componentMovedOrREsized callback.

It listens to child bounds changed https://github.com/DISTRHO/juce/blob/main/modules/juce_audio_plugin_client/LV2/juce_LV2_Wrapper.cpp#L361

baconpaul commented 3 years ago

Right! Well that's the problem. setMovedResizedMessages calls componendMovedOrResized

falkTX commented 3 years ago

Those are for listeners, so we have to setup one?

Can you give me instructions to build surge-xt lv2? I think I need to set this up myself.

baconpaul commented 3 years ago

Sure!

https://github.com/surge-synthesizer/surge#building-an-lv2

And yes, VST3PluginWindow is a ComponentMovementWatcher and in its constructor makes a call to ComponentMovementWatcher(this) which does a component->addComponentListener.

KottV commented 3 years ago

hmm? maybe a new thing? I remember the scaling with vitalium working fine. still works fine here, let me see if I can attach a quick recording... (cant attach recording, damn) but you can see it working at https://falktx.com/data/vitalium-lv2-2021-08-24_13.26.04.webm

Yep, I tested vanilla Vital in jalv.gtk only, in Carla and Reaper it works fine. And Vitalium works in jalv.gtk also.