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

Upgrade the Flanger effect #5873

Merged
merged 1 commit into from
Mar 25, 2021
Merged

Upgrade the Flanger effect #5873

merged 1 commit into from
Mar 25, 2021

Conversation

PhysSong
Copy link
Member

@PhysSong PhysSong commented Jan 4, 2021

Originally authored by @douglasdgi in #5168.
Summary of changes:

  1. Allow negative feedback values
  2. Add a phase knob to change the stereo LFO phase
  3. Move QuadratureLfo class to a single file, include/QuadratureLfo.h

@LmmsBot
Copy link

LmmsBot commented Jan 4, 2021

🤖 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

macOS

🤖
{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://1355-94782492-gh.circle-artifacts.com/0/lmms-1.2.3-837%2Bg3dcae02-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/PhysSong/lmms/1355?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://1352-94782492-gh.circle-artifacts.com/0/lmms-1.2.3-837%2Bg3dcae02d8-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/PhysSong/lmms/1352?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://1354-94782492-gh.circle-artifacts.com/0/lmms-1.2.3-837%2Bg3dcae02d8-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/PhysSong/lmms/1354?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/1ccjo04dfx89kfva/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/38391119"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/a4fh0h5q2fhlceo4/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/38391119"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://1351-94782492-gh.circle-artifacts.com/0/lmms-1.2.3-837%2Bg3dcae02d8-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/PhysSong/lmms/1351?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "3dcae02d86f71b0d24594e6779e46b0e666016e7"}

@PhysSong
Copy link
Member Author

@curlymorphic @douglasdgi FYI.

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.

Looks ok to me, my comments are minor.

include/QuadratureLfo.h Outdated Show resolved Hide resolved
include/QuadratureLfo.h Show resolved Hide resolved
include/QuadratureLfo.h Show resolved Hide resolved
PhysSong added a commit to PhysSong/lmms that referenced this pull request Jan 24, 2021
Updates QuadratureLfo and moves to include/

Co-authored-by: Hyunjin Song <tteu.ingog@gmail.com>
Co-authored-by: Dave French <dave.french3@googlemail.com>
PhysSong added a commit to PhysSong/lmms that referenced this pull request Jan 24, 2021
Updates QuadratureLfo and moves to include/

Co-authored-by: Hyunjin Song <tteu.ingog@gmail.com>
Co-authored-by: Dave French <dave.french3@googlemail.com>
@PhysSong
Copy link
Member Author

I squashed commits manually because GitHub's "squash and merge" will make me the primary author whereas it should be @douglasdgi.

plugins/Flanger/FlangerEffect.cpp Outdated Show resolved Hide resolved
plugins/Flanger/FlangerControls.cpp Outdated Show resolved Hide resolved
include/QuadratureLfo.h Show resolved Hide resolved
@PhysSong
Copy link
Member Author

@DomClark Thanks for your review comments. Pinging @douglasdgi in case he can reply some of them.

@PhysSong
Copy link
Member Author

I think I have to fix them myself, except the last one which I can't find a good reason to fix.

@PhysSong
Copy link
Member Author

Fixed two review comments from @DomClark. I also added the unit for the hint text of the phase knob.

@IanCaio
Copy link
Contributor

IanCaio commented Mar 22, 2021

@curlymorphic Do you think this PR is ready for merge? You have a valid point on the trigonometric function comment, should it be fixed in this PR or do we add this to a TODO list and check all the plugins for this type of thing? Think the latter would make more sense as we could create a header on the core with those look up tables and replace those functions everywhere at once. If you approve this I'm merging it!

@PhysSong
Copy link
Member Author

I will have to squash commits manually and fast-forward to master because GitHub's "Squash and merge" will mark me as primary author.

@curlymorphic
Copy link
Contributor

the trigonometric function comment, should it be fixed in this PR

Your suggestion of treating the use of the trigonometric functions as a separate issue is a good idea, and would probably lead to a more consistent solution.

I feel this is ready for merge

@curlymorphic curlymorphic self-requested a review March 23, 2021 08:37
@IanCaio
Copy link
Contributor

IanCaio commented Mar 23, 2021

Thanks @curlymorphic ! Think we should open an issue to keep track of that change? I was about to open it but our issue log is so polluted I wonder if we should just remind ourselves and tackle it soon. We'd probably need to make the look-up tables on lmms_math.h then grep every use of sin/cos/tan (and others) and replace them.

@PhysSong Leaving this one for you to merge so you can handle that authorship issue 🚀

@curlymorphic
Copy link
Contributor

Think we should open an issue to keep track of that change? I was about to open it but our issue log is so polluted I wonder if we should just remind ourselves and tackle it soon.

I would agree, that in the scheme of things, optimizing standard functions may not be the best use of LMMS devs time.

@IanCaio
Copy link
Contributor

IanCaio commented Mar 23, 2021

I would agree, that in the scheme of things, optimizing standard functions may not be the best use of LMMS devs time.

Sorry, I might have expressed myself badly! I think we should optimize those, I just meant that our issue tracker is bloated (could definitely use a huge clean up) so maybe we should just remind ourselves of doing this through Discord. I think it wouldn't be too big of an effort (basically writing the optimized methods on the header and doing a big sed replace. Could be done during the reorg maybe since headers will be moving around.

Updates QuadratureLfo and moves to include/

Co-authored-by: Hyunjin Song <tteu.ingog@gmail.com>
Co-authored-by: Dave French <dave.french3@googlemail.com>
devnexen pushed a commit to devnexen/lmms that referenced this pull request Apr 10, 2021
Updates QuadratureLfo and moves to include/

Co-authored-by: Hyunjin Song <tteu.ingog@gmail.com>
Co-authored-by: Dave French <dave.french3@googlemail.com>
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
Updates QuadratureLfo and moves to include/

Co-authored-by: Hyunjin Song <tteu.ingog@gmail.com>
Co-authored-by: Dave French <dave.french3@googlemail.com>
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.

6 participants