-
Notifications
You must be signed in to change notification settings - Fork 144
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make NAM disablable - bypass dry signal #494
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very nice first attempt. Thanks for your work! But, there's a number of things that I think I've caught that would introduce bugs (see comments).
[This PR is also a nice reminder that I should work on a better PR template so that I can think through the sorts of changes that happen so that contributors can work through a checklist to make sure their contributions take into account all of the considerations.]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few things to check, but first let me do that version bump for you.
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. |
This PR implements #431.
Changes:
"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.