-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
fixes #6354: Sample and Hold for LFO Controller #6850
Conversation
LFO controller's "white noise" wave shape didn't respect the frequency knob at all, so Sample-and-Hold was added to extend the functionality of the LFO Controller with this random waveshape. The original functionallity can still be accessed by setting the FREQ knob to minimum (0.01)
Old projects using the noise LFO might sound different with this change applied, so maybe an upgrade routine could be added to check for LFOs whose shape is set to noise and reset the FREQ? This is trickier to solve if the waveshape is automated, especially if the frequency ks automated as well, but I think that should be rare enough to ignore. |
So this is how it's done, thank you for implementing this! I'm the one who fixed S/H for the instrument LFO but I failed miserably on the LFO Controller. I think it's perfectly reasonable to make it part of the white noise and not a separate button as in the instrument LFO.
I would skip an upgrade routine in this case. Because of the previous lack of S/H, the white noise has probably been used very little with the LFO Controller, so I don't feel a need for an upgrade routine at all. I'll leave that decision to the rest of you. |
@zonkmachine I think @Spekular may be correct about the upgrade routine, just to set FREQ to 0.01 so the project wont audibly change. I'll look at the upgrade routine in a bit. |
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.
Tests fine, I love this! I added some remarks on coding style. Not a hard block from me but others are more strict on this. As for an upgrade routine this sound unnecessarily complicated because of the very nature of white noise but I'm leaving the decision of this to the rest of the devs. It's a 'go' form me.
Sounds reasonable enough. |
Fixed some style issues
For the upgrade routine I'm going to have to figure out how to check out this PR, edit files, and repush. @zonkmachine or @Spekular you wouldn't happen to know how to do that off hand would you? |
You check out the branch like before and commit your changes. Then git push to the same branch as the original commit. If you change the history of the branch like rebasing it against master, you need to do a force push (git push -f). |
Added upgrade routine for old projects using the white noise on LFO controllers
@zonkmachine That should be the upgrade function plumbed in. |
I tested the upgrade routine and it works well. You can make setups that wont upgrade such as controlling the frequency of the noise by a second LFO but I don't think we should cover that. |
The sample and hold functionality seems to work fine, though I haven't tested the upgrade routine. |
Updates the upgrade routines to match other changes in similar PRs Co-authored-by: Kevin Zander <[email protected]>
Co-authored-by: Kevin Zander <[email protected]>
Changed .size() to .length() in data upgrade routine per sakertooth's comment and for consistency with other PR that touches other data upgrade routines
Co-authored-by: saker <[email protected]>
fixes #6354
LFO controller's "white noise" wave shape didn't respect the frequency knob at all, so Sample-and-Hold was added to extend the functionality of the LFO Controller with the "white noise" wave shape.
The original functionality can still be accessed by setting the FREQ knob to minimum (0.01), so this is purely a feature extension.