Skip to content
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

Merged
merged 7 commits into from
Sep 12, 2023

Conversation

consolegrl
Copy link
Contributor

@consolegrl consolegrl commented Sep 7, 2023

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.

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)
@Spekular
Copy link
Member

Spekular commented Sep 7, 2023

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.

@zonkmachine
Copy link
Member

zonkmachine commented Sep 7, 2023

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. This is actually better so maybe that one should be changed similarly? Edit: wrong, there is no pure white noise on that LFO.

This is trickier to solve if the waveshape is automated,

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.

@consolegrl
Copy link
Contributor Author

@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.

Copy link
Member

@zonkmachine zonkmachine left a 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.

src/core/LfoController.cpp Outdated Show resolved Hide resolved
src/core/LfoController.cpp Outdated Show resolved Hide resolved
@zonkmachine
Copy link
Member

just to set FREQ to 0.01 so the project wont audibly change

Sounds reasonable enough.

Fixed some style issues
@consolegrl
Copy link
Contributor Author

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?

@zonkmachine
Copy link
Member

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
@consolegrl
Copy link
Contributor Author

@zonkmachine That should be the upgrade function plumbed in.

@zonkmachine
Copy link
Member

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.

@DomClark DomClark linked an issue Sep 7, 2023 that may be closed by this pull request
src/core/DataFile.cpp Outdated Show resolved Hide resolved
src/core/LfoController.cpp Outdated Show resolved Hide resolved
@messmerd
Copy link
Member

The sample and hold functionality seems to work fine, though I haven't tested the upgrade routine.

consolegrl and others added 3 commits September 11, 2023 00:41
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
@zonkmachine zonkmachine merged commit b64912c into LMMS:master Sep 12, 2023
@consolegrl consolegrl deleted the sample-and-hold branch September 12, 2023 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better functionality of LFO-ctrl with random spectrum
6 participants