-
-
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
Upgrade the Flanger effect #5873
Conversation
🤖 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"} |
@curlymorphic @douglasdgi FYI. |
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.
Looks ok to me, my comments are minor.
Updates QuadratureLfo and moves to include/ Co-authored-by: Hyunjin Song <tteu.ingog@gmail.com> Co-authored-by: Dave French <dave.french3@googlemail.com>
3568932
to
1879bb9
Compare
Updates QuadratureLfo and moves to include/ Co-authored-by: Hyunjin Song <tteu.ingog@gmail.com> Co-authored-by: Dave French <dave.french3@googlemail.com>
1879bb9
to
387b860
Compare
I squashed commits manually because GitHub's "squash and merge" will make me the primary author whereas it should be @douglasdgi. |
@DomClark Thanks for your review comments. Pinging @douglasdgi in case he can reply some of them. |
I think I have to fix them myself, except the last one which I can't find a good reason to fix. |
Fixed two review comments from @DomClark. I also added the unit for the hint text of the phase knob. |
@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! |
I will have to squash commits manually and fast-forward to |
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 |
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 @PhysSong Leaving this one for you to merge so you can handle that authorship issue 🚀 |
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 |
Updates QuadratureLfo and moves to include/ Co-authored-by: Hyunjin Song <tteu.ingog@gmail.com> Co-authored-by: Dave French <dave.french3@googlemail.com>
b535b40
to
3dcae02
Compare
Updates QuadratureLfo and moves to include/ Co-authored-by: Hyunjin Song <tteu.ingog@gmail.com> Co-authored-by: Dave French <dave.french3@googlemail.com>
Updates QuadratureLfo and moves to include/ Co-authored-by: Hyunjin Song <tteu.ingog@gmail.com> Co-authored-by: Dave French <dave.french3@googlemail.com>
Originally authored by @douglasdgi in #5168.
Summary of changes:
QuadratureLfo
class to a single file,include/QuadratureLfo.h