fix(player): an unrecognised waveform is a pulse, not a sine - #21
Merged
Conversation
`gen waveform bogus` produced a sine in the browser and a pulse in the ROM: the JS table's catch-all branch was `Math.sin`, while the GBA bake's catch-all arm in deckpack.rs `pcm_table` is a pulse. `pulse` is already the default for an *absent* waveform on both sides, so the sine was inconsistent with the language's own default as well as with the bake. Adds an explicit `sine` arm first, so `waveform sine` keeps working — the bake has one, and without it this change would have silently removed sine support. `square` still ignores `duty` here, which the bake does not; that half of the divergence is fixed on the Rust side.
spacedevin
added a commit
to schlopai/chuggie
that referenced
this pull request
Sep 1, 2026
`pcm_table` had no `square` arm, so `square` fell through to the `_` catch-all — a pulse that reads `duty`. `gen waveform square duty 25` therefore baked a 25% pulse while @spacedevin/deck-player played a fixed 50% square. Same source, two different ROMs' worth of sound, and `square` is a documented waveform in the chuggie.dev deck docs. `square` now means 50% here too, and ignores `duty` the way the browser does. Found while diffing the two implementations for an audio conformance corpus. The other divergence from that pass — an unrecognised `waveform` being a sine in the browser and a pulse here — is fixed on the JS side in spacedevin/deck#21.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
gen waveform bogusproduced a sine in the browser and a pulse in the ROM.The JS table's catch-all branch in
packages/player/src/generators/GbaDirectSound.tishwasMath.sin(...), while the GBA bake's catch-all arm indeckpack.rspcm_tableis a pulse.pulseis already the documented default for an absentwaveformon both sides, so the sine wasinconsistent with the language's own default as well as with the bake.
This adds an explicit
sinearm first — the bake has one, and without it changing theelsewouldhave silently removed sine support — and then makes the fallback a pulse.
Context
Found while building a cross-implementation audio conformance corpus. There are two remaining known
divergences between this player and the GBA bake:
squareignoresdutyhere (a 50% wave, which is what the name means) but the bake has nosquarearm, so it falls through to pulse and does honourduty.gen waveform square duty 25is 50% here and 25% on hardware. That half belongs in the engine repo, not this one.
bitcrush 16bitbypasses quantisation here, but the bake always emitsi8, so the ROM is 8-bitregardless. That one is genuinely irreconcilable and should be documented rather than "fixed".
No behaviour change for any waveform that was already spelled correctly. The existing
gbaDirectSound waveform tablestest inpackages/player/test/voices.mjsstill passes — itssquare is two-levelassertion holds either way.