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

Allow SampleTCOs/Sample Clips to be reversed #5765

Merged
merged 13 commits into from
Nov 21, 2020
Merged

Conversation

ryuukumar
Copy link
Member

@ryuukumar ryuukumar commented Nov 4, 2020

There was a reverse option in SampleBuffer, so I decided to implement it so that it can actually be used.

capture3

Progress tracker:

  • Have the samples actually reverse
  • Update the GUI
  • Save the reversed state
  • Pixmap for reversing (Fix: use flip_x pixmap found in automation editor)

Pretty minor feature, so I think it won't be much of a nuisance, considering it's practically complete at the time of writing. I should not have said that.

@LmmsBot
Copy link

LmmsBot commented Nov 4, 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

macOS

🤖
{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://10585-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.27%2Bgdb6e492-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10585?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://10589-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.27%2Bgdb6e492c8-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10589?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://10588-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.27%2Bgdb6e492c8-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10588?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/991n29sv8hsetemv/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36415516"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/4wwh9nmcp15wn1od/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36415516"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://10587-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.27%2Bgdb6e492c8-mac10.13.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10587?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "66091b3c84526beb84152dcfac98b66d7b7e7dd7"}

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.

Overall it LGTM. I've wrote a suggestion regarding the drawing for loop, an idea to fix the issue with calling reverseSample while loading the project (which has to be studied first) and some code style convention notes. Didn't explicitly request the changes because they are mostly suggestions not requests and I'd like to hear others opinions on them.

@ryuukumar
Copy link
Member Author

ryuukumar commented Nov 8, 2020

Hey, since I have changed some other code in SampleBuffer.cpp in the functions convertIntToFloat and directFloatWrite because it had reversing snippets which could be made more shorter and readable. However I'm not sure what these really do, so it would be great if someone could test and see if samples reliably work after this.

In general, if anybody wants to test, you can try these and check for stability:

  • Reversing samples in general and seeing if the GUI waveform and the audio are in sync
  • Reversing samples while they're being played, and checking for GUI-audio sync
  • Copying around sample clips which are reversed (or sample tracks with reversed clips) and seeing if they're still reversed
  • Saving reversed samples and seeing if they're still reversed
  • ...any other sample clip and sample track-related stuff that you can think of

Also want to point out that I was preferably looking for either a new icon, however if that's not possible, I can probably use the "loop" icon.

@IanCaio
Copy link
Contributor

IanCaio commented Nov 8, 2020

I also didn't get to read the whole methods you refactored (convertIntToFloat and directFloatWrite) but as far as the logic goes the changes looks good!

I'll try to test the build soon

@Gabrielxd195
Copy link

I think the best icon for the '' reverse sample '' option is this.
Boton de Reverse sample

Which is to '' flip horizontally '' the automation tcos. By reversing an audio sample, you are technically "flipping it horizontally."

And if in the future they implemented the possibility of '' inverting the phase '' to an audio sample, then that would be '' flipping vertically '', and we would use the icon that is already in the automation tco. but I guess that would be discussed in another thread.

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.

Code LGTM!

Just a note for other reviewers, I know that some small details of the code style isn't 100% fixed (@russiankumar did a great job improving it, but SampleBuffer.cpp has quite a lot of stuff to be fixed), but I'm working locally on a small refactor for this file which should fix the remaining issues. It's basically ready (need to double check), I'm just waiting for this one to be merged so I can fix the conflicts.

@ryuukumar
Copy link
Member Author

Great! If @PhysSong and/or @Veratil approve, you can merge this.

@IanCaio, the refactor you’re doing seems like a little sneak peak of what the LMMS refactor is going to be like, IMO xD

@ryuukumar
Copy link
Member Author

@Veratil I've made the requested changes.

@ryuukumar ryuukumar requested a review from Veratil November 19, 2020 10:37
@ryuukumar ryuukumar requested a review from Veratil November 20, 2020 04:51
@ryuukumar
Copy link
Member Author

@Veratil now maybe? 😂

@ryuukumar
Copy link
Member Author

Build error seems time-related, and all other are passing. Merging.

@ryuukumar ryuukumar merged commit 53b003b into LMMS:master Nov 21, 2020
IanCaio added a commit to IanCaio/lmms that referenced this pull request Nov 24, 2020
IanCaio pushed a commit to IanCaio/lmms that referenced this pull request Mar 28, 2021
Enable the reverse option from `SampleBuffer.cpp`, and partially change the style and make more readable `SampleBuffer.cpp`.
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
Enable the reverse option from `SampleBuffer.cpp`, and partially change the style and make more readable `SampleBuffer.cpp`.
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