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

Fix triangle release by switching period by tick #714

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JerwuQu
Copy link
Contributor

@JerwuQu JerwuQu commented Apr 28, 2024

Fixes the bug with the 1ms triangle release not happening after #711.

As you can see, it's not as clean as one could hope.

This is kind of a compromise between having tone be sample-based and having it tick-based. I believe this is the best kind of compromise when perfect 60 Hz ticks aren't possible though. Basically, we respect samples when it comes to envelopes A->D->S but respect ticks for starting the Release.
In practice this means that if ticks were to drift (which they almost always do), they only do so during the Sustain period. If we were to make all parts of ADSR tick based, the drift could happen at any parts.

Still, this is a bit unfortunate for the case of frequency slides where we can't know how many samples long a tone will be until we have already detected the drift, meaning we either have to end the slide short (because of tick-sample drift), or we have to adjust it as it happens (leading to pitch skips).
I chose here to go with having the slide end short which would be the least audible of the two. This required introducing a new releaseTime for what is estimated when the tone starts playing to be able to have a target time for getCurrentFrequency.

I am thinking even more about rewriting the WASM-4 update to be based on the audio routine to both fix the tick-sample drift and issue #678 for good, but for now this is what we can do.

Since this is a bit more code, I would appreciate someone else checking the logic so I don't introduce any new bugs!

@majaha
Copy link
Contributor

majaha commented May 1, 2024

Heya. I've been working on my own patches over here: #716 which I think come at some of the problems you were attacking with a simpler approach. Could you try those changes out and see if they solve the issue you're trying to fix here?

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.

2 participants