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

Add Phaser effect to LMMS #5168

Closed
wants to merge 32 commits into from
Closed

Conversation

LostRobotMusic
Copy link
Contributor

@LostRobotMusic LostRobotMusic commented Sep 4, 2019

(Awesome GUI designed by Roxas)

Introducing the Phaser, the ultimate Flanger companion! :)

Here are the changes made to the Flanger:

  1. Allow negative feedback values (I'm surprised it wasn't an option already)
  2. Add a phase knob to change the stereo LFO phase (so you can have a mono flanger, for example)
  3. QuadratureLfo was combined into one file, because the CPP file had almost nothing in it and QuadratueLfo needed to be included by the Phaser.

All three changes are 100% backwards-compatible.

Here's a screenshot of the new Phaser:
image

Here's an overview of its features (EDIT: This is out of date, I make some updates in some comments below):

  1. For those who don't know, a phaser is an allpass filter with an LFO at a partial wet/dry, which very commonly has an adjustable number of allpass filters as well as a 1-sample delay feedback option.
  2. This Phaser can have as few as 1 and as many as 32 allpass filters, adjustable using the filter order spinbox.
  3. The LFO has an adjustable rate (with optional tempo sync), amount, and stereo phase.
  4. Phaser is the first-ever animated LMMS effect to my knowledge (unless you want to count the volume meters in the fader in the Delay effect...). The two dots at the bottom display the allpass filter frequencies in a very intuitive way, with the green dot showing the left ear and the blue dot showing the right ear.
  5. Unlike most phasers (surprisingly), the LFO can be disabled, allowing the user to automate the cutoff knob to have full control over the Phaser. This Phaser also comes with adjustable resonance, which is also a rare feature for reasons I'm unaware of.
  6. This Phaser allows positive and negative feedback values, which are an essential feature for most phasers. Also, unlike any other phaser I've seen, the feedback delay can be adjusted to values other than just one sample (which I've noticed sounds very good with negative feedback).
  7. I have also added an input follower, which changes the filter frequencies depending on the input volume. The Attack and Release knobs change how quickly that value fluctuates.
  8. And, as is obvious, you can control the input/output volumes with the faders, and there is a built-in Wet/Dry knob as well with a default value of 50% (mixing the dry with the wet is very important for phasers).

I should also mention that because so many filters need to be used, I very aggressively optimized the allpass filters to the point that they only need an impressive two additions, two subtractions, and two multiplications each(!!). Most Phaser controls are also sample-exact, which is a fun bonus.

I'm excited for everybody to get their hands on this tool. It sounds very cool, and can easily add some deliciously vocal characteristics to your sounds.

plugins/Phaser/Phaser.h Outdated Show resolved Hide resolved
plugins/Flanger/QuadratureLfo.h Outdated Show resolved Hide resolved
plugins/Flanger/QuadratureLfo.h Outdated Show resolved Hide resolved
@PhysSong
Copy link
Member

PhysSong commented Sep 4, 2019

I think the changes in the flanger plugin should be decoupled before merging this. Also, I think Phaser.h and Phaser.cpp can be renamed to PhaserEffect.h and PhaserEffect.cpp

@LostRobotMusic
Copy link
Contributor Author

@PhysSong Alright, I fixed all of those.
When you say you want the Flanger changes separated from the Phaser changes, does that include the QuadratureLfo changes? The Phaser relies on the changes to the QuadratureLfo files, which is why I put it all in one PR.

@PhysSong PhysSong added needs code review A functional code review is currently required for this PR needs style review A style review is currently required for this PR needs testing This pull request needs more testing labels Sep 12, 2019
@LmmsBot
Copy link

LmmsBot commented Nov 24, 2019

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

Windows

🤖
{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://13193-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.116%2Bg18e4803-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13193?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://13194-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.116%2Bg18e4803bf-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13194?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://13195-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.116%2Bg18e4803bf-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13195?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/xfcx71chqlpov5c2/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/38404371"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/9xf1893l5rgei9b8/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/38404371"}]}, "commit_sha": "f67954282dfc68f384501c77be51fa47fb7cb559"}

@LostRobotMusic
Copy link
Contributor Author

All merge conflicts and bugs have been removed. Feel free to test now.
Here's a video of me messing around with this effect: https://youtu.be/hoD1MKwEH9k

@LostRobotMusic
Copy link
Contributor Author

I just added some major enhancements to this Phaser effect, including both QOL tweaks and some new features. Here are some of the changes, though I'm sure I won't remember them all...

  • Changed one of the constants to drastically improve the input followers attack/release curves
  • Added an "Invert" button which inverts the wet signal. This effectively shifts the signal by 180 degrees, inverting the notches and peaks created by the phaser when combined with the dry signal.
  • Added a "Wet" button which isolates the wet signal
  • Added some gentle waveshaping distortion to the feedback loop to prevent infinite feedback
  • Added a distortion knob which controls a full-wave rectifier in the feedback loop.
    Why would I put a full-wave rectifier in a feedback loop?
    1. I needed a distortion type that made a noticeable change
      to the sound that wouldn't result in infinite feedback loops.
      An overdriven symmetrical distortion wouldn't work for that
      reason.
    2. I just tried this strange idea as an experiment, and,
      surprisingly, it sounded fantastic.
  • Commented the code a bit better, including a diagram of what's going on.
  • Some style changes I missed
  • Rewrote how the filter feedback works in a way that allows 0-delay feedback. I got the idea from this book: https://www.native-instruments.com/fileadmin/ni_media/downloads/pdf/VAFilterDesign_1.1.1.pdf
  • Made the feedback delay be in milliseconds rather than samples, making it smoother to automate, and 100% sample rate change compatible.

This is 100% ready for review, then merge. I do not intend to make any other changes to it feature-wise, unless you guys have any suggestions.

@PhysSong
Copy link
Member

@douglasdgi Your last commit changed the permission of some files from 644 to 755 accidentally.

@LostRobotMusic
Copy link
Contributor Author

LostRobotMusic commented Feb 17, 2020

@PhysSong Oh, that got messed up when I made a backup of my files before reinstalling my OS. I didn't realize changing permissions mattered with this stuff. I might have a few questions about fixing that, I'll message you about it on Discord if I need help (later when I have time).

@LostRobotMusic
Copy link
Contributor Author

LostRobotMusic commented Feb 17, 2020

Welp, that was a mess... somehow all of the files in my backup got set to 755 and I have no idea how. I managed to copy all of the file permissions over from an older backup (older by like two days), I think that fixed everything.
I also noticed a bug in my code where resetLFO and changeSampleRate weren't labeled as slots so they didn't do anything, I fixed that.
I... think everything's okay now? Hope everything's okay now?

Copy link
Member

@DomClark DomClark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the code looks good; I haven't checked that the DSP code does what it ought to - I believe you know what that is better than I do.

I think there ought to be a nicer way to share code between plugins than the ugly #include "../other_plugin/header.h", but that's out of scope here.

plugins/CMakeLists.txt Outdated Show resolved Hide resolved
plugins/Flanger/FlangerControls.cpp Outdated Show resolved Hide resolved
plugins/Flanger/FlangerControls.cpp Outdated Show resolved Hide resolved
plugins/Phaser/PhaserControlDialog.cpp Outdated Show resolved Hide resolved
plugins/Phaser/PhaserControlDialog.cpp Outdated Show resolved Hide resolved
plugins/Phaser/PhaserEffect.h Outdated Show resolved Hide resolved
plugins/Phaser/PhaserEffect.h Outdated Show resolved Hide resolved
plugins/Phaser/PhaserEffect.h Outdated Show resolved Hide resolved
plugins/Phaser/PhaserEffect.h Outdated Show resolved Hide resolved
plugins/Phaser/PhaserEffect.h Outdated Show resolved Hide resolved
plugins/Phaser/PhaserControls.cpp Outdated Show resolved Hide resolved
@zonkmachine
Copy link
Member

Testing it and it's brilliant!

I think the Envelope Follower should have an enable button like the LFO.
What does +/-30 octaves mean on the Envelope Follower Amount knob?

plugins/Phaser/PhaserControls.cpp Outdated Show resolved Hide resolved
plugins/Phaser/PhaserControls.h Outdated Show resolved Hide resolved
plugins/Phaser/PhaserEffect.cpp Outdated Show resolved Hide resolved
plugins/Phaser/PhaserEffect.cpp Outdated Show resolved Hide resolved
plugins/Phaser/PhaserEffect.cpp Show resolved Hide resolved
plugins/Phaser/PhaserEffect.cpp Outdated Show resolved Hide resolved
plugins/Phaser/PhaserEffect.cpp Outdated Show resolved Hide resolved
plugins/Phaser/PhaserEffect.cpp Outdated Show resolved Hide resolved
@LostRobotMusic
Copy link
Contributor Author

Alright, I think that covers everything.

What does +/-30 octaves mean on the Envelope Follower Amount knob?

@zonkmachine +1 octave means the input follower detecting an input volume of 0 dbFs (taking attack/release into account) will increase the Phaser frequency by 1 octave.
Also, I don't know how to resolve your "requested change", am I supposed to "dismiss" it or do you have to do something on your end? I'm still learning Github, heh.

I think the Envelope Follower should have an enable button like the LFO.

Done. Good suggestion, I never bothered adding one since setting the Input Follower amount to 0 does the same thing, but that's rather inconvenient.

@PhysSong
Copy link
Member

I got a ton of uninitialized variable errors while testing with Valgrind because QuadratureLfo constructor doesn't initialize m_phase and m_frequency. I got them from the existing Flanger plugin, too. It seems like the problem existed before you touched anything, though.

@LostRobotMusic
Copy link
Contributor Author

@PhysSong Just changed it so m_frequency and m_phase are set to 0 in the constructor, I assume that fixes the issue.

@curlymorphic
Copy link
Contributor

This is a really nice update, the old flanger has a new lease of life and sounds much better, it's not often that 3Osc can make me smile on the default patch, but with your upgrades to the flanger I did. I have to be honest, testing this PR has made me somewhat embarrassed at the horrid sound made when adjusting the delay knob, something I shall be looking into.

The Phaser, nice work, the UI look clean and well laid out, sounds good, and is fun to play with. This will be a nice new addition to LMMS.

I think the changes in the flanger plugin should be decoupled before merging this.

I think this is very important, we have 2 very distinct updates in this one pull request. This makes reviewing, testing, maintenance and commits logs very messy. I don't think it will take too much effort to separate these into individual pull requests, I am happy to provide any assistance you want.

My proposal would be to start a new PR with the Flanger updates, due to the phaser being dependant on these changes. I would take the opportunity to refactor any DPS classes that are used in both plugins and relocate them within the core codebase, in preparation for the Phaser. I feel this would mainly be a copy-paste exercise, so all the previous work put into reviews will remain valid, I may have a few code comments but let's get the code separated first.

Progression onto the Phasor will then be a lot simpler, as I am hoping the number of files changes drops from the current significant 29 to a more manageable level. We can look at the details as soon as the flanger PR is complete.

@PhysSong
Copy link
Member

PhysSong commented Jan 4, 2021

@curlymorphic I extracted the Flanger changes to #5873.

@LMMS LMMS deleted a comment from rdrpenguin04 Jan 24, 2021
@PhysSong PhysSong dismissed zonkmachine’s stale review January 24, 2021 02:43

The review comments are addressed.

@PhysSong
Copy link
Member

Due to the changes in #5873, this PR needs to be updated.
@douglasdgi Do you need some help, or you can handle the conflict? Please see #5168 (comment) if you want to try it yourself.

@PhysSong PhysSong changed the title Add Phaser effect to LMMS, and add minor upgrades to Flanger Add Phaser effect to LMMS Mar 25, 2021
@PhysSong
Copy link
Member

@curlymorphic Do you want to re-review this PR?

@curlymorphic curlymorphic self-requested a review March 25, 2021 07:35
Copy link
Contributor

@curlymorphic curlymorphic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My previous comment about spitting this pull request, the flanger changes being separated from this new plugin, but more active devs may have a different opinion.

plugins/Phaser/PhaserEffect.cpp Outdated Show resolved Hide resolved
}
if (analog)
{
firstAdd[0] = tanh(s[0] * analogDist) / analogDist + tanh(tempAdd[0] * analogDist) / analogDist;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is the aliasing being limited, non linear processing such as tanh always produces content higher in the frequency spectrum, and measures are required to reduce this, commonly oversampling / decimation are used but are expensive.

I feel if an analog distortion is added it should be done properly, not produce a digital-only sound such as, Aliasing. An alternative could be to implement this feature once lmms has adopted an oversampling / decimation library.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't called "analog" because the distortion is analog, it's called "analog" because having distortion inside of the feedback loop is a characteristic of analog audio hardware. Like how Moog filters work by adding nonlinearities (distortion) into the filter feedback loops.

That being said, I do agree that aliasing should be avoided... but I am of the opinion that oversampling should be handled by LMMS itself rather than individual plugins, and that's out-of-scope for this plugin. I think every plugin should come with oversampling options, just like how every plugin comes with a Wet/Dry knob.

So, unless you disagree, I don't think we should bother with putting oversampling into here. I didn't put it into any of my other plugins for the same reason. If we implement oversampling as an LMMS feature rather than a per-plugin feature, we'll never have to worry about it ever again... because, in reality, over 90% of the plugins in the entire catalog could benefit from oversampling, so adding it in on a per-plugin basis seems quite unnecessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aliasing is a very important fundamental concept in DSP, and has many reasons to be avoided, harmonic sounds become inharmonic, sound out of tune when played with other notes, fill the audio spectrum with unwanted content that makes final mixdowns muddy, and are an indicator of the quality of the product. I am aware that historically aliasing has not been of a high priority in LMMS, but I am not convinced adding to the problem would be constructive.

It is good practice to only oversample where necessary, due to the increased processing, If LMMS were to have a user option to oversample by plugin, this would be very inefficient due to the whole signal path in the plugin being processed multiple times, and expecting the artist to have an understanding of DSP and the consequences of setting the oversample rate. This is why I am not aware of any reputable DAW that takes this approach. It is always up to the plugin developer to handle anti-aliasing in an efficient manner.

That leaves the issue of how to progress with this plugin, specifically the "Analog distortion", I have a few suggestions, any one of these alone would solve this issue.

  1. Oversample and decimate the non linear processing (tanh).
  2. Rename the option from "Analog" to better describe the outcome, I would suggest a name that makes it clear that the aliasing is intended, as you have done elsewhere.
  3. Leave it as is, and hope no one notices or cares.

I shall leave the decision up to yourself, and shall not raise this issue again on this PR, unless asked.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that we should try to reduce aliasing, but there are some issues.

Oversample and decimate the non linear processing (tanh).

AFAIK, that add some delays around the non-linear processing. Wouldn't that affect the outcome in a way other than reducing aliasing?
Slightly out of scope, but I think filters may also leverage from oversampling for better frequency/phase response.

Rename the option from "Analog" to better describe the outcome

That might be a good choice, but I have no ideas on better names.

decreased the volume too much for my liking.
*/
tempAdd[0] = tanh(tempAdd[0] * 0.5) * 2;
tempAdd[1] = tanh(tempAdd[1] * 0.5) * 2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has the aliasing been considered? although I feel this tanh needs to remain, as it is much preferable to the infinite signal rise that could happen if removed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is just for protection, how about bumping up the limit to 4 or so?

plugins/Phaser/PhaserEffect.cpp Show resolved Hide resolved
@LostRobotMusic
Copy link
Contributor Author

This PR has been replaced by #6540.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs code review A functional code review is currently required for this PR needs style review A style review is currently required for this PR needs testing This pull request needs more testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants