Skip to content

Commit b7a0836

Browse files
obiotclaude
andcommitted
fix: PR #1464 review round 4 — three more bugs
**Camera3d.isVisible bounding-sphere radius was inradius, not circumradius** The frustum-sphere test used `max(width, height) * 0.5` for the renderable's culling radius, which is the inradius of the bounds rect — smaller than the distance from center to a corner. Sprites near a frustum edge could be marked invisible while a corner was still on-screen. Switched to `√(w² + h²) / 2` (the circumradius); the sphere now always encloses every corner of the bounds rect. **Stage.reset read `settings.cameras[0]` for sortOn, not the camera registered as "default"** A split-screen / minimap stage can list a non-default Camera2d BEFORE its main Camera3d in the cameras array (e.g. `new Stage({ cameras: [minimap2d, main3d] })`), and the previous code would pick the minimap's Camera2d class — leaving `world.sortOn = "z"` while the perspective view needed "depth". Switched to `this.cameras.get("default")?.constructor`, gated on `settings.cameras.length > 0` so the singleton fallback case (no class declared anywhere) still leaves `world.sortOn` untouched. Regression test in `camera3d_integration.spec.js`. **MaterialBatcher GPU_TEXTURE_CACHE_RESET subscription leaked across renderer disposal** `MaterialBatcher.destroy()` unsubscribed from `event.on(...)` but nothing called it — neither `WebGLRenderer` nor `Application` walked the batchers on teardown. Every discarded renderer kept its batchers (and itself) alive via the listener, and future cache-reset events fired into dead renderers. Added `WebGLRenderer.destroy()` that walks `this.batchers` and calls each one's `destroy()`. `Application.destroy()` now invokes `this.renderer?.destroy?.()` so the chain is wired end-to-end. 3768 tests pass (+1 new regression test). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 2669c13 commit b7a0836

5 files changed

Lines changed: 72 additions & 3 deletions

File tree

packages/melonjs/src/application/application.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -649,6 +649,14 @@ export default class Application {
649649
this.world.destroy();
650650
}
651651

652+
// tear down the renderer's batchers so their `event.on(...)`
653+
// subscriptions (e.g. `GPU_TEXTURE_CACHE_RESET` on
654+
// `MaterialBatcher`) don't leak across the disposal — every
655+
// otherwise-discarded `Application` would otherwise keep the
656+
// batchers (and their renderer reference) alive for the rest
657+
// of the page's lifetime.
658+
this.renderer?.destroy?.();
659+
652660
// remove the canvas from the DOM
653661
if (removeCanvas && this.renderer) {
654662
const canvas = this.renderer.getCanvas();

packages/melonjs/src/camera/camera3d.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -564,7 +564,14 @@ export default class Camera3d extends Camera2d {
564564
// `pos.z`) here, which silently mis-culled children of any
565565
// container whose own depth was non-zero.
566566
const bounds = obj.getBounds();
567-
const radius = Math.max(bounds.width, bounds.height) * 0.5;
567+
// Half-diagonal — the conservative bounding-sphere radius for
568+
// a rectangular bounds rect. `max(w, h) * 0.5` is the
569+
// inradius and can mark a renderable invisible while one of
570+
// its corners is still on-screen near a frustum edge; the
571+
// circumradius √(w² + h²) / 2 always encloses every corner.
572+
const radius =
573+
Math.sqrt(bounds.width * bounds.width + bounds.height * bounds.height) *
574+
0.5;
568575
const absPos = obj.getAbsolutePosition();
569576
return this.frustum.intersectsSphere(absPos.x, absPos.y, absPos.z, radius);
570577
}

packages/melonjs/src/state/stage.ts

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -209,8 +209,22 @@ export default class Stage {
209209
} else if (typeof AppCameraClass === "function") {
210210
chosenClass = AppCameraClass as unknown as SortAwareCameraClass;
211211
} else if (this.settings.cameras.length > 0) {
212-
chosenClass = this.settings.cameras[0]
213-
.constructor as unknown as SortAwareCameraClass;
212+
// Read the camera actually registered under the "default"
213+
// key, NOT `settings.cameras[0]`. A split-screen / minimap
214+
// stage can list a non-default Camera2d before its main
215+
// Camera3d in the cameras array, and `[0].constructor`
216+
// would pick the wrong class — leaving `world.sortOn` at
217+
// "z" while the main perspective view needed "depth".
218+
//
219+
// Gated on `settings.cameras.length > 0` so the
220+
// shared-Camera2d-singleton fallback (no cameraClass +
221+
// no explicit cameras anywhere) leaves `world.sortOn`
222+
// untouched, preserving pre-19.7 behavior.
223+
const defaultCam = this.cameras.get("default");
224+
if (defaultCam) {
225+
chosenClass =
226+
defaultCam.constructor as unknown as SortAwareCameraClass;
227+
}
214228
}
215229
const defaultSortOn = chosenClass?.defaultSortOn;
216230
if (defaultSortOn && app.world.sortOn !== defaultSortOn) {

packages/melonjs/src/video/webgl/webgl_renderer.js

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -362,6 +362,22 @@ export default class WebGLRenderer extends Renderer {
362362
/**
363363
* Reset context state
364364
*/
365+
/**
366+
* Tear down this renderer and free GPU/event resources. Walks every
367+
* registered batcher's `destroy()` so cross-renderer subscriptions
368+
* (`GPU_TEXTURE_CACHE_RESET` on `MaterialBatcher`, etc.) don't
369+
* leak across `Application.destroy()` cycles. Safe to call multiple
370+
* times — subsequent calls are no-ops.
371+
*/
372+
destroy() {
373+
if (this.batchers) {
374+
this.batchers.forEach((batcher) => {
375+
batcher.destroy?.();
376+
});
377+
this.batchers.clear();
378+
}
379+
}
380+
365381
reset() {
366382
super.reset();
367383

packages/melonjs/tests/camera3d_integration.spec.js

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -307,6 +307,30 @@ describe("Camera3d × Stage × Application integration", () => {
307307
stage.reset(app);
308308
expect(app.world.sortOn).toBe("depth");
309309
});
310+
311+
// Regression for PR #1464 review round 3: the prior fix read
312+
// `settings.cameras[0].constructor`, which picks the wrong
313+
// class for a split-screen / minimap stage whose array lists
314+
// a non-default Camera2d BEFORE its main Camera3d.
315+
it("explicit `cameras: [...]` reads the camera registered as 'default', not [0]", () => {
316+
const app = new Application(400, 300, {
317+
parent: "screen",
318+
renderer: video.CANVAS,
319+
});
320+
expect(app.world.sortOn).toBe("z");
321+
322+
// Non-default Camera2d minimap listed FIRST, main Camera3d
323+
// listed second but explicitly named "default".
324+
const minimap = new Camera2d(0, 0, 100, 100);
325+
minimap.name = "minimap";
326+
const main = new Camera3d(0, 0, 400, 300);
327+
main.name = "default";
328+
329+
new Stage({ cameras: [minimap, main] }).reset(app);
330+
// Reading `[0].constructor` would give Camera2d → "z";
331+
// reading the "default"-named entry gives Camera3d → "depth".
332+
expect(app.world.sortOn).toBe("depth");
333+
});
310334
});
311335

312336
// --- Adversarial / regression coverage --------------------------------

0 commit comments

Comments
 (0)