Notification port output is incorrect.

rerdavies commented 1 year ago

A series of problems with notification output.

atom_forge is not initialized at the start of each cycle.
Plugin MUST write at least an empty sequence into the notification port on every process call.
patch_get response MUST be wrapped in an atom_sequence.
Plugin SHOULD write a state_changed event into the notification port after the new model is transferred back onto the main thread. (the host cannot assume that writing a property changes the state of the plugin.

I have a pull request that addresses all of these issues (plus reducing compiler warnings to a functional level, and fixing a broken debug build on GCC/Linux. But I don't have permissions to make a pull request. (Either that or I'm doing it wrong). I get a permission error when I try to create a pull request.

btw, I used your (fixed) plugin as a test target for an lv2:property/atom:path file dialog UI in the PiPedal project (https:://github.com/rerdavies/pipedal). So you can tell the reaper guys that if PiPedal can provide a file dialog for you, then they bloody well should as well. :-P. A version of pipedal that implements file dialogs is going up shortly (a day or two). But you'll need my fixes first.

If you would prefer it, I can create a fork, and you can look at the changes and decided what you want to do with them.

Your choice.

rerdavies commented 1 year ago

I think I'm probably doing it wrong. Do I have to fork first to make a pull request? I've never done this before.

mikeoliphant commented 1 year ago

@rerdavies Those changes sound welcome. The LV2 api is (imho) a hot mess - way too much finicky error-prone stuff to accomplish basic functionality. And I thought Vst3 was bad (well, it is, just bad in different ways)...

No special permissions are required to submit a PR - maybe you were trying to commit directly to my repo?

The best way to handle things, generally, is for you to fork this repo, create a branch in your copy of the repo, commit your changes on your branch, and then submit that branch as a PR to my repo. Having a branch on your end isn't required, but it generally makes thing easier to manage.

rerdavies commented 1 year ago

Sorry for the delay. I had a source control accident. :-/ Pull request has been posted. Let me know if there are any problems.

LV2 is indeed a hot mess. lol.

mikeoliphant commented 1 year ago

Resolved with PR https://github.com/mikeoliphant/neural-amp-modeler-lv2/pull/17

mikeoliphant / neural-amp-modeler-lv2

Notification port output is incorrect. #15