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

SlicerT reversed slices and bug fixes #6992

Closed
wants to merge 2 commits into from

Conversation

DanielKauss
Copy link
Contributor

Fixes the bugs mentioned by @zonkmachine in #6857 and adds reversed slices below the base note. Backwards compatibility should be preserved.

Here is an example, the selected notes get played in reverse:

image

@DanielKauss DanielKauss mentioned this pull request Nov 18, 2023
@sakertooth
Copy link
Contributor

Wouldn't it better for reversed slices to be opt-in for each slice, rather than playing slices in reverse below the root? This would probably mean more infrastructure changes to your plugin, but I'm curious what you think.

@DanielKauss
Copy link
Contributor Author

Wouldn't it better for reversed slices to be opt-in for each slice, rather than playing slices in reverse below the root? This would probably mean more infrastructure changes to your plugin, but I'm curious what you think.

Of course, the ideal version of this plugin would let you modify every individual slice, but this seems like a good compromise for the moment. The keys were unused anyway, so this way they at least have some use.
I will probably come back in the future and add a lot of the features that some professional slicers have, but it will be a while until then. At least I would want #6610 to be merged and aubio added.

Copy link
Member

@zonkmachine zonkmachine left a comment

Choose a reason for hiding this comment

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

Tests fine.

@messmerd
Copy link
Member

Of course, the ideal version of this plugin would let you modify every individual slice, but this seems like a good compromise for the moment. The keys were unused anyway, so this way they at least have some use.

So this is a stopgap for better functionality that may come later? My concern is whether this could create difficulties in implementing future functionality in a backwards compatible way. If it may be an issue, we should probably wait until the new functionality is fully ready and just merge the bug fixes now.

@DanielKauss
Copy link
Contributor Author

So this is a stopgap for better functionality that may come later? My concern is whether this could create difficulties in implementing future functionality in a backwards compatible way. If it may be an issue, we should probably wait until the new functionality is fully ready and just merge the bug fixes now.

I don't really think this is only a stopgap, the keys aren't going to have any extra use in the future, so might as well have them act as the reverse. Also, it'll be a while until I start working on new functionality, since like I said I need the sample buffer PR and aubio. Once I do there won't be any keys with functionality, so this will be preserved. Of course there is no problem removing it for now, I just wanted to add it since it was requested in the original PR.

@zonkmachine
Copy link
Member

It now has conflicts that needs resolving. I think this adds great value to SlicerT.

@@ -143,8 +142,7 @@ void SlicerT::playNote(NotePlayHandle* handle, sampleFrame* workingBuffer)
workingBuffer[i + offset][1] *= fadeValue;
}

instrumentTrack()->processAudioBuffer(workingBuffer, frames + offset, handle);

Copy link
Member

Choose a reason for hiding this comment

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

I removed the two lines above in #7174

@messmerd
Copy link
Member

I think the bug fixes and the reversed slices should be two separate PRs. Then the bug fixes can be merged quickly while we take time to consider how we want to implement reversed samples or other features.

@zonkmachine
Copy link
Member

I think the bug fixes and the reversed slices should be two separate PRs. Then the bug fixes can be merged quickly while we take time to consider how we want to implement reversed samples or other features.

Yes, I agree.

@luzpaz
Copy link
Contributor

luzpaz commented Aug 21, 2024

Just a heads up...there is another slicert PR in the pipeline: #7453
Does it impact this code ?

@Rossmaxx
Copy link
Contributor

@DanielKauss update on the bugfix? If you can seperate the bugfix, I'll consider fast tracking the fixes. I'm considering to merge #7453, hope there's no merge conflicts.

@Rossmaxx Rossmaxx mentioned this pull request Sep 15, 2024
@DanielKauss
Copy link
Contributor Author

@Rossmaxx The bug was already fixed in this PR #7174. I will close this PR, and add all the features like reverse slices and effects together in a future PR.

@Rossmaxx
Copy link
Contributor

Ohh ok then.

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