sdatkinson / NeuralAmpModelerPlugin

Plugin for Neural Amp Modeler
MIT License
1.99k stars 133 forks source link

Make NAM disablable - bypass dry signal #494

Closed marmal95 closed 6 days ago

marmal95 commented 2 months ago

This PR implements #431.

Changes:

  1. New UI control added - clickable "NEURAL AMP MODELER" logo title.
  2. Clicking logo toggles NAM state between enabled/disabled. Title color also changes between white/black - depending on state.
  3. Dry signal is passed to output directly when NAM is disabled - meters are still showing audio levels.
  4. All UI controls get disabled when whole NAM is disabled.
  5. Mouse events on switches are ignored to prevent enabling noise gate or EQ section. Although it would not impact the signal, disabling them make their state consintent with the whole NAM state and make it more readable for user that those signal chain elements are bypassed.
  6. Knobs are still editable in disabled state - they do not impact the signal until NAM is enabled again.

"NEURAL AMP MODELER" logo in now clickable. It changes color between white and black - depending if NAM is enabler or disabled. UI controls are disabled when whole NAM was disabled - dry signal is passed to output.

nam_disabled
sdatkinson commented 6 days ago

Ok. Thanks for your patience with the long wait.

I want to get this in before #525 so let me see what I can do to shape up anything I need to then get this merged hopefully if nothing big pops up. I'll take care of the unserialization in #526--I'll have more to pile into it thanks to #525 anyways.

sdatkinson commented 6 days ago

Ok. I pulled your branch down to have a look and do some checking.

The punchline is that I don't think that I'm going to accept this PR, unfortunately. But let me explain why.

The core problem is complexity, in short--this "global" switch has a lot of interactions with a lot of things across the whole plug-in. The problem with that is that it opens up the possibility for a lot of bugs due to unforeseen interactions with other parts of the plugin.

For example, when you bypass the plugin by clicking on the title, then reactivate it by clicking a second time, the current implementation enables the output normalization switch. That's a potential problem because if there's a model that's loaded that doesn't have output loudness, then that switch should stay disabled--there are two different inputs to determine the state of that control.

The way to resolve this particular issue itself isn't too hard--if the global bypass is not active and either (A) there's no model loaded or (B) there's a model loaded and it has an output loudness, then set the output normalization switch to non-disabled. I actually started implementing this and refactoring. But then I started running into other problems and stopped since this sort of confirms my worry.

My worry is that this is only one problem, and there might be others. Identifying and verifying that all of them are functioning as intended is this combinatorially-large state space to check. It's not too bad right now, but it would only get worse as more features are added (and adds another thing that needs to be checked every time a feature is added).

What I'm foreseeing is a large amount of time getting sunk into trying to continually debug functionality related to this feature--in addition to this PR, I'm going to be on the hook to maintain it into the future--which is the real deal-breaker. I don't think I can do that.

I get that this feature is potentially useful to some folks of people, so let me try to suggest some alternatives:

Hopefully this helps, and again, thanks for putting together this PR. It wasn't obvious to me what the full implications of this feature might be, but now that I've got a little better idea with it in front of me, I do think that it's the right thing to do to pass on it for the repo that I'm maintaining.