Various fixes and improvements to the APU #716
Conversation
This bug can be recreated by running `tone(345, 1, 50, 4)` as well as a big time-wasting spin-loop every `update()`.
This bug can be recreated by running `tone(345, 2, 50, 4)` as well as a big time-wasting spin-loop every `update()`.
The old logic could cause sound glitches if a tick comes in earlier than expected.
This means that tone() calls that are made at different times within a frame all start at the same time.
Also fixes up the code that pauses audio, which was buggy and some of which was dead code.
…ping This reverts commit 4e50857. That change was causing tones to stop before the end of their release if an ending tick came in first. I've decided to let tones that aren't replaced with another tone to overrun the ending tick instead, to prevent sudden stops and popping.
| int channelIdx = flags & 0x3; | ||
| bufferedToneCalls[channelIdx].active = true; | ||
| bufferedToneCalls[channelIdx].frequency = frequency; | ||
| bufferedToneCalls[channelIdx].duration = duration; | ||
| bufferedToneCalls[channelIdx].volume = volume; | ||
| bufferedToneCalls[channelIdx].flags = flags; |
There was a problem hiding this comment.
Not sure if keeping APU state outside of the APU is desired here.
The APU already has internal global state, so maybe this logic could be placed in apu.c instead to keep it isolated?
| @@ -117,7 +114,7 @@ class APUProcessor extends AudioWorkletProcessor { | |||
|
|
|||
| getCurrentVolume (channel: Channel) { | |||
| const time = this.time; | |||
| if (time >= channel.sustainTime && (channel.releaseTime - channel.sustainTime) > RELEASE_TIME_TRIANGLE) { | |||
| if (time >= channel.sustainTime) { | |||
There was a problem hiding this comment.
I quickly tested.
This change brings back the continous tone bug.
Here's a cart you can use to test the bug: cart.zip
With this test cart, only the "Short attack" should be popping and all other ones should be smooth.
There was a problem hiding this comment.
Ah yes, so it does.
Can I ask what your specific goal with the whole tick thing was anyway? The more I think about it, the more I think that messing with the ADSR envelope using ticks is weird, and possibly overcomplicating things. If the user asks for the envelope to be only one tick long, why would they be surprised when it ends within one tick?
There was a problem hiding this comment.
If the user asks for the envelope to be only one tick long, why would they be surprised when it ends within one tick?
Put another way: if a user asks for the tone to be one tick long, shouldn't they be surprised if isn't actually one tick long? That doesn't make sense in terms of the abstraction WASM-4 is. There have been bug reports from people being confused about it behaving like that when wanting to make continous tones.
More technically (which I plan to go into in a blog post), custom volume/pitch envelopes and effects like vibrato are simply not feasible if you can't rely on tone to last the length you say it should last. You can do workarounds by extending each tone to be longer (e.g. 2 or 3 ticks), but then you end up with pretty inaccurate envelopes instead that will overextend in whatever direction they are heading towards, especially if framerates are unstable. In these cases it's better to have it stay stable on the last known volume and pitch instead.
Some other features that are very difficult to do if tone length can't be relied upon are live playback (e.g. virtual piano) with working ADSR, and temporarily muting audio and then unmuting with retained note and envelope state.
The issue I am concerned with is specifically when the tone is only one tick long. It should last until the next tick.
Worth noting is all of these issues would disappear if the update loop is bound to the audio callback which I plan on giving a shot later, but it's a big effort.
There was a problem hiding this comment.
In the previous version, when the user asked for a note that was the length of one tick, that's exactly what they got: a note that was precisely 1/60th of a second long. I think the problem of confused users may be better serviced with documentation saying that only long or overlapping notes are guaranteed to be continuous.
Phrased another way: There's a lot to be said for taking the approach of "a note is a note is a note", i.e. given a particular tone() call, the sound produced is always exactly the same, in length, volume, and pitch. No stretching or other irregularities can occur.
Are the main problems you have with live playback to do with the pitch gliding? I can think of solutions to other problems you might have, but trying to make the pitch-glides work while making extra-duration tone() calls does present a tricky challenge.
I do also see the value in having the note lengths stay in sync with the game's ticks. Especially as the developer can schedule sound events over 10 seconds into the future, and the tone() API presents the lengths as a number of ticks (I wonder whether the same issue would be brought up if tone lengths were in an integer number of milliseconds, or a float of seconds). I see these two approaches as competing ideas, both with their own pros and cons.
However, my gut feeling is that if a tick-based solution is chosen, then the whole note should be tick-based, with each ADSR point waiting for it's tick to come in. The developer could use any of the ADSR points as an important sync point, so we should sync them all. What do you think of that idea? I think I'll try to implement this.
(Another idea is to sync on every tick, but that may be overkill.)
Worth noting is all of these issues would disappear if the update loop is bound to the audio callback which I plan on giving a shot later, but it's a big effort.
About this, I can't see how that's going to be workable. At least, not in the Web runtime, which I know better. You could try though.
After the tick-awareness was added recently, I took the opportunity to review the APU code, and found various issues I could fix.
The biggest changes are that now tone() calls will all take affect simultaneously at the end of a tick, and that the audio will pause more smoothly when the game is paused. Other changes are mostly bug fixes and clean-ups.