search

mikeoliphant / neural-amp-modeler-lv2

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

Switching to a preset with an empty model does not clear the current model #42

Closed mikeoliphant closed 1 year ago

mikeoliphant commented 1 year ago

When a model is loaded, switching to a preset with no model set will not clear the current model.

mikeoliphant commented 1 year ago

Reaper seems to only get the model path correct on a preset switch if the path is updated in restore().

It only seems to update the path based on an explicit patch_Get, which it seems to do right after a restore (but before any background work processing can happen). It seems to ignore any patch_Set coming at a different time.

mikeoliphant commented 1 year ago

Resolved with commit https://github.com/mikeoliphant/neural-amp-modeler-lv2/commit/33a0c083278c2abed3a4ce202597d6ea4d5a8d7f

rerdavies commented 1 year ago

Fabulous! I reviewed the changes. They seem to address all the of concerns and issues I had.

Please reject my pull request and close the associated issue. (Huge sigh of relief).

A VERY minor point of concern. At line 135 of nam_plugin.cpp, the code reads:

result = LV2_WORKER_ERR_UNKNOWN;

While it is true that the load has failed, I'm not sure it's a good idea to tell the host that the call failed. You've already logged an error, and there is nothing the host can or should do to improve the situation. At best, you're going to get an ERROR along the lines of "Unexpected error while running a background task." What ever it is, it's going to be a message that is confusing to the user, and unhelpful to you, since you've already logged a good error message. And it's the sort of error that a host must log: very unexpected, and potentially an indication that things are about to go sidewise very soon.

Not truly a crushing issue. If it doesn't get fixed, I'm not that worried. But you might want to consider slipping it in to a future release, especially if you're in the neighborhood of that code.

Thank you!

mikeoliphant commented 1 year ago

While it is true that the load has failed, I'm not sure it's a good idea to tell the host that the call failed.

That seems reasonable.