mikeoliphant / neural-amp-modeler-lv2

Neural Amp Modeler LV2 plugin implementation
GNU General Public License v3.0
202 stars 25 forks source link

Pipedal could not load plugin. I had to change the .ttl file. #27

Closed 38github closed 1 year ago

38github commented 1 year ago

This is what I got when looking at journalctl:

error: /usr/lib/lv2/neural_amp_modeler.lv2/neural_amp_modeler.ttl:102:73: expected prefixed name
error: /usr/lib/lv2/neural_amp_modeler.lv2/neural_amp_modeler.ttl:102:73: expected `]', not `;'
lilv_world_load_file(): error: Error loading file `file:///usr/lib/lv2/neural_amp_modeler.lv2/neural_amp_modeler.ttl'

I opened the neural_amp_modeler.ttl and the end of the file looked like this:

    state:state [
            <http://github.com/mikeoliphant/neural-amp-modeler-lv2#model>;
    ].

When I changed it to the following it worked:

    state:state [
            <http://github.com/mikeoliphant/neural-amp-modeler-lv2#model> <empty> ;
    ].

I know almost nothing about coding so please let me know if my insertion of "< empty >" is wrong and might cause issues.

mikeoliphant commented 1 year ago

Yeah - that field is for the model default value. I removed it, since there is no default value. I'm not sure what the technically correct way to handle this is. Maybe @rerdavies has some insight?

rerdavies commented 1 year ago

@drobilla would be the ultimate authority on such questions. (principal of the LV2 spec, and prime developer of the lilv/lv2 suite of libraries.

But here's my take. And I feel a bit guilty asking him to make hard decisions after having done a magnificent job of championing a difficult set of features in a way that has its pain points, but is infinitely better than VST3. Hopefully, I'm exactly right, and Dave will only have to leap in if I'm seriously wrong.

See my important disclaimer with respect to my state of knowledge with respect LV2 states and presets [1]. PiPedal leverages libraries that provide partial implementations of features that are being discussed.. My understanding of these libraries is evolving. And see my important disclaimer with respect to probable bugs in PiPedal [2], PiPedal may not be right, and is very wrong in one particular case.

With that in mind, I would recommend:

 state:state [
            <http://github.com/mikeoliphant/neural-amp-modeler-lv2#model> "" ;
    ].

and check that state restore code accepts either an ATOM_Path or an ATOM_String. You might also want to check that the State save code saves the value as ATOM_String, not ATOM_Path. While the type of the Patch Property must be ATOM_Path, there's nothing that says that the state variable has to be the same type. Using a string here avoids complex issues with round-tripping of filenames, and makes it trivially easy to say "there isn't one". And, most important of all, it's not WRONG!

Omitting the value altogether is a syntax error in TTL files. It rightly fails while parsing the TTL file. It's a syntax error; and any sensible host should have nothing to do with it. Nothing good is going to happen from that.

This seems like an attractive option; but it's definitely wrong:

 state:state [ #WRONG!!!
    ].

The native lilv implementation does not make any state restore call at all if the state is empty; and PiPedal does not either. As a result, your plugin wil have no basis on which it can choose to set the model back to unselected state.

I'm not sure what , means in a turtle file or whether or how hosts are going to treat this. I, personally, think it's a Turtle IRI (a URL enclosed in <>), which is not what you want. But the actual translation would be done by serd. You'd have to try it and find out what actually happens. My prediction: on a functioning host, you'll get "/usr/lib/lv2/blah.ttl/empty". This would be a feature of Turtle TTL, and occurs at TTL parse time as a consequence of the fact that a Turtle Interpreter will convert the relative path "empty" to an absolute path, relative to the preset file.

My guess (although I'm too lazy to run it down) is that a Turtle equivalent of NULL would look something like "NULL"^^xsd:null; but I don't think that's actually right. And I have no idea how serd would translate that to an LV2 Atom. (The ever mysterious ATOM__Blank, maybe?) So, being lazy, I can't recommend that either. (But PiPedal will do the right thing, and just blindly transcode the atom it's given by the lilv preset loader). +1 for probably not wrong; -1 for "if only we could figure out what the syntax is".

Also, I think saving ATOM_Path properties in state variables should be firmly AVOIDED. There's a serious ambiguity in the Turtle spec with respect to how to convert relative paths, and casually, lilv/serd appears to violate the current Turtle specification with regard to use of %-encoding of URLs (lilv should not). And it's improbable that hosts are going to do the right thing once preset files start moving around, or when users start providing user-selected file paths. ATOMPath file paths stored in TTL-based preset files will be encoded, decoded, transcoded and converted to absolute paths in ways that are not predictable. I can't remember which way I left your code; if it's using ATOM)Path, it should be converted to save ATOMStrings, and load ATOMString or ATOM__Path (for backwards compatibility).

As I read the lilv implementation, saving a path into a state property with type ATOM_Path does not carry the same meaning as it does when you declare a Patch property to be of type ATOM_Path. So don't. (The former says "transcode a URL, and make it absolute when you load or reload it"); the latter says (this really IS a filename, which is absolute, and a file dialog would be a Very Good Thing for it if you have one, and I use the MapPath feature to ensure trasnportability). Fortunately, the state property doesn't have to have the same type as it's corresponding patch property.

The simple solution: use ATOM_String variables, which will (probably) round-trip transparently, even across machines and locales. Pipedal avoids this issue because it stores user presets in JSON files instead of TTL files. Your mileage will vary on other hosts -- not all of which, as you know, make any attempt to implement this feature. So you would have to conduct your own tests, and make perhaps make some hard decisions.

The one exception, ironically, is when loading factory presets, and default presets, where it may be useful to exploit the conversion of relative paths to absolute paths at load time in order to reference files in the plugin's own resource directory. so <Click.wav> was actually pretty good, save for the fact that you didn't have one.


[1] My understanding of this part of LV2 is far from perfect, although it's evolving. I am just emerging from a really nasty battle with presets in PIpedal and TooB plugins. So I have some insights, but probably imperfect knowledge. PiPedal uses a set of llv2 libraris (lilv, serd, sord) written by David Robillard, author of the LV2 specification; but these libraries put an awful lot of details especially with respect to state and presets, in the hands of host applications like PiPedal.

[2] PiPedal has just (today) added proper support for paths and state in factory preset files (it was broken in previous versions), But pipedal has not yet implemented default presets. I know. Your TTL file said it was a mandatory feature; and I ignored it. So,...

PiPedal's support of this specific feature is evolving, and quite possibly deficient in some respects. I am currently in a position to properly implement loading a default preset now. And the current version of PiPedal (but not the previous one) will properly recognize and load non-default presets that you declare. , If you really need that feature, let me know, and I will implement it expeditiously.

mikeoliphant commented 1 year ago

I suspect the "correct" answer is probably "whatever works and doesn't crash any hosts"...

rerdavies commented 1 year ago

I think "" is pretty safe.

On Wed, Jun 7, 2023 at 10:22 PM Mike Oliphant @.***> wrote:

I suspect the "correct" answer is probably "whatever works and doesn't crash any hosts"...

— Reply to this email directly, view it on GitHub https://github.com/mikeoliphant/neural-amp-modeler-lv2/issues/27#issuecomment-1581792103, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACXK2DANCWDW42R2A5WYX5DXKEZN7ANCNFSM6AAAAAAY6FH6KM . You are receiving this because you were mentioned.Message ID: @.***>

mikeoliphant commented 1 year ago

"" seems to not cause problems with other DAWs, so I'll call that a win. Thanks for the help.

Resolved via commit https://github.com/mikeoliphant/neural-amp-modeler-lv2/commit/8aff7658e8458eedaf9e7379e4a94e90bf3d0ed8

drobilla commented 1 year ago

In general, you can't have a statement without an object (the last field, or "value") like that. I don't entirely understand what this state normally looks like when there is something there or how it's used/implemented (I'm lazy), but the least weird thing to do would be to just leave the statement out entirely. If it's the only one in the state, then you can just leave that whole thing out entirely as well.

If this is usually a file URI, it's... not invalid to put a string there, but it's a bit lax and means you have more strange cases to deal with, technically the property supports both URIs and strings then, which is fine but more complicated. You could also simply use an empty blank, like "[]". If it does indeed support JSON strings there normally, then an empty string seems fine. Alternatively, if it's always a URI, you can just invent some sentinel and handle that specially, myprefix:noModel or whatever.

Definitely don't invent weird stuff like "NULL"^^xsd:null, that's going to be a bad time.

mikeoliphant commented 1 year ago

the least weird thing to do would be to just leave the statement out entirely. If it's the only one in the state, then you can just leave that whole thing out entirely as well

That seems to work (thanks!) and seems like the best way to go.