Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lv2 UI - Testing #7201

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Lv2 UI - Testing #7201

wants to merge 6 commits into from

Conversation

JohannesLorenz
Copy link
Contributor

@JohannesLorenz JohannesLorenz commented Apr 8, 2024

This is solely for early testing yet. From the code review perspective, there are still a few things to be done.

When reporting an issue, please try to answer the following questions:

  1. Does this issues occur in any other host, like Ardour/Jalv/...?
  2. If the issue happens with UI, does it also happen if UI is turned off? (Edit -> Settings -> Performance -> Plugins -> External plugin embedding -> "No embedding")

Open issues:

  • Wait for SUIL vcpkg port?
  • Regression: The old ZASFX window is oversized (but TripleOsc is not)
  • Always show LMMS UI (allow to hide it), even if Plugin UI is available
  • The size and the resizability seem sometimes buggy in this PR (LV2_UI__noUserResize. LV2_UI__fixedSize)
    • "Rotary Speaker - simple" is way too small
    • Issues with Cardinal Synth
    • Effects have the resize markers even if not resizable
    • Sometimes, plugins UIs do not have optimal size. Reason: Lv2ViewProc inside Lv2ViewBase might be embedded wrong? Waiting on L/R routing, see below
  • Ban plugins if they are GUI only AND GUI is not enabled
  • LSP EQs cannot use any filters
  • Implement requestValue (this has to wait for the Patch extension)
  • Implement ShowInterface (will not be implemented for now)
  • Implement touch - NO, separate PR
  • Add Mono-Effects to ban-list if there are analogue Stereo effects (especially LSP)
  • Rebase on messmerd's pin connector (Add Plugin Pin Connector #7459), but it has not been finished yet
  • X42 effects have only GTK UI - are not loadable with this PR - jalv has the same problem - x42 effects need Qt UI for that
  • Effects do not seem to load anything (Ping Pong Pan, 3 Band EQ)
  • setBfree/Dexed issues
  • Dexed octave issues

@JohannesLorenz
Copy link
Contributor Author

This now runs successfully in the CI, and it supports UI there for Linux and Macos, but not Windows yet. @DomClark , as discussed, I would appreciate if you would submit a vcpkg PR (and then add suil to our vcpkg.json in this PR or tell me to). In the end, the CI for Windows should not show

* SUIL for plugin UIs         : not found, install it or set PKG_CONFIG_PATH appropriately

but

* SUIL for plugin UIs         : OK

@zonkmachine
Copy link
Member

Dexed.

  • When I press it's built-in keyboard there is only sound in the left channel.
    Sending a note from lmms presents sound on both channels but if I change sound on Dexed, the left channel will follow the new setting but the right will stay with the default sound.

  • Octaves. Dexed C4 corresponds to LMMS C5. Both can't be right. Right?

@zonkmachine
Copy link
Member

  • When I press it's built-in keyboard there is only sound in the left channel.

I assume it's related to this PR: #7247

@messmerd
Copy link
Member

I assume it's related to this PR: #7247

The LV2 implementation in LMMS uses linked models, so mono plugins are actually 2 linked plugin instances (L and R) behind the scenes. Linked models need to be kept in sync for parameter updates which is already a complicated task, but adding a UI where updates can come not just from LMMS but also the plugin itself makes the linked model design untenable in my opinion. The issue with dexed sounds like exactly the type of synchronization problem I feared.

I would strongly recommend to @JohannesLorenz to replace the linked model design with the L/R routing design from #7247 once it's merged. I've been using it successfully for both CLAP and VST plugins and it works great. Making the switch will almost certainly be less work than trying to fix the bugs with linked models.

Not only is the L/R routing approach dramatically simpler and less buggy than trying to use linked models, it is also very flexible and intuitive from a user standpoint and uses roughly half the resources in the common situation where you only want a mono-to-stereo mix from your mono plugin.

@michaelgregorius
Copy link
Contributor

I assume it's related to this PR: #7247

The LV2 implementation in LMMS uses linked models, so mono plugins are actually 2 linked plugin instances (L and R) behind the scenes. Linked models need to be kept in sync for parameter updates which is already a complicated task, but adding a UI where updates can come not just from LMMS but also the plugin itself makes the linked model design untenable in my opinion. The issue with dexed sounds like exactly the type of synchronization problem I feared.

Has this already been fixed? It sound like a waste of resources, a source for tons of bugs and a complete misunderstanding of how these things work. Sorry, if I am a bit blunt here but keeping it like this would take the implementation in a completely wrong direction. I am pretty sure that no other DAW implements it like this.

If a plugin reports as mono then simply copy the resulting mono output buffer of the single instance into the left and right channel buffers of the stereo track.

I would strongly recommend to @JohannesLorenz to replace the linked model design with the L/R routing design from #7247 once it's merged. I've been using it successfully for both CLAP and VST plugins and it works great. Making the switch will almost certainly be less work than trying to fix the bugs with linked models.

100% agree!

@tresf
Copy link
Member

tresf commented Jun 5, 2024

I pushed a few commits to help with AppImage findings over on Discord's #testing channel.

For now, I copy the suil-0 modules to <APPDIR>/lib/suil-0/, but I can't find a suil API to notify it of this location, so per @JohannesLorenz's advice, I set SUIL_MODULE_DIR in our launcher script.

(Until #7252 is merged, I can't test this on ARM64, so I'm just hoping this fixes the AppImages)

Initial testing on MacOS is OK but we don't use a launcher script for MacOS, Perhaps qputenv() combined with qApp->applicationDirPath() + "../Frameworks/suil-0/? Thoughts welcome

Screenshot 2024-06-05 at 12 15 54 AM

@tresf
Copy link
Member

tresf commented Jun 5, 2024

Initial testing on MacOS is OK but we don't use a launcher script for MacOS, Perhaps qputenv() combined with qApp->applicationDirPath() + "../Frameworks/suil-0/? Thoughts welcome

Done via 5bb73b0. Feel free to move the code elsewhere if needed.

tresf added a commit to tresf/lmms that referenced this pull request Jun 6, 2024
@zonkmachine zonkmachine marked this pull request as ready for review June 11, 2024 17:19
@zonkmachine zonkmachine marked this pull request as draft June 11, 2024 20:24
@zonkmachine
Copy link
Member

When I reload a project the lv2 gui doesn't update. The settings seem to stick, just the gui is showing it's init setting.

@JohannesLorenz
Copy link
Contributor Author

When I reload a project the lv2 gui doesn't update. The settings seem to stick, just the gui is showing it's init setting.

@zonkmachine Can you please elaborate?

I did:

  • Creat a new Lv2 project, opened Vitalium, changed some params, saved the project
  • File -> Recently Opened Projects ->
  • Reopen the Vitalium UI

=> All params are as before.

Did I understand "reload" wrong?

@zonkmachine
Copy link
Member

* Creat a new Lv2 project, opened Vitalium, changed some params, saved the project

* File -> Recently Opened Projects ->

* Reopen the Vitalium UI

=> All params are as before.

With nekobi I do:
nekobisave

I save and reload and I get:
nekobisave2

The sound is the same as when I saved but the settings show a default nekobi.

@JohannesLorenz
Copy link
Contributor Author

@zonkmachine Thanks. I fixed it.

@zonkmachine
Copy link
Member

@zonkmachine Thanks. I fixed it.

This took care of the instruments but the effects see the same issue and this is not fixed by 3bcf80b.

@JohannesLorenz
Copy link
Contributor Author

This took care of the instruments but the effects see the same issue and this is not fixed by 3bcf80b.

Actually, this commit should have fixed it for both... Which effect do you mean?

Sometimes, producing input for an effect (i.e. pressing a key) might force a lazy UI to update. Not sure if this helps here.

@zonkmachine
Copy link
Member

Actually, this commit should have fixed it for both... Which effect do you mean?

Ping Pong Pan
3 Band Eq
GxRedeye Chump

There were more. Actually, the only Lv2 effect with gui that I've come across that works in this regard is Calf Analyzer (not an effect per se but the stored parameters are displayed correctly when reloading a project).

Sometimes, producing input for an effect (i.e. pressing a key) might force a lazy UI to update. Not sure if this helps here.

Nope.

@JohannesLorenz
Copy link
Contributor Author

JohannesLorenz commented Jun 16, 2024

Oh, I get it. For instruments, first, the instrument is loaded, which includes loading the savefile. Only after that, the UI is initiated - at which time the mentioned fix sends all port values (knobs, sliders etc) to the UI.
For effects, the UI seems to be opened immediately, and the default values are sent to the UI. Only after that, the savefile values are being loaded, but they are not sent to the UI anymore.

Thinking about a fix...

Confirming: Ardour sends port updates of all ports to UI after loading a preset (LV2PluginUI::queue_port_update).

@zonkmachine
Copy link
Member

New issue. The setBfree organ plugin doesn't save the settings at all.

@JohannesLorenz
Copy link
Contributor Author

New issue. The setBfree organ plugin doesn't save the settings at all.

Does this work in Ardour or another DAW (my favorite question to procrastinate all bugs)?

@zonkmachine
Copy link
Member

Does this work in Ardour

Yes. Works fine in Ardour.

@zonkmachine
Copy link
Member

setbfree gui explained here: https://setbfree.org/lv2/lv2_setbfree_up.html
I think it could be because it uses presets and it's not implemented yet. Like Dexed.

@JohannesLorenz
Copy link
Contributor Author

I think it could be because it uses presets and it's not implemented yet. Like Dexed.

I think some use "Preset" and some use "State". Multiple of them contain "organ" in the description. Do you mean any specific one?

@zonkmachine
Copy link
Member

Do you mean any specific one?

No. I'm very unspecific here.

@JohannesLorenz
Copy link
Contributor Author

I cannot reproduce your Dexed issues: I turned all 8 "EG" level in "1" to 0, and when loading, they were still 0 and I think it sounded correct. How do I reproduce?

With X42 whirl (from the b_free plugins), I can reproduce the issue.

@JohannesLorenz
Copy link
Contributor Author

The setBfree organ, Dexed and Zyn all are unable to save anything. This requires the "State" extension, since these do not have ports.

I think we should (independently) ban all plugins that use state - until this is implemented. Otherwise, users will get annoyed because their GUI values are not being saved.

@JohannesLorenz
Copy link
Contributor Author

However, saving does not seem to work (as you reported) - even on the same effects that did not load: Ping Pong Pan and 3 Band EQ.

Fixed in dbe71a5. From now, all know load/save issues should be fixed!

@JohannesLorenz
Copy link
Contributor Author

  • Octaves. Dexed C4 corresponds to LMMS C5. Both can't be right. Right?

Right. How is it with Ardour? If Ardour does not tell you what C4/C5 is, you could compare it with help of a different instrument.

@zonkmachine
Copy link
Member

Right. How is it with Ardour? If Ardour does not tell you what C4/C5 is, you could compare it with help of a different instrument.

Right, right. Did a quick test in Ardour and it's the same issue there. I'll look into this some more.

@JohannesLorenz
Copy link
Contributor Author

I fixed really a lot of issues. One remaining issue though is that the opening LMMS effect window sometimes is a bit too small to contain the UI. This will hopefully be fixed soon.

Aside from this, if anyone finds any issues with the current version, please post it.

Also, keep in mind the "When reporting an issue" section from above, and keep in mind that the PR does not cover automation of UI controls, presets and LMMS file browsers inside of the UI.

@zonkmachine zonkmachine marked this pull request as ready for review July 1, 2024 12:38
@zonkmachine
Copy link
Member

I can't believe I did this again... Oh, lawd!

@zonkmachine zonkmachine marked this pull request as draft July 1, 2024 12:39
Comment on lines 40 to 45
template<class T>
struct LilvPtrDeleter
{
void operator()(T* n) { lilv_free(static_cast<void*>(n)); }
};

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this needs to be a class template. If the operator takes void*, it should still work with std::unique_ptr.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A void* would suffice, but then, in statements like

auto bundlePath = AutoLilvPtr<char>(lilv_file_uri_parse(bundleUri, NULL));

we would lose the information that this is a char pointer. So, to improve type safety, I think a template makes sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, you can have a std::unique_ptr<T> that stores T* but uses a deleter which takes void*. This works because T* implicitly converts to void*.

Here's a working demo: https://godbolt.org/z/Msfrc5jfa

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but if we get an explicit cast for free, I prefer that.

@JohannesLorenz
Copy link
Contributor Author

Update: I have multiple times asked whether the LMMS-UI should be displayed even if a Plugin-UI is displayed, and the answer was rather yes, because it is required for automation. While this might be doable via the "UI touch" function, many plugins do not support "touch" at all (e.g. zyn does not), so for those, there is no other choice. This leads to the following decision:

  1. If no Plugin UI can be loaded, load LMMS UI (as before)
  2. If a Plugin UI can be loaded, load LMMS UI additional (but possibly add an expander to hide it)

In other words, the LMMS UI is displayed in any case, though sometimes, an expander to hide it might be useful.

JohannesLorenz and others added 2 commits August 4, 2024 21:44
NOTE: This commit MUST GO BEFORE the lv2 UI commit
This fixes `EffectRackView` to have a permanent size hint instead of
resizing the widget once in `InstrumentTrackWindow`. The size hint tells
the `InstrumentTrackWindow` to not increase with a growing number of
effects in the `EffectRackView`.
@zonkmachine
Copy link
Member

lv2dexedgui

Some of the LV2's don't fit well in their windows. I don't remember this from earlier. This is how Dexed shows up, with a lot of margin on top and to the left and cut off on the bottom/right. I also can't change the size of the window manually.

@Gabrielxd195
Copy link

Don't you think it would be better if lv2 plugins had their own separate window like vst plugins? Don't you think it would be better to have a "vestige" type instrument but for lv2 plugins? I think we would have two advantages over the current way of displaying lv2 instruments and that is that it would be better visually and more consistent with the size of the other instruments, and that each plugin has a different size, they cannot be resized and this can be very annoying on small screens.

The other advantage would be better performance since on low-resource computers the graphical interfaces of the plugins being open can consume more resources; however, from a "vestige" type instrument window but with the option (a "Show/Hide GUI" button) to show the graphical interface to make some precise changes, you can control some parameters of the plugin without opening its graphical interface.

(Google Translate)

@JohannesLorenz
Copy link
Contributor Author

Some of the LV2's don't fit well in their windows. I don't remember this from earlier. This is how Dexed shows up, with a lot of margin on top and to the left and cut off on the bottom/right.

This is a know issue (still open in my checkbox list at the OP): "Sometimes, plugins UIs do not have optimal size.". This waits for L/R routing because after that, the way we put the plugin into the window changes.

. I also can't change the size of the window manually.

Dexed does not allow resizing. However, as soon as we have fixed the size, resizing should not be required anymore.

@JohannesLorenz
Copy link
Contributor Author

@Gabrielxd195 Thanks for your input.

Don't you think it would be better if lv2 plugins had their own separate window like vst plugins? Don't you think it would be better to have a "vestige" type instrument but for lv2 plugins?

I don't find it necessary to be exactly like VST.

it would be better visually and more consistent with the size of the other instruments

This could be a point, but most Lv2 instruments already have a larger than the default size (before lv2-ui). We did this to use the space better, especially on large monitors. E.g. try Helm, putting all these knobs into the small default-sized window with only 1 vertical scrollbar would be insane to navigate.

The other advantage would be better performance since on low-resource computers the graphical interfaces of the plugins being open can consume more resources; however, from a "vestige" type instrument window but with the option (a "Show/Hide GUI" button) to show the graphical interface to make some precise changes, you can control some parameters of the plugin without opening its graphical interface.

We had a discussion in Discord lately, and the result was that I wanted to try out a TabWidget, like we currently have it for the Instrument Track Window already (where you can switch between Plugin, Envelopes, MIDI etc.). If the UI is hidden at the start, it can be loaded at the moment where the user switches the TAB.

Ultimatively, it is up to the users what they prefer. But whatever we do, it is a UI thing, so we can still change it easily...

@zynskeywolf
Copy link
Contributor

In any case there must not be another step to open the ui just because it's external. The way vestige works is horribly unintuitive and lv2 needs to move away from that approach. Plugin handling must be transparent and the user should get the same experience regardless of the plugin format.

@michaelgregorius
Copy link
Contributor

In any case there must not be another step to open the ui just because it's external. The way vestige works is horribly unintuitive and lv2 needs to move away from that approach. Plugin handling must be transparent and the user should get the same experience regardless of the plugin format.

It might not be the scope of this pull request but in general I agree with what @zynskeywolf wrote. Ideally all external plugins should feel like entities of their own and the parameter handling should be transparent to the users (also with regards to plugin type, i.e. VST, LV2, CLAP, etc.). Many other DAWs just present you with a list of the available plugin parameters. If you select a parameter from that list you can then define the automation curve for it, etc.

I once did some investigation with regards to some problems with plugin parameters and if I remember correctly, what I found was that LMMS seems to automate the movement and changes of the GUI elements and not of the parameters themselves. If that is really the case then this would need to be changed first before better and more uniform parameter handling could be implemented. If I understand correctly then this is also the reason why some extra parameter widgets need to be shown for the parameters in the first place. If the automations worked directly on the plugin parameters then the would not be needed.

@JohannesLorenz
Copy link
Contributor Author

In any case there must not be another step to open the ui just because it's external. The way vestige works is horribly unintuitive and lv2 needs to move away from that approach.

I agree here, though it is indeed a bit overhead to load the plugin UI. My idea is to have a tab widget to switch between both UIs. If the user previously selected the plugin UI, the next plugin will also let the TabWidget show the plugin UI. If they previously used the LMMS UI (the parameters), they get this when they open a plugin the next time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

7 participants