DISTRHO / DPF

DISTRHO Plugin Framework
ISC License
672 stars 97 forks source link

Regression with lv2 presets #393

Closed zamaudio closed 1 year ago

zamaudio commented 1 year ago

Since we updated zamaudio/zam-plugins to dpf @ e840509 the lv2 presets no longer function in Ardour 7.1

The preset name is displayed in the drop down, but when you select it, nothing seems to happen.

Could I be missing a library as I upgraded my OS recently?

falkTX commented 1 year ago

this is a weird one since the host is the one responsible for loading presets and telling the plugin about it.

can you try in jalv? that would be a much simpler host to debug compared to ardour. thanks

zamaudio commented 1 year ago

It works in jalv.gtk, will report to Ardour

zamaudio commented 1 year ago

No, I was mistaken, it does not work in jalv.gtk, the preset works the first time, but then when you deselect it in jalv, tweak a knob and reselect the preset, it does not apply the changes to the controls...

falkTX commented 1 year ago

trying with urn:zamaudio:ZamCompX2 I am unable to reproduce, presets are recalled correctly.

do you perhaps have an old and new zam lv2 plugin install side by side?

zamaudio commented 1 year ago

No, I removed all traces of my plugins and tried my old 3.14 branch, it works fine. There is something wrong with either dpf (or my system lv2 stack).

zamaudio commented 1 year ago
Package lilv-0.24.14-3.fc37.x86_64 is already installed.
Package lilv-libs-0.24.14-3.fc37.x86_64 is already installed.
Package jalv-1.6.4-8.fc37.x86_64 is already installed.
Package lv2-1.18.8-2.fc37.x86_64 is already installed.
Package sratom-0.6.10-2.fc37.x86_64 is already installed.
Package serd-0.30.12-2.fc37.x86_64 is already installed.
Package sord-0.16.10-1.fc37.x86_64 is already installed.
Package suil-0.10.12-2.fc37.x86_64 is already installed.
falkTX commented 1 year ago

I am yet to update lv2 libs to latest, so maybe these libs are more picky now. but it doesnt really make a lot of sense, the preset ttl generation didnt change much or at all and using new dpf with old jalv works fine..

can you try with other plugins? amsynth has a bunch of presets

zamaudio commented 1 year ago

I made it print in lv2_plugin.cc in ardour when it restores the lilv state and then sets the values:

lilv_state_restore...
set_port_value (att=0.000000)
set_port_value (kn=0.000000)
set_port_value (mak=0.000000)
set_port_value (rat=0.000000)
set_port_value (rel=0.000000)
set_port_value (sidechain=0.000000)
set_port_value (slew=0.000000)
set_port_value (stereodet=0.000000)
set_port_value (thr=-nan)

It seems the port values are mostly zeroes when it comes to restore them.

zamaudio commented 1 year ago

I tried with another non-dpf plugin and it works fine.

x42 commented 1 year ago

it works when editing the presets.ttl and changing all values floating point numbers:

change e.g. pset:value 10; to pset:value 10.0 ;

zamaudio commented 1 year ago

@falkTX can you please fix dpf to append .0 to the preset values?

x42 commented 1 year ago

Since the value is an integer and not a float, in Ardour the following clause is entered:

    if (type != 0 && type != URIMap::instance().urids.atom_Float) {
      return;  // TODO: Support non-float ports                                                                                                                                                
    }
falkTX commented 1 year ago

this is perfectly valid ttl though, and used to work before too. I am not against changing it, but seems the wrong fix.

x42 commented 1 year ago

Yeah we'll have to port the following code to Ardour https://gitlab.com/drobilla/jalv/-/blob/master/src/state.c#L134-142

x42 commented 1 year ago

this is perfectly valid ttl though, and used to work before too.

yes. It is valid ttl, but integers in preset or state files never worked in Ardour. strictly speaking it is also wrong to set a float control port using an integer value.

falkTX commented 1 year ago

this is perfectly valid ttl though, and used to work before too.

yes. It is valid ttl, but integers in preset or state files never worked in Ardour. strictly speaking it is also wrong to set a float control port using an integer value.

this is a text file though, not a memory data type. the lack of decimal point helps to indicate the value is meant as integer. the .0 all over the values is a tiny bit wasteful too.

and actually for MOD we went all the other way around. if port is tagged as integer, having a .0 suffix is seen as a warning because it is superfluous and can be a sign of buggy behaviour somewhere.

I recall drobilla mentioning this on #lv2 at some point, or something related to it, makes me think this is a lilv update triggering the breakage and maybe even intentional. nasty that it happens though, as we now have to test every single preset file on every plugin for the supposedly missing .0 suffix, really not great...

so I am going with the stance: I will adjust the behaviour in dpf if lv2_validate, lv2lint or other validation tool is able to catch this. until then, I prefer not to change this as a way to force a proper fix in the right place. this change is not DPF specific, so the fix should not be DPF specific either.

falkTX commented 1 year ago

btw the jalv code you mentioned is 10 years old https://gitlab.com/drobilla/jalv/-/commit/ac68a26a30d9f6917da9c2cab29b6f612648c4d0

I have an old jalv (but not 10 years old) and presets work here. Damien has new jalv and presets do not work there. So the issue I assume is not that code section, but somewhere else.

x42 commented 1 year ago

yes, we know, and the code in Ardour around 9 years https://github.com/ardour/ardour/commit/adf40b97e061150c0a7830c2591728b0ee72d5b4

PS. and the issue was just fixed in https://github.com/Ardour/ardour/commit/27dfd8a7e373716fb449e1e74a9dcf699440aa3b

falkTX commented 1 year ago

this is the first time I see such a report though, I really think there is a deeper issue here. as pointed out by Damien, dpf presets are not working in jalv and that has the various lilv state types implemented since ever.

x42 commented 1 year ago

@zamaudio are you sure it fails/failed in jalv?

As discussed on IRC it worked in jalv both on mine and drobilla's machine (but failed in Ardour for obvious reasons). It also only affected state files (not initial values or ranges from the plugin.ttl). I don't see a deeper issue, can you elaborate what you think the deeper issue is?

x42 commented 1 year ago

No, I was mistaken, it does not work in jalv.gtk, the preset works the first time, but then when you deselect it in jalv, tweak a knob and reselect the preset, it does not apply the changes to the controls...

That seems to be a different issue.

zamaudio commented 1 year ago

I had different versions of jalv.gtk on my system confusing me. Presets work in jalv.gtk with system lv2 libs.

zamaudio commented 1 year ago

And with Ardour/ardour@27dfd8a7e3 it works in ardour too now

zamaudio commented 1 year ago

I am going to close this now, it does not seem to be a dpf bug.