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.
| 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?
runtimes/web/src/apu-worklet.ts
Outdated
| @@ -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.
Each tone call is split into sections, each of which has a start volume and frequency, and an end volume and frequency. Each section is played continuously until either its end *tick* is reached, ending the section and starting the next, or its end *time* is reached, which holds the tone there and stretches it out until its end tick is reached.
Add a short fade-out period to the end of all tones to prevent audio clicks and pops.
The web runtime doesn't run on NodeJS, but was using node types. This was causing issues because some node types override the DOM types, like setTimeout(). Adding `"types": []` to tsconfig.json prevents types in node_modules/@types from being used, including node_modules/@types/node, which is installed because it is a dependency of our dependencies.
This commit reworks the frame timing of the runtime ensuring a regular update every frame on 60 fps monitors, while still supporting other framerates both higher and lower. It achieves this by measuring the framerate continuously, and updating in a requestAnimationFrame() callback when the framerate is close to 60, but switching to a setTimer() timing scheme otherwise. This keeps animation smooth on a 60 fps screen, but keeps updates both full-speed and at regular 16ms intervals on a 30 fps screen, important for making tick-based audio timings accurate. This also has the effect of allowing you to run the runtime in a minimised or background browser window. This is really improves the audio experience, allows you to run games or demos in the background, and also makes it much easier to test netplay without having to keep many windows on screen at once.
Previously favicon.ico was being requested from wasm4.org when developing locally. This won't work if the user is offline, the website goes down etc. It also doesn't work well with the new CORS headers I'll introduce in the next commit.
Setting these security headers grants us access to higher precision time. This improves our ability to pace frame timings and make accurate performance measurements. I hope to take advantage of this to improve the devtools in the future.
|
Alrighty, bit of a behemoth patch here. I made the APU completely tick based, and I also ended up rewriting the frame timing code so that it's much smoother now, whereas before even in perfect conditions it would microstutter. Wasm-4 can also now keep running in a background tab, which is great for the audio experience, and should make netplay much less annoying to work with too. There's also a few other little improvements and clean-ups sprinkled in. |
|
Sounds awesome! Can give it a proper look after I'm back from summer vacation, but if it works properly for the cart linked here and for music in games currently published on wasm4.org it has my approval! |
|
Yup, it should be all good! Hopefully you can see that it all sounds very smooth and in-time. And that you can pause Journey to Entorus or minimise the browser window and it behaves nicely. Also the falling stars should move in a more regular way, hopefully! |
|
@JerwuQu Should we merge this? 👀 |
Nah, not right now. I think I'm going to have another pass at some of these things, eventually |
|
Sorry! Postponed this due to the complexity in evalutating it, then completely forgot 😓 Last thing I remember was that Really appreciate all the effort you put into this! |
|
I'm closing this in favour of other pull requests. |
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.