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

Glitch stutter built-in effect #11329

Merged
merged 8 commits into from
Jun 16, 2023
Merged

Conversation

pcktm
Copy link
Contributor

@pcktm pcktm commented Mar 5, 2023

I wanted to share my progress on the effect I've been working on, as discussed on Zulip. I've managed to implement the basic functionality that I was going for, and I'm hoping to get some feedback on it. One thing to note is that the timing code I'm using is borrowed directly from the Echo effect, and while it seems to work, I'm definitely open to suggestions on alternatives! Please have a look and let me know what you think.

@github-actions github-actions bot added the build label Mar 5, 2023
@JoergAtGithub
Copy link
Member

Welcome and thanks for developing this effect! Please sign the Mixxx Contributor Agreement and comment here when you have done so. It gives us permission to distribute your contribution under the GPL v2 or later license and the Apple Mac App Store. It is also helpful for us to have contact information for contributors in case we may need it in the future.

@pcktm
Copy link
Contributor Author

pcktm commented Mar 5, 2023

Thanks, I've signed the agreement.

@JoergAtGithub
Copy link
Member

It's recommended to install the pre-commit framework for automatic code checking: https://github.com/mixxxdj/mixxx/wiki/Using-Git#set-up-automatic-code-checking

@daschuer
Copy link
Member

daschuer commented Mar 7, 2023

I have just tested it and it is a great fun, especially when vocals are involved.
Congratulations.

Unfortunately we have a pending issue, that new effects are hidden by default.

@ronso0
Copy link
Member

ronso0 commented Mar 7, 2023

Unfortunately we have a pending issue, that new effects are hidden by default.

Is there already an issue for that?

@daschuer
Copy link
Member

daschuer commented Mar 8, 2023

I have also found none. Here it is: #11343

@pcktm
Copy link
Contributor Author

pcktm commented Mar 8, 2023

I have scaled the timing to be more natural, with no glitch on knob at 0 and with infinite stretch on knob maxed out.

@pcktm pcktm marked this pull request as ready for review March 8, 2023 19:41
@github-actions
Copy link

github-actions bot commented Jun 7, 2023

This PR is marked as stale because it has been open 90 days with no activity.

@github-actions github-actions bot added the stale Stale issues that haven't been updated for a long time. label Jun 7, 2023
@Swiftb0y Swiftb0y added needs review and removed stale Stale issues that haven't been updated for a long time. labels Jun 7, 2023
@ferranpujolcamins ferranpujolcamins self-requested a review June 11, 2023 10:03
src/effects/backends/builtin/glitcheffect.cpp Outdated Show resolved Hide resolved
src/effects/backends/builtin/glitcheffect.cpp Outdated Show resolved Hide resolved
src/effects/backends/builtin/glitcheffect.cpp Outdated Show resolved Hide resolved
src/effects/backends/builtin/glitcheffect.cpp Outdated Show resolved Hide resolved
@pcktm pcktm requested a review from ferranpujolcamins June 11, 2023 15:26
@pcktm
Copy link
Contributor Author

pcktm commented Jun 11, 2023

One thing that I've noticed that can be problematic, and is quite unexpected from a UX point of view, is that the buffer keeps repeating after the track has been paused. It is fairly obvious why this is the case, but is there any way to do anything about it?

@ferranpujolcamins
Copy link
Contributor

ferranpujolcamins commented Jun 11, 2023

One thing that I've noticed that can be problematic, and is quite unexpected from a UX point of view, is that the buffer keeps repeating after the track has been paused. It is fairly obvious why this is the case, but is there any way to do anything about it?

Yeah I've noticed this. Another similar issue is that when the delay knob is at high values you cannot smoothly fade out the track with the fader, since the effects are post-fader.

This is a bigger and more general issue with sampling effects. I'd prefer to merge this PR as is and think of a solution later. At the end, the user can fade the effect away with the effect unit dry/wet knob. It's not super intuitive, but if you think about it, is what's logical given effects are post-fader.

@pcktm pcktm requested a review from ferranpujolcamins June 11, 2023 21:16
@Swiftb0y
Copy link
Member

Thank you for your Pull request. Please understand that in order for us to be able to merge this, your contribution need to follow our style guide. Most of the required changes can be made automatically by use of pre-commit. Please set up pre-commit and make sure to manually apply the required changes using pre-commit run --from-ref main --to-ref HEAD. Thank you very much.

@pcktm
Copy link
Contributor Author

pcktm commented Jun 12, 2023

Sure, last commit passed code style check already.

Copy link
Contributor

@ferranpujolcamins ferranpujolcamins left a comment

Choose a reason for hiding this comment

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

I've requested a couple more changes. Besides that LGTM. So from my side it's ready to merge after these last changes are implemented.

src/effects/backends/builtin/glitcheffect.cpp Outdated Show resolved Hide resolved
src/effects/backends/builtin/glitcheffect.cpp Outdated Show resolved Hide resolved
@pcktm
Copy link
Contributor Author

pcktm commented Jun 13, 2023

Thanks a lot for the review and suggestions, can't wait to seeing this effect in a future release!

@pcktm pcktm requested a review from ferranpujolcamins June 13, 2023 21:18
Co-authored-by: Swiftb0y <[email protected]>
@ferranpujolcamins
Copy link
Contributor

LGTM

@Swiftb0y
Copy link
Member

Great. feel free to go ahead and merge 👍

@ferranpujolcamins ferranpujolcamins merged commit 305322b into mixxxdj:main Jun 16, 2023
@ferranpujolcamins
Copy link
Contributor

@pcktm congrats on your first Mixxx contribution 🎉

I've thought about a couple possible next steps to make the effect even nicer, if you are up to:

  • Make the buffer length editable with a parameter. I'd be curious how the effect sounds with longer repeat windows, kind of a beat repeater I guess.
  • I believe the sound quality can be improved by crossfading a bit the buffers.

@daschuer daschuer added this to the 2.5.0 milestone Jan 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants