fix(audio): seven audit findings — loading, retries, API contract honesty#1541
Merged
Conversation
…esty - load() gains the already-loaded guard every other parser has: re-preloading a manifest used to silently replace each Howl and leak the old instance's decoded buffers / HTML5 nodes. Returns 0 (cached). - soundLoadError retries are budgeted PER SOUND (state.retryCounters keyed by name): the single shared counter let parallel loads steal each other's retries — one flaky file pushed another sound's first failure over the give-up threshold, a spurious fatal preload error under the default stopOnAudioError. - new me.audio.setStopOnAudioError(value): the flag was documented as settable, but it's a module export — assigning through the namespace throws a TypeError, so the "disable audio instead of throwing" mode was unreachable. The setter is the supported way to change it. - seek()/rate() get honest overloads (the #1456 stereo/position pattern): the setter form returned Howler's Howl object while typed number. Arity-exact forwarding — Howler's core methods dispatch on argument count, so a trailing explicit undefined would parse as an id. - position()/stereo() getters returned Howler's internal null for sounds never positioned/panned; now neutral defaults ([0,0,0] / 0). - resume() without id resumes EVERY paused instance, as its JSDoc always promised: Howler's bare play() only auto-resumes when exactly one instance is paused — with 2+ it spawned a new instance from 0 and left the paused ones stuck. Reads Howl._sounds (no public list; same justified private access as the Howler _muted read). - unload() clears state.currentTrackId when it referenced the unloaded clip, so getCurrentTrack()/pauseTrack()/resumeTrack() don't act on a ghost. tests/audio-audit.spec.js written failing-first: 7/7 red on the old code, 7/7 green after. Full suite 4604 passed / 0 failed. The resume test asserts on the deterministic instance count (a locked headless audio context doesn't flip _paused synchronously). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A
Contributor
There was a problem hiding this comment.
Pull request overview
This PR applies a final batch of audio-module audit fixes in melonJS, focusing on correcting loader/retry behavior, making the public API contracts match runtime behavior, and preventing stale internal state (with a dedicated regression test file and CHANGELOG entries).
Changes:
- Prevent audio re-preload leaks and fix shared retry budgeting by introducing per-sound retry counters and clearing them on successful load.
- Make API behavior “contract-honest” for
seek()/rate()(getter vs setter return types), normalizestereo()/position()getters to return neutral defaults, and fixresume()to resume paused instances without spawning a new one. - Add a supported
setStopOnAudioError()setter and ensureunload()clears the current-track pointer; add 7 new regression tests and document the fixes in the changelog.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/melonjs/tests/audio-audit.spec.js | Adds 7 regression tests covering the audited audio loading/retry and API contract fixes. |
| packages/melonjs/src/audio/playback.ts | Implements audio load caching guard, per-sound retry reset on load, honest seek/rate overloads, getter defaults, and corrected resume semantics. |
| packages/melonjs/src/audio/backend.ts | Introduces setStopOnAudioError() and replaces a shared retry counter with per-sound retry counters. |
| packages/melonjs/src/audio/audio.ts | Re-exports the new setter and clears the current track pointer on unload. |
| packages/melonjs/CHANGELOG.md | Documents the audio audit fixes in the current release notes section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+50
to
+52
| if (typeof state.tracks[sound.name] !== "undefined") { | ||
| return 0; | ||
| } |
Comment on lines
+280
to
288
| // forget the current-track pointer if it referenced this sound, so | ||
| // getCurrentTrack() / pauseTrack() / resumeTrack() don't act on a ghost | ||
| if (state.currentTrackId === sound_name) { | ||
| state.currentTrackId = null; | ||
| } | ||
|
|
||
| // destroy the Howl object | ||
| sound.unload(); | ||
| delete state.tracks[sound_name]; |
Comment on lines
+123
to
+125
| // the give-up path (stopOnError=false) mutes audio globally — restore | ||
| audio.unmuteAll(); | ||
| }); |
# Conflicts: # packages/melonjs/CHANGELOG.md
obiot
added a commit
to asadullahbro/melonJS
that referenced
this pull request
Jul 7, 2026
…ine-wide
Rework of the originPoint proposal into the hybrid agreed in review:
Sprite3d anchors through the SAME settings.anchorPoint key, convention
and centered default as the 2D Sprite — no second anchor API — while
keeping the (verified-correct) vertex-bake mechanism from the original
commit.
- new shared resolver (renderable/anchorPoint.ts): the named presets
("center", "top", "bottom", ..., "bottom-right") now work everywhere
settings.anchorPoint does — Sprite, Entity, Collectable, ImageLayer,
Text, BitmapText (and subclasses), plus Sprite3d. On the 2D classes
invalid values keep their historical silent (0, 0) outcome for full
backward compatibility, but log a console warning; Sprite3d (a new
surface) throws. Values are never clamped; Vector2d instances and
bare {x, y} objects pass through unchanged.
- Sprite resolves the anchor once, up front, and forwards the RESOLVED
pair into the texture-cache descriptor (spritesheet atlases store it
as the cached per-frame pivot), so presets and {x, y} behave
identically on both paths and a raw string or aliased Vector2d never
lands in the shared cache.
- ImageLayer normalizes its bare-number shorthand pre-super (its Sprite
base consumes the same setting and the strict resolver rejects bare
numbers).
- Sprite3d wires the inherited ObservablePoint as the LIVE anchor:
setMuted + setCallback post-super; runtime anchorPoint.set() re-bakes
the quad (region path via _applyFrame, plain-quad path via
bakeQuadVertices) and re-derives the cull bounds (direct point
mutation — the Rect width setter's recalc() walks Mesh's repurposed
normals array and would throw).
- Mesh gains the _anchorBaked opt-out consulted by preDraw's per-frame
applyAnchorTransform recompute, so the anchor never leaks into the
renderer transform on either camera path (plain Mesh behavior
unchanged, webgl_mesh_anchor.spec.js untouched and green).
- cull bounds fixed: side = max(max(w,h), sqrt2*hypot(hw+|ox|, hh+|oy|))
— the sphere exactly encloses the anchored quad in any billboard
orientation (a bottom-anchored 130x225 character was ~47% under-sized
and popped out on screen); centered default byte-identical.
- billboard example: anchorPoint: "bottom", camera-target framing
restored (GY * 0.6 became dead code aiming at the floor), stale
comment fixed.
- tests: new tests/anchorpoint-presets.spec.js (cross-renderable
contract, 72 tests, failing-first) + 9 geometry/runtime/cull tests in
sprite3d.spec.js (failing-first). Full suite 4698 passed / 0 failed.
Master merged in (CHANGELOG + the melonjs#1539-melonjs#1541 fixes).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A
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
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.
Batch 3 (final) of the bug-hunt fixes — the audio cluster, all test-first (7/7 new tests red on the old code, 7/7 green after).
Loading & retries
audio.load()was the only asset parser without an already-loaded guard: re-preloading a manifest (returning to a stage that preloads is a common pattern) silently replaced each Howl and leaked the old instance's decoded buffers / HTML5 nodes. Now returns0(cached), matching every other parser;unload()first to genuinely reload.soundLoadErrorused one globalretryCounteracross all sounds, so three failures of one flaky file pushed another sound's first failure straight over the give-up threshold — a spurious fatal preload error under the defaultstopOnAudioError. Retries are now per-sound (state.retryCounterskeyed by name, cleared on load).stopOnAudioErrorwas documented as settable but couldn't be set — it's an ESMletexport;me.audio.stopOnAudioError = falsethrows a TypeError (module namespace properties are read-only), making the documented "disable audio instead of throwing" mode unreachable, forever. Newme.audio.setStopOnAudioError(value)setter; reads through the namespace keep working (live binding).API contract honesty (the #1456 follow-through)
seek()/rate()setter forms returned Howler'sHowlobject while typednumber— the exact get/set lie class feat(audio): procedural audio primitives (tone + noise) + audio.ts cleanup #1456 fixed forstereo/position/orientation/panner, missed on these two. Now honest overloads with arity-exact forwarding — Howler's core methods dispatch onarguments.length, so naively passing a trailing explicitundefinedwould be parsed as an instance id (parseInt(undefined) → NaN); the fix forwards exactly the arguments given.position()/stereo()getters returnednullwhen never set — Howler keeps group state atnulluntil first written; the getters passed it through while typed tuple / number. Now neutral defaults:[0, 0, 0]/0.resume()without an id could spawn a new instance — Howler's bareplay()auto-resumes only when exactly one instance is paused; with 2+ (e.g.pause()without id pauses the whole group) it started a brand-new playback from 0 and left the paused instances stuck forever.resume()now resumes every paused instance — which is what its JSDoc promised all along. ReadsHowl._sounds(no public instance list exists; same justified private access as the existing Howler_mutedread).unload()leftgetCurrentTrack()pointing at a ghost — the current-track pointer is now cleared when the unloaded clip is the active track.Test notes
tests/audio-audit.spec.js(7 tests, silent-WAV data-URL harness from audio.spec.js). The resume test asserts on the instance count (_sounds.lengthstays 2, ids unchanged) rather than playback state — deterministic even when the headless audio context is locked, where_pauseddoesn't flip synchronously; on the old code the bareplay()deterministically allocates a 3rd instance.Full suite: 4604 passed / 0 failed / 15 skipped; eslint + biome + build clean.
Note: #1540 (loader/geometry batch) touches the same CHANGELOG section — whichever merges second will need a trivial conflict resolution there.
🤖 Generated with Claude Code
https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A