From 65188aa48ea58a1a8c49849596ccf158a7d10494 Mon Sep 17 00:00:00 2001 From: Olivier Biot Date: Thu, 2 Jul 2026 19:46:05 +0800 Subject: [PATCH] fix(loader): unloadAll threw on fontfaces and never freed videos MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three long-latent bugs in the unload path, surfaced by #1534's pre-merge review: - unloadAll's video-cache sweep dispatched entries as type "json" (copy- paste from the loop above), so the jsonList lookup missed and video assets survived every unloadAll. - unloadAll's font-cache sweep used type "font" — an unknown type that hit unload()'s default throw, ABORTING the whole unloadAll mid-way (obj/mtl/ gltf/shader/audio sweeps never ran) whenever any fontface was loaded. (Its comment also said "video resources" — same copy-paste.) - The fontface unload case guarded on `typeof typeof globalThis.document` — always "string", never "undefined" — so the DOM check was inert. Regression test seeds videoList + fontList (real FontFace) and asserts unloadAll doesn't throw and empties both — verified to FAIL against the old code and pass with the fix. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A --- packages/melonjs/CHANGELOG.md | 1 + packages/melonjs/src/loader/loader.js | 8 ++++---- packages/melonjs/tests/loader.spec.js | 22 ++++++++++++++++++++++ 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/packages/melonjs/CHANGELOG.md b/packages/melonjs/CHANGELOG.md index 69797f5dc..9627297c4 100644 --- a/packages/melonjs/CHANGELOG.md +++ b/packages/melonjs/CHANGELOG.md @@ -13,6 +13,7 @@ - **Shader preloading: `"shader"` asset type + `loader.getShader()` + `ShaderEffect.clone()`** — custom shaders can now be preloaded like any other asset and are **compiled at load time** (the GLSL compile cost lands in the loading screen, and a compile error fails the load — `loader.load()` rejects with an error carrying the asset's name). The source is a GLSL fragment body following the `ShaderEffect` convention (`uniform` declarations + `vec4 apply(vec4 color, vec2 uv)`), loaded from a `src` URL / data: URI or inline via the `data` field (the inline-TMX convention): `{ name: "waterRipple", type: "shader", src: "shaders/waterRipple.frag" }`. `loader.getShader(name)` returns the **shared, loader-owned instance** — the *same* object on every call, with `shared = true` so renderable cleanup never auto-destroys it; everything it is assigned to shares one set of uniform values, and it is freed only by `loader.unload()` / `loader.unloadAll()` (which destroy the GL program). For per-renderable uniform values, the new `ShaderEffect.clone()` compiles an independent, caller-owned copy: it copies the *recipe* (fragment source, precision, uniform values, `setTexture` bindings — with its own GL uploads) but never the *ownership* — the clone's `shared` flag is **always reset to `false`**, so it is auto-destroyed with the renderable it is assigned to unless explicitly re-shared. `GLShader.clone()` exists likewise for raw full-program (vertex + fragment) shaders, with the same recipe-not-ownership semantics. The **Water Refraction** example now ships its fragment as a preloaded shader asset. ### Fixed +- **`loader.unloadAll()` threw if any fontface was loaded, and never actually freed videos** — two copy-paste type strings: the video-cache sweep dispatched entries as `type: "json"` (the lookup missed, so video assets survived every `unloadAll`), and the font-cache sweep used `type: "font"` — an unknown type that hit the loader's `unload` error path and **aborted the whole `unloadAll` mid-way**. Both now route through their correct types (`"video"` / `"fontface"`). Also fixed an always-true `typeof typeof globalThis.document` guard in the fontface unload case. - **Physics startup banner logged a minifier-mangled class name** — the `Application` console header derived the physics adapter label from `adapter.constructor.name`, which is renamed in minified builds, so the built-in adapter printed e.g. `physics: ry`. It now prefers the adapter's stable `physicLabel` before falling back to the class name, so the built-in adapter reads `physics: builtin` (external adapters that set `name` are unaffected). - **`Text` generic CSS font families silently fell back to serif** — `setFont` quoted *every* family name, which turned a CSS generic keyword (`sans-serif`, `monospace`, `serif`, `system-ui`, …) into a quoted, nonexistent specific family, so the browser rendered its default serif instead. Generic families are now left unquoted; specific names (e.g. `"Arial"`, `"My Font"`) are still quoted so names with spaces keep working. - **Animated `NineSliceSprite` collapsed to its frame size** (#1115) — a `NineSliceSprite` driven by an animation (or any per-frame texture change) had its user-specified "expanded" `width`/`height` overwritten by each frame's source dimensions, shrinking the 9-slice panel to a single frame after the first animation step. Static nine-slices were unaffected because the frame is applied only once (during construction, before the expanded size is set). `NineSliceSprite` now applies a new frame by swapping only the source sub-texture, leaving the expanded size and bounds intact. First-ever test coverage for `NineSliceSprite` (the class previously had none). diff --git a/packages/melonjs/src/loader/loader.js b/packages/melonjs/src/loader/loader.js index dc92dedd7..27d91ba5d 100644 --- a/packages/melonjs/src/loader/loader.js +++ b/packages/melonjs/src/loader/loader.js @@ -640,7 +640,7 @@ export function unload(asset) { case "fontface": if ( - typeof typeof globalThis.document !== "undefined" && + typeof globalThis.document !== "undefined" && typeof globalThis.document.fonts !== "undefined" ) { globalThis.document.fonts.delete(fontList[asset.name]); @@ -779,17 +779,17 @@ export function unloadAll() { if (videoList.hasOwnProperty(name)) { unload({ name: name, - type: "json", + type: "video", }); } } - // unload all video resources + // unload all font resources for (name in fontList) { if (fontList.hasOwnProperty(name)) { unload({ name: name, - type: "font", + type: "fontface", }); } } diff --git a/packages/melonjs/tests/loader.spec.js b/packages/melonjs/tests/loader.spec.js index 0901506f4..bb63f96f0 100644 --- a/packages/melonjs/tests/loader.spec.js +++ b/packages/melonjs/tests/loader.spec.js @@ -1,5 +1,6 @@ import { beforeAll, describe, expect, it } from "vitest"; import { audio, boot, event, loader } from "../src/index.js"; +import { fontList, videoList } from "../src/loader/cache.js"; describe("loader", () => { let audioURI; @@ -655,4 +656,25 @@ describe("loader", () => { expect(rejected).toBe(true); }); }); + + // regression: unloadAll used to dispatch videoList entries as type "json" + // (so videos were never removed) and fontList entries as type "font" (an + // unknown type — unloadAll THREW if any fontface had ever been loaded). + // Runs last: unloadAll wipes every cache this spec has populated. + describe("unloadAll routes every cache through its own type", () => { + it("unloads video + fontface entries without throwing", () => { + // seed the caches directly — the bug lived purely in unloadAll's + // per-list type strings, which seeding exercises in full + const ff = new FontFace("unloadall-test", "url(data:font/woff2;base64,)"); + globalThis.document.fonts.add(ff); + fontList["unloadall-test"] = ff; + videoList["unloadall-test"] = document.createElement("video"); + + expect(() => { + loader.unloadAll(); + }).not.toThrow(); + expect("unloadall-test" in fontList).toBe(false); + expect("unloadall-test" in videoList).toBe(false); + }); + }); });