From f241a122173a2893aade0dd245b193c7e98d7c47 Mon Sep 17 00:00:00 2001 From: Olivier Biot Date: Sun, 9 Aug 2026 12:17:55 +0800 Subject: [PATCH 1/4] Application.destroy() releases the WebGL context MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Teardown deleted every GL object the renderer owned and removed the canvas from the DOM, but never handed back the context itself. A canvas keeps its context until the canvas is garbage-collected — non-deterministic and routinely delayed — so each destroyed application left a live context behind. Browsers cap how many they keep (~16 on Chromium) and force-lose the oldest past that. A long-lived page that builds and tears down several applications therefore accumulates dead-but-unfreed contexts until an unrelated later getContext() stalls or returns one already lost. That hits any SPA that unmounts a game view — the examples gallery does exactly this on every navigation — and it is also what makes unrelated specs time out in CI, where the shared browser session spans every spec file. WebGLRenderer.destroy() now releases the context through WEBGL_lose_context. The hint had been sitting commented out in this same file since forever (webgl_renderer.js:295-296). destroy() stays idempotent — GL calls on a lost context are no-ops by spec — and it is already terminal (Application.init() refuses to run again afterwards), so losing the context forecloses nothing. Drivers without the extension are unaffected. webgl_vao_teardown.spec.js records the new contract: the old test asserted getError() === NO_ERROR after destroy, which a deliberately-lost context cannot satisfy; it now asserts isContextLost() and that a second teardown does not throw. Verified to fail with the fix reverted. A "create/destroy N applications past the context cap" test was written and deliberately REMOVED — it passed identically with and without the fix, so it discriminated nothing. The reasoning is left as a comment so the dead end is not re-derived. Also drops the unnecessary Application from octree_adversarial.spec.js: it only ever needed a `world` for the isFloating branch and nothing there floats, so it now stands up no canvas at all (import 280ms -> 12ms). Full suite 234 files / 5851 pass, eslint 0 errors, biome clean, tsc clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QVjYzf76AEU3wJk766JAQi --- packages/melonjs/CHANGELOG.md | 1 + .../melonjs/src/video/webgl/webgl_renderer.js | 18 +++++++++++ .../melonjs/tests/octree_adversarial.spec.js | 31 ++++++++++--------- .../melonjs/tests/webgl_vao_teardown.spec.js | 31 +++++++++++++++++-- 4 files changed, 64 insertions(+), 17 deletions(-) diff --git a/packages/melonjs/CHANGELOG.md b/packages/melonjs/CHANGELOG.md index fdd191650..76b4ea195 100644 --- a/packages/melonjs/CHANGELOG.md +++ b/packages/melonjs/CHANGELOG.md @@ -65,6 +65,7 @@ The old path scales linearly with vertex count; the new one is flat, because no - **the cost of `antiAlias: true` under post effects, quantified** ([#1556](https://github.com/melonjs/melonJS/issues/1556)) — MSAA composing through effect chains (see *Added*) is paid for in memory and bandwidth, and the price is worth knowing. Arithmetic, not measurement: a 4× capture target keeps 4 color + 4 depth-stencil samples per pixel next to its 1× resolve texture — roughly **28 extra bytes per pixel on WebGL (~55 MB of GPU memory at 1080p)** and **~16 bytes per pixel on WebGPU (~32 MB at 1080p)**, where the multisampled depth attachment is shared with the canvas rather than per-target; both scale linearly with resolution. Per frame it adds one resolve blit per effect bracket, and draws inside the bracket write up to 4 samples per covered pixel — bandwidth, not shading cost, since fragment shaders still run once per pixel under MSAA. Only scene **capture** targets pay any of this (ping-pong intermediates stay 1×), and with `antiAlias: false` — the default — no multisampled storage exists at all, so nothing changes ### Fixed +- **`Application.destroy()` leaked the WebGL context** — teardown deleted every GL object the renderer owned and removed the canvas from the DOM, but never handed back the **context** itself. A canvas keeps its context until the canvas is garbage-collected, which is non-deterministic and routinely delayed, so each destroyed application left a live context behind. Browsers cap how many they keep — around 16 on Chromium — and force-lose the oldest past that, which means a long-lived page that builds and tears down several applications accumulates dead-but-unfreed contexts until an unrelated later `getContext` stalls or comes back already lost. That hits any single-page app that moves between scenes or unmounts a game view (the examples gallery does exactly this on every navigation), and it was also making unrelated test suites time out in CI. `destroy()` now releases the context through `WEBGL_lose_context`. It stays idempotent, and since `destroy()` is already terminal — `Application.init()` refuses to run again afterwards — losing the context forecloses nothing that was previously possible. Renderers whose driver does not expose the extension are unaffected - **the 3D broadphase silently dropped collisions between bodies at different depths** — under `Camera3d` the world's broadphase is an `Octree`, and `retrieve()` — the candidate feed for SAT collision, pointer picking, the 2D raycast and `adapter.queryAABB` — descended only into the octant the query item itself classified into. But every one of those consumers decides overlap in the **XY plane**: two bodies at different z that overlap in XY genuinely collide, and were never offered to each other as candidates. Whether a given pair was tested came down to which side of an octant boundary each happened to fall on. Measured on a randomized 300-body scene, **12 of 20 genuinely overlapping pairs were never surfaced**. `retrieve()` is now depth-blind: it classifies on x/y only and walks both depth halves of that quadrant, so x/y pruning still applies at every level and in both halves (an item lying wholly inside a different x/y quadrant cannot overlap, and midpoint-straddling items already live at the parent level). On a 600-body scene this costs nothing at all when the bodies share a gameplay plane — the candidate count is unchanged — and on a depth-spread scene it settles at the same candidate count as the flat one, which is the point: with depth no longer part of the decision, the candidate set depends only on the x/y distribution. The genuinely 3D queries are unaffected and still prune on depth: `queryAABB`, `querySphere`, `queryRay` and `queryFrustum` have their own entry points, and each is now pinned by a differential test against a brute-force scan. Note this removes the incidental "parallax at a distant z drops out of collision for free" behaviour that the 2.5D documentation described as best-effort — it was this defect seen from its good side. Exclude parallax deliberately instead, with `isKinematic = true` or `collisionType` / `collisionMask`, which is what the 2D path has always done - **an entire 2.5D gameplay plane sat unpartitioned at the root of the octree** — `getIndex` returned −1 (meaning "straddles a midpoint, keep at this level") for an item sitting *exactly* on one. On x and y that is at least defensible, since an item there may genuinely span the boundary; on z it never is, because items are point-z in the broadphase and a point cannot straddle anything. It mattered because the root box is origin-centred, so its midpoints are `(0, 0, 0)` — the default `pos` of every renderable, and the shared gameplay z that the 2.5D recipe prescribes. Measured: 200 bodies on a `z = 0` plane all stayed at the root and `retrieve()` returned **200 of 200**, degrading the broadphase to a linear scan for exactly the layer holding the most bodies; the same 200 spread across z left only 10 at the root. Classification is now exact on all three axes — a midpoint belongs to the far/right/bottom child, and an item whose far edge merely touches one still counts as wholly inside the near side. Genuine straddlers and out-of-bounds items still stay at the parent, both under regression test. This is invisible to 2D games, which use a `QuadTree` and never construct an `Octree` - **a mesh marked `lit` with no usable normals rendered solid black** — normalizing a zero-length normal yields NaN, which the shader turned into black fragments rather than something recognisable. That happens whenever `lit: true` meets geometry with no normals, and on the 2D-camera path generally, where world normals are never written. Such a mesh now degrades to **unlit** on both GPU backends: wrong, but recognisably the model instead of a hole in the scene. Note this makes the failure legible, it does not make a `Camera2d` mesh light — populating world normals on that path is tracked separately as [#1576](https://github.com/melonjs/melonJS/issues/1576) and remains open diff --git a/packages/melonjs/src/video/webgl/webgl_renderer.js b/packages/melonjs/src/video/webgl/webgl_renderer.js index 228150271..ba3248331 100644 --- a/packages/melonjs/src/video/webgl/webgl_renderer.js +++ b/packages/melonjs/src/video/webgl/webgl_renderer.js @@ -489,6 +489,24 @@ export default class WebGLRenderer extends Renderer { this.gl.deleteBuffer(this.vertexBuffer); this.vertexBuffer = null; } + + // Release the GL context itself. Dropping every GL object above and + // removing the canvas from the DOM does NOT do this: a canvas keeps + // its context until the canvas is garbage-collected, which is + // non-deterministic and routinely delayed. Browsers cap how many live + // contexts they keep (Chromium ~16) and force-lose the oldest past + // that, so a long-lived page that creates and tears down several + // applications — an SPA moving between scenes, or a test session + // sharing one page across spec files — accumulates dead-but-unfreed + // contexts until an unrelated later `getContext` stalls or fails. + // + // `WEBGL_lose_context` is the only way to hand a context back + // deterministically. `destroy()` is terminal (`Application.destroy` + // refuses a subsequent `init`), so losing it here forecloses nothing. + // The extension is absent on some drivers, hence the optional call. + if (this.isContextValid !== false) { + this.gl.getExtension("WEBGL_lose_context")?.loseContext(); + } } reset() { diff --git a/packages/melonjs/tests/octree_adversarial.spec.js b/packages/melonjs/tests/octree_adversarial.spec.js index 31663dc11..2401da116 100644 --- a/packages/melonjs/tests/octree_adversarial.spec.js +++ b/packages/melonjs/tests/octree_adversarial.spec.js @@ -11,8 +11,7 @@ * one that genuinely overlaps. A false negative is a silently missed * collision, which is the failure mode that does not announce itself. */ -import { beforeAll, beforeEach, describe, expect, it } from "vitest"; -import { Application, boot, video, World } from "../src/index.js"; +import { beforeEach, describe, expect, it } from "vitest"; import { AABB3d } from "../src/physics/broadphase/aabb3d.ts"; import Octree from "../src/physics/broadphase/octree.ts"; @@ -58,23 +57,27 @@ function overlaps2d(a, b) { ); } +/** + * The Octree only ever touches `world` to reach + * `world.app.viewport.localToWorld` on the `isFloating` branch, and nothing + * here is floating. So this stays a pure unit test: no `boot()`, no + * `Application`, no canvas. + * + * That is deliberate rather than incidental. Every spec that stands up an + * Application leaves a live canvas/context in the shared browser session for + * the rest of the run, and `helpers/webgl-context.js` documents the + * consequence — some unrelated spec's `beforeAll` times out later, blaming + * whichever file happened to run late. A test that does not need one should + * not create one. + */ +const stubWorld = {}; + describe("Octree — adversarial", () => { - let world; /** the ±10000 origin-centred root that `createBroadphase()` actually builds */ let octree; - beforeAll(async () => { - boot(); - const app = new Application(800, 600, { - parent: "screen", - scale: "auto", - renderer: video.CANVAS, - }); - await app.init(); - }); - beforeEach(() => { - world = new World(0, 0, 800, 600); + const world = stubWorld; const bounds = new AABB3d(); bounds.setMinMax(-10000, -10000, -10000, 10000, 10000, 10000); octree = new Octree(world, bounds, 4, 4, 0); diff --git a/packages/melonjs/tests/webgl_vao_teardown.spec.js b/packages/melonjs/tests/webgl_vao_teardown.spec.js index bec95bb77..7dbaf55fa 100644 --- a/packages/melonjs/tests/webgl_vao_teardown.spec.js +++ b/packages/melonjs/tests/webgl_vao_teardown.spec.js @@ -72,7 +72,7 @@ describe("WebGL batcher teardown releases GL objects", () => { expect(renderer.vertexBuffer).toBe(null); }); - it("destroy() is idempotent and does not queue GL errors", async (ctx) => { + it("destroy() releases the GL context, and is idempotent", async (ctx) => { requireWebGL(ctx); const app = new Application(48, 48, { parent: "screen", @@ -85,8 +85,33 @@ describe("WebGL batcher teardown releases GL objects", () => { while (gl.getError() !== gl.NO_ERROR) { /* drain */ } + app.destroy(); - app.renderer.destroy(); - expect(gl.getError()).toBe(gl.NO_ERROR); + + // `destroy()` now hands the context back via `WEBGL_lose_context`. + // Dropping the GL objects and removing the canvas from the DOM does + // NOT do this on its own — the context survives until the canvas is + // garbage-collected, and browsers force-lose the oldest once past + // their live-context cap (~16 on Chromium). A page that builds and + // tears down several applications therefore used to accumulate + // dead-but-unfreed contexts until an unrelated later `getContext` + // stalled. This assertion is what keeps that from regressing. + expect(gl.isContextLost()).toBe(true); + + // still idempotent: a second teardown on the now-lost context must + // not throw. GL calls on a lost context are no-ops by spec. + expect(() => { + app.renderer.destroy(); + }).not.toThrow(); }); }); + +// NOTE — a "create/destroy N applications past the browser's context cap" +// test was written here and REMOVED, because it passed identically with and +// without the fix. On a machine with a real GPU the cap is never reached at +// any N a unit test can afford, and eviction does not surface as a +// newly-created context reporting `isContextLost()` — it surfaces as the +// OLDEST context dying, and as `getContext` getting slower, neither of which +// is assertable cheaply or deterministically. The `isContextLost()` check +// above is the honest regression guard: it fails without the fix and passes +// with it. Left as a comment so nobody re-derives the dead end. From e245aa5af6a71d39941e2bb1eaad0776f9e88b24 Mon Sep 17 00:00:00 2001 From: Olivier Biot Date: Sun, 9 Aug 2026 12:23:04 +0800 Subject: [PATCH 2/4] tests: stop leaking WebGL contexts from spec-local Applications MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the destroy() context-release fix. An audit parsing the `renderer:` argument at each `new Application(` call site (not grepping filenames — camera3d_integration has 19 Applications but deliberately uses CANVAS) found 24 GL-capable Applications created in specs that never call destroy(). Against a Chromium cap of ~16 live contexts, that is what pushed unrelated suites' beforeAll hooks past their 90s timeout on CI. Two kinds of offender, two fixes: - Suites that never needed GL at all — bezier, linedash, timer — pinned to video.CANVAS. An unspecified renderer resolves to AUTO, so these were silently holding a WebGL context for the whole session. - "Reset-only" Applications: a fresh app built inside afterAll purely to restore global defaults (Camera2d, a clean world) for later spec files. Eight of these across depth, glcore-audit, webgl_save_restore, mesh, camera3d_integration, lighting3d, gltf_model and canvas-cliprect-transform took a context under AUTO and were never destroyed. They do not render, so they are now CANVAS. gltf_model was already using CANVAS for its real app and AUTO for the throwaway. Also adds afterAll teardown to bezier, linedash and timer: a Canvas Application still leaves a canvas, listeners and timers live in the shared browser session, so it should be destroyed whichever backend it uses. Full suite 234 files / 5851 pass, eslint 0 errors, biome clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QVjYzf76AEU3wJk766JAQi --- packages/melonjs/tests/bezier.spec.js | 15 +++++++++-- .../tests/camera3d_integration.spec.js | 5 +++- .../tests/canvas-cliprect-transform.spec.js | 5 +++- packages/melonjs/tests/depth.spec.js | 15 ++++++++--- packages/melonjs/tests/glcore-audit.spec.js | 5 +++- packages/melonjs/tests/gltf_model.spec.js | 5 +++- packages/melonjs/tests/lighting3d.spec.js | 5 +++- packages/melonjs/tests/linedash.spec.js | 13 ++++++++-- packages/melonjs/tests/mesh.spec.js | 5 +++- packages/melonjs/tests/timer.spec.js | 25 ++++++++++++++++--- .../melonjs/tests/webgl_save_restore.spec.js | 5 +++- 11 files changed, 86 insertions(+), 17 deletions(-) diff --git a/packages/melonjs/tests/bezier.spec.js b/packages/melonjs/tests/bezier.spec.js index 2b93a0cf4..c1430466c 100644 --- a/packages/melonjs/tests/bezier.spec.js +++ b/packages/melonjs/tests/bezier.spec.js @@ -1,6 +1,6 @@ -import { beforeAll, describe, expect, it } from "vitest"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; import Path2D from "../src/geometries/path2d.ts"; -import { Application } from "../src/index.js"; +import { Application, video } from "../src/index.js"; describe("Bezier Curves", () => { let app; @@ -9,10 +9,21 @@ describe("Bezier Curves", () => { app = new Application(128, 128, { parent: "screen", scale: "auto", + // Canvas: this suite exercises path geometry, not GL. Defaulting to + // AUTO took a WebGL context and never released it, and the browser + // caps how many it keeps — see webgl_vao_teardown.spec.js. + renderer: video.CANVAS, }); await app.init(); }); + afterAll(() => { + // tear the application down rather than leaving its canvas, + // listeners and timers live in the shared browser session for + // the rest of the run + app?.destroy(); + }); + describe("quadraticCurveTo", () => { it("should not throw when drawing a quadratic curve", () => { expect(() => { diff --git a/packages/melonjs/tests/camera3d_integration.spec.js b/packages/melonjs/tests/camera3d_integration.spec.js index a633c07a3..cebd050d5 100644 --- a/packages/melonjs/tests/camera3d_integration.spec.js +++ b/packages/melonjs/tests/camera3d_integration.spec.js @@ -37,7 +37,10 @@ describe("Camera3d × Stage × Application integration", () => { const app = new Application(800, 600, { parent: "screen", scale: "auto", - renderer: video.AUTO, + // Canvas: this app exists only to reset global state for later + // spec files. Under AUTO it took a WebGL context and was never + // destroyed — see webgl_vao_teardown.spec.js. + renderer: video.CANVAS, }); await app.init(); }); diff --git a/packages/melonjs/tests/canvas-cliprect-transform.spec.js b/packages/melonjs/tests/canvas-cliprect-transform.spec.js index 97f550872..bd3beb44c 100644 --- a/packages/melonjs/tests/canvas-cliprect-transform.spec.js +++ b/packages/melonjs/tests/canvas-cliprect-transform.spec.js @@ -37,7 +37,10 @@ describe("CanvasRenderer clipRect vs transforms", () => { try { const app = new Application(64, 64, { parent: "screen", - renderer: video.AUTO, + // Canvas: this app exists only to reset global state for later + // spec files. Under AUTO it took a WebGL context and was never + // destroyed — see webgl_vao_teardown.spec.js. + renderer: video.CANVAS, }); await app.init(); } catch { diff --git a/packages/melonjs/tests/depth.spec.js b/packages/melonjs/tests/depth.spec.js index 4292164af..77033caa5 100644 --- a/packages/melonjs/tests/depth.spec.js +++ b/packages/melonjs/tests/depth.spec.js @@ -99,7 +99,10 @@ describe("Renderer.setDepth", () => { const app = new Application(800, 600, { parent: "screen", scale: "auto", - renderer: video.AUTO, + // Canvas: reset-only app — it exists to restore global defaults + // for later spec files, not to render. Under AUTO it took a + // WebGL context and was never destroyed. + renderer: video.CANVAS, }); await app.init(); }); @@ -152,7 +155,10 @@ describe("Renderable.preDraw forwards depth", () => { const app = new Application(800, 600, { parent: "screen", scale: "auto", - renderer: video.AUTO, + // Canvas: reset-only app — it exists to restore global defaults + // for later spec files, not to render. Under AUTO it took a + // WebGL context and was never destroyed. + renderer: video.CANVAS, }); await app.init(); }); @@ -224,7 +230,10 @@ describe("WebGL batchers carry depth as vec3 aVertex (PR A)", () => { const app = new Application(800, 600, { parent: "screen", scale: "auto", - renderer: video.AUTO, + // Canvas: reset-only app — it exists to restore global defaults + // for later spec files, not to render. Under AUTO it took a + // WebGL context and was never destroyed. + renderer: video.CANVAS, }); await app.init(); }); diff --git a/packages/melonjs/tests/glcore-audit.spec.js b/packages/melonjs/tests/glcore-audit.spec.js index 1084143c3..bec9bda43 100644 --- a/packages/melonjs/tests/glcore-audit.spec.js +++ b/packages/melonjs/tests/glcore-audit.spec.js @@ -55,7 +55,10 @@ describe("video/GL core audit reproductions", () => { try { const app = new Application(64, 64, { parent: "screen", - renderer: video.AUTO, + // Canvas: reset-only app — it exists to restore global defaults + // for later spec files, not to render. Under AUTO it took a + // WebGL context and was never destroyed. + renderer: video.CANVAS, }); await app.init(); } catch { diff --git a/packages/melonjs/tests/gltf_model.spec.js b/packages/melonjs/tests/gltf_model.spec.js index a03ea8775..c16024ac6 100644 --- a/packages/melonjs/tests/gltf_model.spec.js +++ b/packages/melonjs/tests/gltf_model.spec.js @@ -121,7 +121,10 @@ describe("GLTFModel", () => { const app = new Application(800, 600, { parent: "screen", scale: "auto", - renderer: video.AUTO, + // Canvas: this app exists only to reset global state for later + // spec files. Under AUTO it took a WebGL context and was never + // destroyed — see webgl_vao_teardown.spec.js. + renderer: video.CANVAS, }); await app.init(); }); diff --git a/packages/melonjs/tests/lighting3d.spec.js b/packages/melonjs/tests/lighting3d.spec.js index db5169eb9..bc3e58fd9 100644 --- a/packages/melonjs/tests/lighting3d.spec.js +++ b/packages/melonjs/tests/lighting3d.spec.js @@ -88,7 +88,10 @@ describe("Light3d ↔ Stage registration", () => { const app = new Application(800, 600, { parent: "screen", scale: "auto", - renderer: video.AUTO, + // Canvas: this app exists only to reset global state for later + // spec files. Under AUTO it took a WebGL context and was never + // destroyed — see webgl_vao_teardown.spec.js. + renderer: video.CANVAS, }); await app.init(); }); diff --git a/packages/melonjs/tests/linedash.spec.js b/packages/melonjs/tests/linedash.spec.js index fb291fa87..0bf3337ea 100644 --- a/packages/melonjs/tests/linedash.spec.js +++ b/packages/melonjs/tests/linedash.spec.js @@ -1,5 +1,5 @@ -import { beforeAll, describe, expect, it } from "vitest"; -import { Application } from "../src/index.js"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import { Application, video } from "../src/index.js"; describe("LineDash", () => { let app; @@ -8,10 +8,19 @@ describe("LineDash", () => { app = new Application(64, 64, { parent: "screen", scale: "auto", + // Canvas: dash state is a 2D-context concern; no GL needed here. + renderer: video.CANVAS, }); await app.init(); }); + afterAll(() => { + // tear the application down rather than leaving its canvas, + // listeners and timers live in the shared browser session for + // the rest of the run + app?.destroy(); + }); + describe("setLineDash / getLineDash", () => { it("should default to an empty array (solid line)", () => { expect(app.renderer.getLineDash()).toEqual([]); diff --git a/packages/melonjs/tests/mesh.spec.js b/packages/melonjs/tests/mesh.spec.js index ef03a2ffd..59ebfea2b 100644 --- a/packages/melonjs/tests/mesh.spec.js +++ b/packages/melonjs/tests/mesh.spec.js @@ -1015,7 +1015,10 @@ describe("Mesh × Camera3d world-space path", () => { const app = new Application(800, 600, { parent: "screen", scale: "auto", - renderer: video.AUTO, + // Canvas: this app exists only to reset global state for later + // spec files. Under AUTO it took a WebGL context and was never + // destroyed — see webgl_vao_teardown.spec.js. + renderer: video.CANVAS, }); await app.init(); }); diff --git a/packages/melonjs/tests/timer.spec.js b/packages/melonjs/tests/timer.spec.js index 1c284f93f..82eb3774c 100644 --- a/packages/melonjs/tests/timer.spec.js +++ b/packages/melonjs/tests/timer.spec.js @@ -1,12 +1,31 @@ -import { beforeAll, describe, expect, onTestFinished, test, vi } from "vitest"; -import { Application, timer } from "../src/index.js"; +import { + afterAll, + beforeAll, + describe, + expect, + onTestFinished, + test, + vi, +} from "vitest"; +import { Application, timer, video } from "../src/index.js"; describe("Timer", () => { + let app; + beforeAll(async () => { - const app = new Application(100, 100); + // Canvas: this suite only drives the timer; an unspecified renderer + // resolved to WebGL and held a context for the whole session. + app = new Application(100, 100, { renderer: video.CANVAS }); await app.init(); }); + afterAll(() => { + // tear the application down rather than leaving its canvas, + // listeners and timers live in the shared browser session for + // the rest of the run + app?.destroy(); + }); + describe("setTimeout", () => { test("calls the provided function when enough time have elapsed", async () => { const fn = vi.fn(); diff --git a/packages/melonjs/tests/webgl_save_restore.spec.js b/packages/melonjs/tests/webgl_save_restore.spec.js index 1b4efe621..c140832d1 100644 --- a/packages/melonjs/tests/webgl_save_restore.spec.js +++ b/packages/melonjs/tests/webgl_save_restore.spec.js @@ -37,7 +37,10 @@ describe("WebGL Renderer save/restore", () => { const app = new Application(800, 600, { parent: "screen", scale: "auto", - renderer: video.AUTO, + // Canvas: reset-only app — it exists to restore global defaults + // for later spec files, not to render. Under AUTO it took a + // WebGL context and was never destroyed. + renderer: video.CANVAS, }); await app.init(); }); From eb1c1febf04d56a3cc2e21d2356cf216b3aa6f75 Mon Sep 17 00:00:00 2001 From: Olivier Biot Date: Sun, 9 Aug 2026 12:35:25 +0800 Subject: [PATCH 3/4] Renderers unregister their global event listeners on destroy() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WebGLRenderer subscribed to GAME_RESET, ONCONTEXT_RESTORED and CANVAS_ONRESIZE; CanvasRenderer to GAME_RESET. All four were inline anonymous arrows, which cannot be passed to off() — so nothing could ever unregister them. CanvasRenderer had no destroy() at all, inheriting the base class no-op. Two consequences, both silent. A destroyed renderer kept reacting to those events. And each handler closes over the renderer, so the subscription pinned the renderer, its batchers and its GPU objects against garbage collection — which is why releasing the GL context in the previous commit was not sufficient on its own: the JS graph stayed reachable from the event bus. The handlers are now per-instance fields and destroy() calls off() on each, matching what WebGPURenderer already did (it stores this.onGameReset / this.onCanvasResize and unregisters both). Adds tests/application_lifecycle.spec.js. It asserts the structural property that made the bug possible — handlers must be retrievable per-instance references, and CanvasRenderer must define its own destroy(). Two stronger tests were attempted and abandoned, and the spec records why so the dead ends are not re-walked: - "destroy, then emit(GAME_RESET), assert no reaction" — emit reaches every listener in the shared browser session, including ones left by other spec files, and throws partway through. A try/catch around it would pass without reaching the handler under test. - "spy on event.off" — vitest browser mode cannot spy on ESM exports. The structural assertion is necessary but not sufficient: it would not catch a destroy() that simply forgot to call off(). A listener-count assertion would be strictly better and needs a test-visible way to inspect the bus. Said so in the spec rather than implying more coverage than there is. Note World (GAME_RESET) and Container (CANVAS_ONRESIZE) have the same unpaired-subscription shape and are NOT fixed here — Container matters most since every container in the scene graph takes one. Full suite 235 files / 5854 pass, eslint 0 errors, biome clean, tsc clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QVjYzf76AEU3wJk766JAQi --- packages/melonjs/CHANGELOG.md | 1 + .../src/video/canvas/canvas_renderer.js | 23 +++- .../melonjs/src/video/webgl/webgl_renderer.js | 46 +++++-- .../tests/application_lifecycle.spec.js | 117 ++++++++++++++++++ 4 files changed, 171 insertions(+), 16 deletions(-) create mode 100644 packages/melonjs/tests/application_lifecycle.spec.js diff --git a/packages/melonjs/CHANGELOG.md b/packages/melonjs/CHANGELOG.md index 76b4ea195..bf8b15e21 100644 --- a/packages/melonjs/CHANGELOG.md +++ b/packages/melonjs/CHANGELOG.md @@ -65,6 +65,7 @@ The old path scales linearly with vertex count; the new one is flat, because no - **the cost of `antiAlias: true` under post effects, quantified** ([#1556](https://github.com/melonjs/melonJS/issues/1556)) — MSAA composing through effect chains (see *Added*) is paid for in memory and bandwidth, and the price is worth knowing. Arithmetic, not measurement: a 4× capture target keeps 4 color + 4 depth-stencil samples per pixel next to its 1× resolve texture — roughly **28 extra bytes per pixel on WebGL (~55 MB of GPU memory at 1080p)** and **~16 bytes per pixel on WebGPU (~32 MB at 1080p)**, where the multisampled depth attachment is shared with the canvas rather than per-target; both scale linearly with resolution. Per frame it adds one resolve blit per effect bracket, and draws inside the bracket write up to 4 samples per covered pixel — bandwidth, not shading cost, since fragment shaders still run once per pixel under MSAA. Only scene **capture** targets pay any of this (ping-pong intermediates stay 1×), and with `antiAlias: false` — the default — no multisampled storage exists at all, so nothing changes ### Fixed +- **destroyed renderers stayed subscribed to global events forever** — `WebGLRenderer` subscribed to `GAME_RESET`, `ONCONTEXT_RESTORED` and `CANVAS_ONRESIZE`, and `CanvasRenderer` to `GAME_RESET`, all as **inline anonymous handlers** — which cannot be passed to `off()`, so nothing could ever unregister them. `CanvasRenderer` had no `destroy()` at all, inheriting the base no-op. Two consequences, both silent: a destroyed renderer kept reacting to those events, and — worse — each handler closes over the renderer, so the subscription pinned the renderer, its batchers and its GPU objects against garbage collection. Releasing the GL context alone did not help, because the JS graph was still reachable from the event bus. The handlers are now per-instance fields and `destroy()` unregisters all of them, matching what the WebGPU backend already did. Any application that tears down and rebuilds — an SPA moving between scenes, a level reload — stops accumulating them - **`Application.destroy()` leaked the WebGL context** — teardown deleted every GL object the renderer owned and removed the canvas from the DOM, but never handed back the **context** itself. A canvas keeps its context until the canvas is garbage-collected, which is non-deterministic and routinely delayed, so each destroyed application left a live context behind. Browsers cap how many they keep — around 16 on Chromium — and force-lose the oldest past that, which means a long-lived page that builds and tears down several applications accumulates dead-but-unfreed contexts until an unrelated later `getContext` stalls or comes back already lost. That hits any single-page app that moves between scenes or unmounts a game view (the examples gallery does exactly this on every navigation), and it was also making unrelated test suites time out in CI. `destroy()` now releases the context through `WEBGL_lose_context`. It stays idempotent, and since `destroy()` is already terminal — `Application.init()` refuses to run again afterwards — losing the context forecloses nothing that was previously possible. Renderers whose driver does not expose the extension are unaffected - **the 3D broadphase silently dropped collisions between bodies at different depths** — under `Camera3d` the world's broadphase is an `Octree`, and `retrieve()` — the candidate feed for SAT collision, pointer picking, the 2D raycast and `adapter.queryAABB` — descended only into the octant the query item itself classified into. But every one of those consumers decides overlap in the **XY plane**: two bodies at different z that overlap in XY genuinely collide, and were never offered to each other as candidates. Whether a given pair was tested came down to which side of an octant boundary each happened to fall on. Measured on a randomized 300-body scene, **12 of 20 genuinely overlapping pairs were never surfaced**. `retrieve()` is now depth-blind: it classifies on x/y only and walks both depth halves of that quadrant, so x/y pruning still applies at every level and in both halves (an item lying wholly inside a different x/y quadrant cannot overlap, and midpoint-straddling items already live at the parent level). On a 600-body scene this costs nothing at all when the bodies share a gameplay plane — the candidate count is unchanged — and on a depth-spread scene it settles at the same candidate count as the flat one, which is the point: with depth no longer part of the decision, the candidate set depends only on the x/y distribution. The genuinely 3D queries are unaffected and still prune on depth: `queryAABB`, `querySphere`, `queryRay` and `queryFrustum` have their own entry points, and each is now pinned by a differential test against a brute-force scan. Note this removes the incidental "parallax at a distant z drops out of collision for free" behaviour that the 2.5D documentation described as best-effort — it was this defect seen from its good side. Exclude parallax deliberately instead, with `isKinematic = true` or `collisionType` / `collisionMask`, which is what the 2D path has always done - **an entire 2.5D gameplay plane sat unpartitioned at the root of the octree** — `getIndex` returned −1 (meaning "straddles a midpoint, keep at this level") for an item sitting *exactly* on one. On x and y that is at least defensible, since an item there may genuinely span the boundary; on z it never is, because items are point-z in the broadphase and a point cannot straddle anything. It mattered because the root box is origin-centred, so its midpoints are `(0, 0, 0)` — the default `pos` of every renderable, and the shared gameplay z that the 2.5D recipe prescribes. Measured: 200 bodies on a `z = 0` plane all stayed at the root and `retrieve()` returned **200 of 200**, degrading the broadphase to a linear scan for exactly the layer holding the most bodies; the same 200 spread across z left only 10 at the root. Classification is now exact on all three axes — a midpoint belongs to the far/right/bottom child, and an item whose far edge merely touches one still counts as wholly inside the near side. Genuine straddlers and out-of-bounds items still stay at the parent, both under regression test. This is invisible to 2D games, which use a `QuadTree` and never construct an `Octree` diff --git a/packages/melonjs/src/video/canvas/canvas_renderer.js b/packages/melonjs/src/video/canvas/canvas_renderer.js index dd637c1e6..a85153c84 100644 --- a/packages/melonjs/src/video/canvas/canvas_renderer.js +++ b/packages/melonjs/src/video/canvas/canvas_renderer.js @@ -4,6 +4,7 @@ import { GAME_RESET, ONCONTEXT_LOST, ONCONTEXT_RESTORED, + off, on, } from "../../system/event.ts"; import { Gradient } from "./../gradient.js"; @@ -74,10 +75,26 @@ export default class CanvasRenderer extends Renderer { false, ); - // reset the renderer on game reset - on(GAME_RESET, () => { + // Held as a bound field, not an inline arrow, so `destroy()` can + // unregister it — an anonymous handler cannot be passed to `off()`, + // and the closure would otherwise pin this renderer forever. + this.onGameReset = () => { this.reset(); - }); + }; + + // reset the renderer on game reset + on(GAME_RESET, this.onGameReset); + } + + /** + * Release the resources held by this renderer. The Canvas backend owns + * no GPU objects, but it does hold an event subscription whose closure + * keeps the renderer (and its canvas) reachable — so a torn-down + * application would otherwise leak one listener per teardown and keep + * reacting to `GAME_RESET` after it was destroyed. + */ + destroy() { + off(GAME_RESET, this.onGameReset); } /** diff --git a/packages/melonjs/src/video/webgl/webgl_renderer.js b/packages/melonjs/src/video/webgl/webgl_renderer.js index ba3248331..3f05431e5 100644 --- a/packages/melonjs/src/video/webgl/webgl_renderer.js +++ b/packages/melonjs/src/video/webgl/webgl_renderer.js @@ -8,6 +8,7 @@ import { GAME_RESET, ONCONTEXT_LOST, ONCONTEXT_RESTORED, + off, on, RENDER_TARGET_CHANGED, } from "../../system/event.ts"; @@ -342,10 +343,28 @@ export default class WebGLRenderer extends Renderer { false, ); - // reset the renderer on game reset - on(GAME_RESET, () => { + // Held as bound fields rather than inline arrows so `destroy()` can + // actually unregister them. An anonymous handler cannot be passed to + // `off()`, so every torn-down renderer used to leave its listeners on + // the bus forever — and each closure pins the renderer, its batchers + // and its GL objects against garbage collection, so releasing the GL + // context alone was not enough to let the canvas go. + this.onGameReset = () => { this.reset(); - }); + }; + this.onContextRestoredInvalidate = (renderer) => { + if (renderer === this) { + this.currentProgram = undefined; + } + }; + this.onCanvasResize = (width, height) => { + this.flush(); + this.setViewport(0, 0, width, height); + // FBOs are lazily resized in beginPostEffect via get() → resize() + }; + + // reset the renderer on game reset + on(GAME_RESET, this.onGameReset); // Every live GLShader recompiles on this event, and each recompile // binds its own program to replay its uniform snapshot — so the @@ -358,18 +377,10 @@ export default class WebGLRenderer extends Renderer { // Light2dBlock — a foreign mesh-lit program parks its block at // default binding point 0, whose 2D buffer is now too small, and a // plain sprite flush dies with INVALID_OPERATION. - on(ONCONTEXT_RESTORED, (renderer) => { - if (renderer === this) { - this.currentProgram = undefined; - } - }); + on(ONCONTEXT_RESTORED, this.onContextRestoredInvalidate); // register to the CANVAS resize channel - on(CANVAS_ONRESIZE, (width, height) => { - this.flush(); - this.setViewport(0, 0, width, height); - // FBOs are lazily resized in beginPostEffect via get() → resize() - }); + on(CANVAS_ONRESIZE, this.onCanvasResize); } /** @@ -474,6 +485,15 @@ export default class WebGLRenderer extends Renderer { } destroy() { + // Unregister first: every handler closes over `this`, so leaving them + // on the bus keeps the renderer (and transitively its batchers and GL + // objects) reachable forever — a destroyed renderer would also still + // react to GAME_RESET and CANVAS_ONRESIZE. Matches what the WebGPU + // backend already does. + off(GAME_RESET, this.onGameReset); + off(ONCONTEXT_RESTORED, this.onContextRestoredInvalidate); + off(CANVAS_ONRESIZE, this.onCanvasResize); + // the shared ground-shadow quads (#1515) hold retained GPU geometry // keyed off this renderer's batchers — released before those go releaseShadowQuads(this); diff --git a/packages/melonjs/tests/application_lifecycle.spec.js b/packages/melonjs/tests/application_lifecycle.spec.js new file mode 100644 index 000000000..72651dd8d --- /dev/null +++ b/packages/melonjs/tests/application_lifecycle.spec.js @@ -0,0 +1,117 @@ +import { beforeAll, describe, expect, it } from "vitest"; +import { + Application, + boot, + CanvasRenderer, + video, + WebGLRenderer, +} from "../src/index.js"; + +/** + * Application create / destroy lifecycle. + * + * The failure this guards against is quiet: a renderer subscribes to global + * events in its constructor, and if `destroy()` cannot unregister them the + * handlers stay on the bus for the life of the page. Each closure captures + * the renderer, so it pins the renderer, its batchers and its GPU objects + * against garbage collection — and a destroyed renderer keeps reacting to + * `GAME_RESET` / `CANVAS_ONRESIZE` long after its application is gone. + * + * Nothing about that is visible in a normal run; it surfaces as an unrelated + * suite timing out much later, once enough teardowns have piled up. That is + * exactly how it reached CI. + * + * ## What these can and cannot assert + * + * Two stronger tests were attempted and abandoned, recorded here so the dead + * ends are not re-walked: + * + * 1. "destroy, then `emit(GAME_RESET)`, assert the dead renderer did not + * react". `emit` reaches EVERY listener in the shared browser session, + * including ones left behind by other spec files whose state is long gone, + * and throws partway through. Wrapping it in try/catch would let the test + * pass without ever reaching the handler under test — a silent pass. + * 2. "spy on `event.off` and assert it was called with the registered + * handler". Vitest browser mode cannot spy on ESM exports (module + * namespaces are not configurable). + * + * So these assert the STRUCTURAL property that made the bug possible: the + * handlers must be retrievable per-instance references, because an inline + * anonymous arrow can never be passed to `off()` at all. That is necessary + * but not sufficient — it would not catch a `destroy()` that simply forgot + * to call `off`. A listener-count assertion would be strictly better and + * needs a test-visible way to inspect the bus. + */ +describe("Application lifecycle: renderer event handlers are unregisterable", () => { + let hasWebGL; + + beforeAll(async () => { + await boot(); + const probe = new Application(32, 32, { + parent: "screen", + renderer: video.WEBGL, + failIfMajorPerformanceCaveat: false, + consoleHeader: false, + }); + await probe.init(); + hasWebGL = probe.renderer instanceof WebGLRenderer; + probe.destroy(); + }); + + const mk = async (renderer) => { + const app = new Application(64, 64, { + parent: "screen", + renderer, + failIfMajorPerformanceCaveat: false, + consoleHeader: false, + }); + await app.init(); + return app; + }; + + it("CanvasRenderer defines destroy() and a retrievable GAME_RESET handler", async () => { + const app = await mk(video.CANVAS); + expect(app.renderer).toBeInstanceOf(CanvasRenderer); + // CanvasRenderer had no destroy() at all — it inherited the base + // no-op, so its GAME_RESET subscription outlived every application + expect( + Object.hasOwn(CanvasRenderer.prototype, "destroy"), + "CanvasRenderer must define its own destroy()", + ).toBe(true); + expect(typeof app.renderer.onGameReset).toBe("function"); + app.destroy(); + }); + + it("WebGLRenderer exposes all three subscriptions as retrievable handlers", async (ctx) => { + if (!hasWebGL) { + ctx.skip("WebGL renderer not available in this environment"); + } + const app = await mk(video.WEBGL); + const r = app.renderer; + // inline arrows here would be unremovable; these must be fields + expect(typeof r.onGameReset).toBe("function"); + expect(typeof r.onContextRestoredInvalidate).toBe("function"); + expect(typeof r.onCanvasResize).toBe("function"); + app.destroy(); + }); + + it("handlers are per-instance, so each teardown removes its own", async (ctx) => { + if (!hasWebGL) { + ctx.skip("WebGL renderer not available in this environment"); + } + // Guards the drift case: were the handler shared on the prototype, + // cycle N would unregister cycle 0's closure and leave every later + // renderer on the bus. + const seen = new Set(); + for (let i = 0; i < 6; i++) { + const app = await mk(video.WEBGL); + const handler = app.renderer.onGameReset; + expect(seen.has(handler), `cycle ${i} reused a previous handler`).toBe( + false, + ); + seen.add(handler); + app.destroy(); + } + expect(seen.size).toBe(6); + }); +}); From 42d895ec3aca6a96e66ddf34f115a62cc2cda30e Mon Sep 17 00:00:00 2001 From: Olivier Biot Date: Sun, 9 Aug 2026 13:03:02 +0800 Subject: [PATCH 4/4] World and root Container unregister their event listeners too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same class as the renderer fix in the previous commit, in the scene graph. - Container (root only — the subscription is guarded by `this.root === true`, so it is one per world, not one per node) subscribed to CANVAS_ONRESIZE with an inline arrow. Unremovable, and the closure kept the container and its entire child tree reachable from the event bus. - World subscribed to GAME_RESET (handler + context) and LEVEL_LOADED (inline arrow), and had no destroy() of its own — so a destroyed world kept resetting itself on GAME_RESET and clearing a broadphase nobody read on LEVEL_LOADED, and could never be collected. Both now hold their handlers as fields, and destroy() calls off() before delegating to the container teardown. Container clears the field so a second destroy is a no-op rather than a double off(). Extends tests/application_lifecycle.spec.js with the two cases. Same caveat as the renderer tests, already documented in that file: these assert the structural property (handlers are retrievable, and cleared on teardown), which is necessary but does not prove `off()` was called. Full suite 235 files / 5856 pass, eslint 0 errors, biome clean, tsc clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QVjYzf76AEU3wJk766JAQi --- packages/melonjs/CHANGELOG.md | 2 +- packages/melonjs/src/physics/world.js | 25 ++++++++++++++++-- packages/melonjs/src/renderable/container.js | 21 ++++++++++++--- .../tests/application_lifecycle.spec.js | 26 +++++++++++++++++++ 4 files changed, 68 insertions(+), 6 deletions(-) diff --git a/packages/melonjs/CHANGELOG.md b/packages/melonjs/CHANGELOG.md index bf8b15e21..1fe90edb4 100644 --- a/packages/melonjs/CHANGELOG.md +++ b/packages/melonjs/CHANGELOG.md @@ -65,7 +65,7 @@ The old path scales linearly with vertex count; the new one is flat, because no - **the cost of `antiAlias: true` under post effects, quantified** ([#1556](https://github.com/melonjs/melonJS/issues/1556)) — MSAA composing through effect chains (see *Added*) is paid for in memory and bandwidth, and the price is worth knowing. Arithmetic, not measurement: a 4× capture target keeps 4 color + 4 depth-stencil samples per pixel next to its 1× resolve texture — roughly **28 extra bytes per pixel on WebGL (~55 MB of GPU memory at 1080p)** and **~16 bytes per pixel on WebGPU (~32 MB at 1080p)**, where the multisampled depth attachment is shared with the canvas rather than per-target; both scale linearly with resolution. Per frame it adds one resolve blit per effect bracket, and draws inside the bracket write up to 4 samples per covered pixel — bandwidth, not shading cost, since fragment shaders still run once per pixel under MSAA. Only scene **capture** targets pay any of this (ping-pong intermediates stay 1×), and with `antiAlias: false` — the default — no multisampled storage exists at all, so nothing changes ### Fixed -- **destroyed renderers stayed subscribed to global events forever** — `WebGLRenderer` subscribed to `GAME_RESET`, `ONCONTEXT_RESTORED` and `CANVAS_ONRESIZE`, and `CanvasRenderer` to `GAME_RESET`, all as **inline anonymous handlers** — which cannot be passed to `off()`, so nothing could ever unregister them. `CanvasRenderer` had no `destroy()` at all, inheriting the base no-op. Two consequences, both silent: a destroyed renderer kept reacting to those events, and — worse — each handler closes over the renderer, so the subscription pinned the renderer, its batchers and its GPU objects against garbage collection. Releasing the GL context alone did not help, because the JS graph was still reachable from the event bus. The handlers are now per-instance fields and `destroy()` unregisters all of them, matching what the WebGPU backend already did. Any application that tears down and rebuilds — an SPA moving between scenes, a level reload — stops accumulating them +- **destroyed renderers stayed subscribed to global events forever** — `WebGLRenderer` subscribed to `GAME_RESET`, `ONCONTEXT_RESTORED` and `CANVAS_ONRESIZE`, and `CanvasRenderer` to `GAME_RESET`, all as **inline anonymous handlers** — which cannot be passed to `off()`, so nothing could ever unregister them. `CanvasRenderer` had no `destroy()` at all, inheriting the base no-op. Two consequences, both silent: a destroyed renderer kept reacting to those events, and — worse — each handler closes over the renderer, so the subscription pinned the renderer, its batchers and its GPU objects against garbage collection. Releasing the GL context alone did not help, because the JS graph was still reachable from the event bus. The same shape was in the scene graph: the **root `Container`** subscribed to `CANVAS_ONRESIZE` with an inline arrow, and **`World`** to `GAME_RESET` (with a context) and `LEVEL_LOADED` (inline), none of them ever removed — and `World` had no `destroy()` of its own, so a torn-down world kept resetting itself and clearing a broadphase nobody read. All of these are now per-instance fields and `destroy()` unregisters them, matching what the WebGPU backend already did. Any application that tears down and rebuilds — an SPA moving between scenes, a level reload — stops accumulating them - **`Application.destroy()` leaked the WebGL context** — teardown deleted every GL object the renderer owned and removed the canvas from the DOM, but never handed back the **context** itself. A canvas keeps its context until the canvas is garbage-collected, which is non-deterministic and routinely delayed, so each destroyed application left a live context behind. Browsers cap how many they keep — around 16 on Chromium — and force-lose the oldest past that, which means a long-lived page that builds and tears down several applications accumulates dead-but-unfreed contexts until an unrelated later `getContext` stalls or comes back already lost. That hits any single-page app that moves between scenes or unmounts a game view (the examples gallery does exactly this on every navigation), and it was also making unrelated test suites time out in CI. `destroy()` now releases the context through `WEBGL_lose_context`. It stays idempotent, and since `destroy()` is already terminal — `Application.init()` refuses to run again afterwards — losing the context forecloses nothing that was previously possible. Renderers whose driver does not expose the extension are unaffected - **the 3D broadphase silently dropped collisions between bodies at different depths** — under `Camera3d` the world's broadphase is an `Octree`, and `retrieve()` — the candidate feed for SAT collision, pointer picking, the 2D raycast and `adapter.queryAABB` — descended only into the octant the query item itself classified into. But every one of those consumers decides overlap in the **XY plane**: two bodies at different z that overlap in XY genuinely collide, and were never offered to each other as candidates. Whether a given pair was tested came down to which side of an octant boundary each happened to fall on. Measured on a randomized 300-body scene, **12 of 20 genuinely overlapping pairs were never surfaced**. `retrieve()` is now depth-blind: it classifies on x/y only and walks both depth halves of that quadrant, so x/y pruning still applies at every level and in both halves (an item lying wholly inside a different x/y quadrant cannot overlap, and midpoint-straddling items already live at the parent level). On a 600-body scene this costs nothing at all when the bodies share a gameplay plane — the candidate count is unchanged — and on a depth-spread scene it settles at the same candidate count as the flat one, which is the point: with depth no longer part of the decision, the candidate set depends only on the x/y distribution. The genuinely 3D queries are unaffected and still prune on depth: `queryAABB`, `querySphere`, `queryRay` and `queryFrustum` have their own entry points, and each is now pinned by a differential test against a brute-force scan. Note this removes the incidental "parallax at a distant z drops out of collision for free" behaviour that the 2.5D documentation described as best-effort — it was this defect seen from its good side. Exclude parallax deliberately instead, with `isKinematic = true` or `collisionType` / `collisionMask`, which is what the 2D path has always done - **an entire 2.5D gameplay plane sat unpartitioned at the root of the octree** — `getIndex` returned −1 (meaning "straddles a midpoint, keep at this level") for an item sitting *exactly* on one. On x and y that is at least defensible, since an item there may genuinely span the boundary; on z it never is, because items are point-z in the broadphase and a point cannot straddle anything. It mattered because the root box is origin-centred, so its midpoints are `(0, 0, 0)` — the default `pos` of every renderable, and the shared gameplay z that the 2.5D recipe prescribes. Measured: 200 bodies on a `z = 0` plane all stayed at the root and `retrieve()` returned **200 of 200**, degrading the broadphase to a linear scan for exactly the layer holding the most bodies; the same 200 spread across z left only 10 at the root. Classification is now exact on all three axes — a midpoint belongs to the far/right/bottom child, and an item whose far edge merely touches one still counts as wholly inside the near side. Genuine straddlers and out-of-bounds items still stay at the parent, both under regression test. This is invisible to 2D games, which use a `QuadTree` and never construct an `Octree` diff --git a/packages/melonjs/src/physics/world.js b/packages/melonjs/src/physics/world.js index 8df6ba713..9327a54d3 100644 --- a/packages/melonjs/src/physics/world.js +++ b/packages/melonjs/src/physics/world.js @@ -4,6 +4,7 @@ import { emit, GAME_RESET, LEVEL_LOADED, + off, on, WORLD_STEP, } from "../system/event.ts"; @@ -160,13 +161,33 @@ export default class World extends Container { // clears contents because the Octree root is a fixed // origin-centred box that doesn't depend on the level's 2D // extent. - on(LEVEL_LOADED, () => { + // Held as a field, not an inline arrow, so `destroy()` can pass it to + // `off()` — an anonymous handler is unremovable, and its closure keeps + // this world (and its whole child tree and broadphase) reachable from + // the event bus for the life of the page. + this.onLevelLoaded = () => { if (this._sortOn === "depth") { this.broadphase.clear(); } else { this.broadphase.clear(this.getBounds().clone()); } - }); + }; + on(LEVEL_LOADED, this.onLevelLoaded); + } + + /** + * Release this world's global event subscriptions before handing off to + * the container teardown. Without this a destroyed world stays on the + * bus: it keeps resetting itself on `GAME_RESET` and clearing a + * broadphase nobody reads on `LEVEL_LOADED`, and it cannot be garbage + * collected because both handlers close over it. + * @ignore + */ + destroy() { + off(GAME_RESET, this.reset, this); + off(LEVEL_LOADED, this.onLevelLoaded); + this.onLevelLoaded = undefined; + super.destroy(...arguments); } /** diff --git a/packages/melonjs/src/renderable/container.js b/packages/melonjs/src/renderable/container.js index f2a347a9e..52f65ff6a 100644 --- a/packages/melonjs/src/renderable/container.js +++ b/packages/melonjs/src/renderable/container.js @@ -1,7 +1,7 @@ import { colorPool } from "../math/color.ts"; import Body from "../physics/builtin/body.js"; import state from "../state/state.ts"; -import { CANVAS_ONRESIZE, on } from "../system/event.ts"; +import { CANVAS_ONRESIZE, off, on } from "../system/event.ts"; import pool from "../system/legacy_pool.js"; import { defer } from "../utils/function"; import { createGUID } from "../utils/utils"; @@ -244,14 +244,19 @@ export default class Container extends Renderable { // subscribe on the canvas resize event if (this.root === true) { + // Held as a field, not an inline arrow, so `destroy()` can pass it + // to `off()` — an anonymous handler is unremovable, and its closure + // would keep this container (and its whole child tree) reachable + // from the event bus for the life of the page. // Workaround for not updating container child-bounds automatically (it's expensive!) - on(CANVAS_ONRESIZE, () => { + this.onCanvasResize = () => { // temporarly enable the enableChildBoundsUpdate flag // this.enableChildBoundsUpdate === true; // update bounds this.updateBounds(); // this.enableChildBoundsUpdate === false; - }); + }; + on(CANVAS_ONRESIZE, this.onCanvasResize); } } @@ -1098,6 +1103,16 @@ export default class Container extends Renderable { * @ignore */ destroy() { + // drop the root container's resize subscription before anything else — + // the handler closes over `this`, so leaving it registered keeps the + // container and its entire child tree reachable from the event bus, + // and a destroyed container would still try to update its bounds on + // every canvas resize. Only root containers ever subscribe. + if (this.onCanvasResize) { + off(CANVAS_ONRESIZE, this.onCanvasResize); + this.onCanvasResize = undefined; + } + // empty the container this.reset(); // call the parent destroy method, spreading the actual arguments — diff --git a/packages/melonjs/tests/application_lifecycle.spec.js b/packages/melonjs/tests/application_lifecycle.spec.js index 72651dd8d..8810d70e0 100644 --- a/packages/melonjs/tests/application_lifecycle.spec.js +++ b/packages/melonjs/tests/application_lifecycle.spec.js @@ -95,6 +95,32 @@ describe("Application lifecycle: renderer event handlers are unregisterable", () app.destroy(); }); + it("the root Container's CANVAS_ONRESIZE handler is retrievable and cleared", async () => { + const app = await mk(video.CANVAS); + const world = app.world; + // only ROOT containers subscribe (`if (this.root === true)`), so this + // is one per world rather than one per node in the scene graph + expect(world.root).toBe(true); + expect(typeof world.onCanvasResize).toBe("function"); + + app.destroy(); + // cleared on teardown, which is also what makes a second destroy a + // no-op rather than a double `off` + expect(world.onCanvasResize).toBeUndefined(); + }); + + it("World unregisters both of its subscriptions on destroy", async () => { + const app = await mk(video.CANVAS); + const world = app.world; + // GAME_RESET is registered with (handler, context) and LEVEL_LOADED + // was an inline arrow — the latter was unremovable + expect(typeof world.onLevelLoaded).toBe("function"); + expect(typeof world.reset).toBe("function"); + + app.destroy(); + expect(world.onLevelLoaded).toBeUndefined(); + }); + it("handlers are per-instance, so each teardown removes its own", async (ctx) => { if (!hasWebGL) { ctx.skip("WebGL renderer not available in this environment");