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

Arpeggiator - Note repeats #5784

Merged
merged 5 commits into from Jan 1, 2021
Merged

Arpeggiator - Note repeats #5784

merged 5 commits into from Jan 1, 2021

Conversation

zonkmachine
Copy link
Member

I finally got around to finish the arpeggiator note repeats function that I started working on here #687.

The first attempt modified the direction logic but I found a simpler way to do away with that.

@zonkmachine zonkmachine added needs style review A style review is currently required for this PR needs code review A functional code review is currently required for this PR needs testing This pull request needs more testing labels Nov 13, 2020
@LmmsBot
Copy link

LmmsBot commented Nov 13, 2020

🤖 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://11790-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.60%2Bg075e608-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/11790?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://11794-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.60%2Bg075e608df-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/11794?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://11791-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.60%2Bg075e608df-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/11791?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "b474a9c1694dabd7a13b2338aa24ddc0598a6f4d"}

Copy link
Contributor

@IanCaio IanCaio left a comment

Choose a reason for hiding this comment

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

First review just goes through code style, which I know sounds pointless considering the current state of the InstrumentFunctionArpeggio::processNote method (IMHO it needs a refactor on the code style and improvements on comments, but that's outside the scope of this PR), but I guess the general consensus is to at least keep new additions compliant, even if the surrounding code isn't.

I'm still learning the logic of the method itself to further review, but testing the build I noticed a misbehavior that I'm still trying to figure out the source of in the code: The repeating notes overlap with the previous note. Probably something to do with the calculation of the frames per note, but I suppose the repeating notes should be played alone not on top of the previous one.

src/core/InstrumentFunctions.cpp Show resolved Hide resolved
src/core/InstrumentFunctions.cpp Outdated Show resolved Hide resolved
src/core/InstrumentFunctions.cpp Outdated Show resolved Hide resolved
src/core/InstrumentFunctions.cpp Outdated Show resolved Hide resolved
src/core/InstrumentFunctions.cpp Show resolved Hide resolved
src/core/InstrumentFunctions.cpp Show resolved Hide resolved
src/gui/widgets/InstrumentFunctionViews.cpp Show resolved Hide resolved
src/gui/widgets/InstrumentFunctionViews.cpp Show resolved Hide resolved
src/gui/widgets/InstrumentFunctionViews.cpp Show resolved Hide resolved
src/gui/widgets/InstrumentFunctionViews.cpp Show resolved Hide resolved
@zonkmachine
Copy link
Member Author

The repeating notes overlap with the previous note.

Right. The envelope I used on my test case hide that well.

Thanks for the review, I'll look into it!

@superpaik
Copy link
Contributor

This is a very tricky one to test. Something weird happens but I'm not sure it's due to this PR, though the repeat function make it clearer.
Anyway. I try to explain it.
If you set a project like the one in the image, where REP is set to one (no repeat), mode FREE and TIME synced to 16th note, everything seem to work fine. But if you set REP to two, you'll notice a silence before the thrid beat (just before E4 plays), and again at the end of 4th beat.
If you set mode to SYNC, some notes of the arpegiator (I think are those of the E4 note) are not sync at all.
I said that maybe is unrelated to this PR because in a similar situation (without no REP, of course) on the stable version, though notes are sync, some notes of the E4 arpeggio are not playing.
imatge

@zonkmachine
Copy link
Member Author

@superpaik Thanks for testing the PR!
I don't think the problems you mention are related to this PR. There are known issues with disappearing notes. #2606 #3880

@IanCaio
Copy link
Contributor

IanCaio commented Nov 14, 2020

@zonkmachine The overlapping notes issue seems to be fixed!

I noticed some notes skipping or odd behavior (even on other modes like Sort), but as you mentioned those are unrelated to this PR but a problem with the current state of the Arpeggiator. I was going to suggest a refactor of this method (on another PR ofc), but seems like there are some issues opened already regarding this and other deeper problems that apparently causes those glitches on the notes.

Everything seems to be fine on the code, but since I'm still getting familiar with this method I'll review it again.

@zonkmachine
Copy link
Member Author

I'm thinking we can give the ComboBoxes a bit more space.

Changes to the layout on the left instrument window

-       mainLayout->setHorizontalSpacing( 20 );
+       mainLayout->setHorizontalSpacing( 14 );

layout

Copy link
Contributor

@IanCaio IanCaio left a comment

Choose a reason for hiding this comment

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

Sorry I resolved the comments and wrote all over again, but I think I couldn't reuse the comments for a last review. It has been brought up recently again the argument in favor of us being more strict with code style on lines that are touched by the PR, which I agree with since if we take the surrounding code into account we will probably never move away from the previous non-conforming styles used.

As for the logic, I reviewed again after the gate issue was fixed and it all seems good! 😃

src/core/InstrumentFunctions.cpp Show resolved Hide resolved
src/core/InstrumentFunctions.cpp Outdated Show resolved Hide resolved
src/core/InstrumentFunctions.cpp Outdated Show resolved Hide resolved
src/core/InstrumentFunctions.cpp Show resolved Hide resolved
src/core/InstrumentFunctions.cpp Show resolved Hide resolved
src/core/InstrumentFunctions.cpp Show resolved Hide resolved
src/gui/widgets/InstrumentFunctionViews.cpp Show resolved Hide resolved
src/gui/widgets/InstrumentFunctionViews.cpp Show resolved Hide resolved
src/gui/widgets/InstrumentFunctionViews.cpp Show resolved Hide resolved
src/gui/widgets/InstrumentFunctionViews.cpp Show resolved Hide resolved
@zonkmachine
Copy link
Member Author

@IanCaio @Veratil I find the interface a bit confusing. Can you please commit your changes or just push them directly?

zonkmachine and others added 2 commits December 7, 2020 23:06
Co-authored-by: Kevin Zander <veratil@gmail.com>
Co-authored-by: IanCaio <iancaio_dev@hotmail.com>
@zonkmachine
Copy link
Member Author

I have now gone over your suggestions and merged two of them. Much appreciated! In regards to the clang format changes the pendulum now seem to have changed, once more, to waiting for a script to do the job.

@superpaik
Copy link
Contributor

If there is only one note playing at a time, repeat function works well in every mode (free, sort, sync)
But if there are two notes playing at a time, in sync mode some notes, at random, are not repeated, they "desappear" (you can use the same use-case I detailed above)

@zonkmachine
Copy link
Member Author

I think it's this issue you're seeing: #2606

@superpaik
Copy link
Contributor

Yes. I see, it's related to that other PR.

@zonkmachine
Copy link
Member Author

zonkmachine commented Dec 28, 2020

I think this has been reviewed for both style and code and has been tested thoroughly (I've been trying out most of these code changes for well over six years).

@zonkmachine zonkmachine removed 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 Dec 30, 2020
@zonkmachine
Copy link
Member Author

Merge? :)

@zonkmachine zonkmachine merged commit d769459 into LMMS:master Jan 1, 2021
@zonkmachine zonkmachine deleted the arprepeats branch January 1, 2021 13:43
@zonkmachine zonkmachine mentioned this pull request Jan 1, 2021
7 tasks
IanCaio added a commit to IanCaio/lmms that referenced this pull request Mar 28, 2021
Co-authored-by: Kevin Zander <veratil@gmail.com>
Co-authored-by: IanCaio <iancaio_dev@hotmail.com>
devnexen pushed a commit to devnexen/lmms that referenced this pull request Apr 10, 2021
Co-authored-by: Kevin Zander <veratil@gmail.com>
Co-authored-by: IanCaio <iancaio_dev@hotmail.com>
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
Co-authored-by: Kevin Zander <veratil@gmail.com>
Co-authored-by: IanCaio <iancaio_dev@hotmail.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.

None yet

5 participants