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

Catch Notes setting to allow not catching existing notes. #221

Merged
merged 3 commits into from Jul 21, 2023

Conversation

weavermedia
Copy link
Contributor

Small thing that always bugged me. When shift switching between clips there was code that looked for any notes in the new clip that would've been recently triggered and plays them. For drum clips this was especially noticeable - producing double hits and flams and didn't sound good at all.

This community setting I called Catch Notes is a simple on/off that adds a conditional to the code that finds and plays these 'already triggered' notes. Set to 'off' these notes will not be triggered and shift switching between drum clips now becomes flawless.

It defaults to 'on' which is the current Deluge behavior so no surprises for users.

Again, a video is worth a billion words. In the first half Catch Notes is on (current default behavior) and you'll hear lots of double hits and flamming. After I turn it 'off' switching clips is silky smooth.

NOTE: I would've called this a bug fix until I found the code that plays these notes. It's very deliberate and intentional code so I wouldn't consider it a bug to fix. Just a behavior to optionally change.

Deluge-CatchNotes.mp4

@weavermedia weavermedia changed the title Add on/off setting for Catch Notes to allow not catching late notes. Catch Notes setting to allow not catching existing notes. Jul 21, 2023
@jamiefaye jamiefaye added this pull request to the merge queue Jul 21, 2023
Merged via the queue into SynthstromAudible:community with commit 6b17091 Jul 21, 2023
2 checks passed
@weavermedia weavermedia deleted the catch-notes branch July 22, 2023 02:42
@PaulFreund
Copy link
Collaborator

PaulFreund commented Aug 7, 2023

In reference to the discussion in #253 I found a way to reliably reproduce the issue:

  1. With current Community HEAD open to a blank song
  2. Make sure Catchnotes is turned on in community settings
  3. Set tempo to 120 or higher (might make it easier)
  4. Create a TR-808 clip with a kick in position 1, 5, 9 and 13 (four to the floor) and a snare in 3, 7, 11 and 15
  5. Copy the clip to the next row
  6. Hold shift and rapidly click between the two row arm buttons
  7. You will hear a lot of double sounding clips

Very important edit: Looking at the code I just noticed that your default = ON is the official behavior and turning the setting OFF introduces your chaanges. Very good job fixing this issue :D !

@sichtbeton can you please describe in which state of the Catchnotes setting you are experiencing problems? If you had the same problem I had I think we make this the new default behavior. I also think maybe we should change the logic of the setting and call it "Fix double triggering" or something more appropriate so = ON indicates that the default behavior was changed.

@sichtbeton
Copy link
Contributor

In reference to the discussion in #253 I found a way to reliably reproduce the issue:

  1. With current Community HEAD open to a blank song
  2. Make sure Catchnotes is turned on in community settings
  3. Set tempo to 120 or higher (might make it easier)
  4. Create a TR-808 clip with a kick in position 1, 5, 9 and 13 (four to the floor) and a snare in 3, 7, 11 and 15
  5. Copy the clip to the next row
  6. Hold shift and rapidly click between the two row arm buttons
  7. You will hear a lot of double sounding clips

Very important edit: Looking at the code I just noticed that your default = ON is the official behavior and turning the setting OFF introduces your chaanges. Very good job fixing this issue :D !

@sichtbeton can you please describe in which state of the Catchnotes setting you are experiencing problems? If you had the same problem I had I think we make this the new default behavior. I also think maybe we should change the logic of the setting and call it "Fix double triggering" or something more appropriate so = ON indicates that the default behavior was changed.

with "off" it's working like expected. with "on" it's double triggering. so it's (if I wrap my head around this right) what you are seeing. naming it "off" when actually off is a sensible to do.

good job @weavermedia !

@weavermedia
Copy link
Contributor Author

weavermedia commented Aug 7, 2023

Wow, I got really mixed up there for second! 😂 @PaulFreund @sichtbeton

I'll explain my thought process:

'Catch notes' is what the original Deluge code does. It catches and plays the notes that would have been played it the clip was switched perfectly on the beat. That's what leads to the double hits.

My feature is a switch to disable this 'catch notes' behavior, therefore preventing the double hits.

Maybe it's not very intuitive to have a community feature that has a name of default behavior, rather the the name reflecting new behavior?

The setting defaults to 'on' because that's the original Deluge behavior.

@weavermedia
Copy link
Contributor Author

For reference here is Rohan's response to my FB post asking if the default behavior is a bug:
https://www.facebook.com/groups/DelugeBetaTesters/posts/1677334075806819/

@sichtbeton
Copy link
Contributor

Wow, I got really mixed up there for second! 😂 @PaulFreund @sichtbeton

I'll explain my thought process:

'Catch notes' is what the original Deluge code does. It catches and plays the notes that would have been played it the clip was switched perfectly on the beat. That's what leads to the double hits.

My feature is a switch to disable this 'catch notes' behavior, therefore preventing the double hits.

Maybe it's not very intuitive to have a community feature that has a name of default behavior, rather the the name reflecting new behavior?

The setting defaults to 'on' because that's the original Deluge behavior.

confusing. but you're right on the money there.

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

4 participants