From 64e60778470ea898b6bda102add479f2b8a497d8 Mon Sep 17 00:00:00 2001 From: Olivier Biot Date: Tue, 12 May 2026 15:15:54 +0800 Subject: [PATCH 1/2] fix(physics, tilemap): correct ellipse SAT sign and refresh stale bounds on level centering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two long-standing bugs surfaced together once level.load auto-centers the level container on a wider-than-map viewport. SAT: testEllipseEllipse and testPolygonEllipse built the relative-position vector by *adding* a.ancestor.getAbsolutePosition() where they should have subtracted it. With ancestor.absPos at (0, 0) the sign is arithmetically invisible, so every existing SAT unit test (all mocked at zero) passed; with a non-zero ancestor offset the circle was shifted by 2 * ancestor.absPos and ellipse-vs-anything collisions silently missed. The polygon/polygon path is unaffected — it builds two absolute positions and lets isSeparatingAxis do the subtraction. TMX: TMXTileMap.addTo sets container.pos *after* adding children, so each child's cached absolute bounds (computed at addChild time) didn't include the centering offset. Children that moved on their own refreshed via the pos observer, but TMX layers, Tiled collision shapes, triggers, and decorative sprites stayed pinned at their pre-centering bounds — visible as the debug overlay drawing shapes at the wrong screen position, and as broken viewport culling for anything outside the pre-centering box. _setBounds now walks the container subtree and refreshes absolute bounds after the position actually moves (both initial load and viewport resize). Tests: added a 14-case matrix over every SAT shape combination (Polygon×Polygon, Polygon×Ellipse, Ellipse×Polygon, Ellipse×Ellipse, plus circle / true-ellipse variants). Each case asserts both shift-invariance under a common ancestor offset and a near-miss geometry that the buggy +offset would push out of range. Against the reverted sat.js, exactly the 9 bug-affected assertions fail; the 5 polygon-polygonized paths pass — confirming the matrix tracks which SAT paths the sign error actually breaks. Co-Authored-By: Claude Opus 4.7 (1M context) --- packages/melonjs/CHANGELOG.md | 2 + .../melonjs/src/level/tiled/TMXTileMap.js | 37 ++++- packages/melonjs/src/physics/sat.js | 20 ++- packages/melonjs/tests/sat.spec.js | 156 +++++++++++++++++- 4 files changed, 202 insertions(+), 13 deletions(-) diff --git a/packages/melonjs/CHANGELOG.md b/packages/melonjs/CHANGELOG.md index 0757965f3..96a2be5c3 100644 --- a/packages/melonjs/CHANGELOG.md +++ b/packages/melonjs/CHANGELOG.md @@ -11,6 +11,8 @@ ### Fixed - WebGL: `MaterialBatcher.uploadTexture` was using its `w` and `h` parameters (the destination quad size, not the texture's) for the `isPOT` check, which drives both the wrap-mode fallback and the `generateMipmap` gate. Visible as a `GL_INVALID_OPERATION` from `gl.generateMipmap` on WebGL 1; silent wasted work (unnecessary mipmaps, wrong `isPOT`-derived state) on WebGL 2. Texture dimensions are now derived from the source itself. +- SAT: ellipse collisions silently failed whenever the body's ancestor container had a non-zero absolute position (the typical case: `level.load` auto-centers the level container when the viewport is larger than the map, setting `container.pos` to a non-zero offset). `testEllipseEllipse` and `testPolygonEllipse` built the relative-position vector by *adding* `a.ancestor.getAbsolutePosition()` where they should have subtracted it, shifting the circle by `2 * ancestor.absPos`. The polygon/polygon path is unaffected — it builds two absolute positions and lets `isSeparatingAxis` do the subtraction. Latent because every existing SAT unit test wired the mock ancestor to `(0, 0)`, where the sign error is arithmetically invisible. +- TMX: static children of an auto-centered level container kept stale absolute bounds. `TMXTileMap.addTo` sets `container.pos` *after* adding children, so each child's cached absolute bounds (computed at `addChild` time) didn't include the centering offset. Children that moved on their own refreshed via the `pos` observer, but TMX layers, Tiled collision shapes, triggers, and decorative sprites stayed stuck at their pre-centering bounds — visible as debug overlay shapes drawn at the wrong screen position, and as broken viewport culling for anything outside the pre-centering box. `_setBounds` now walks the container subtree and refreshes absolute bounds after the position actually moves (both initial load and viewport resize). ### Changed - WebGL 1: removed the unconditional `[Texture] ... is not a POT texture` warning. The engine handles NPOT correctly (clamp wrap, non-mipmapped filters). A targeted warning now fires only when `repeat: "repeat*"` is requested on an NPOT texture under WebGL 1, the one case where the user's intent is silently downgraded. diff --git a/packages/melonjs/src/level/tiled/TMXTileMap.js b/packages/melonjs/src/level/tiled/TMXTileMap.js index 615cf021c..7677e33a4 100644 --- a/packages/melonjs/src/level/tiled/TMXTileMap.js +++ b/packages/melonjs/src/level/tiled/TMXTileMap.js @@ -115,6 +115,23 @@ function readObjectGroup(map, data, z) { return new TMXGroup(map, data, z); } +/** + * Recursively recompute absolute bounds for a container and all its + * descendants. Used when the container's pos changes (centering on a wider + * viewport): children cache their absolute bounds at addChild time, and + * those caches are stale until each child happens to call updateBounds on + * its own (which only happens organically when its own pos changes). + * @ignore + */ +function refreshAbsoluteBounds(container) { + container.forEach((child) => { + child.updateBounds(true); + if (child instanceof Container) { + refreshAbsoluteBounds(child); + } + }); +} + /** * a TMX Tile Map Object * Tiled QT +0.7.x format @@ -425,12 +442,20 @@ export default class TMXTileMap { Math.max(levelBounds.height, height), ); // center the map if smaller than the current viewport - container.pos.set( - Math.max(0, ~~((width - levelBounds.width) / 2)), - Math.max(0, ~~((height - levelBounds.height) / 2)), - // don't change the container z position if defined - container.pos.z, - ); + const newX = Math.max(0, ~~((width - levelBounds.width) / 2)); + const newY = Math.max(0, ~~((height - levelBounds.height) / 2)); + if (container.pos.x !== newX || container.pos.y !== newY) { + container.pos.set(newX, newY, container.pos.z); + // A child caches its absolute bounds (centered around + // ancestor.absPos at the time of the last `updateBounds`). + // When the parent's pos changes, descendants that don't + // move on their own (TMX layers, static collision + // objects, triggers) keep stale absolute bounds, so + // anything that reads them (debug overlay, viewport + // culling) sees the pre-centering position. Walk the + // tree once after a real move and refresh. + refreshAbsoluteBounds(container); + } } off(VIEWPORT_ONRESIZE, _setBounds); diff --git a/packages/melonjs/src/physics/sat.js b/packages/melonjs/src/physics/sat.js index 13d50bfb1..106b0cd85 100644 --- a/packages/melonjs/src/physics/sat.js +++ b/packages/melonjs/src/physics/sat.js @@ -300,13 +300,19 @@ export function testEllipseEllipse(a, ellipseA, b, ellipseB, response) { // Fast path: both are circles // Check if the distance between the centers of the two - // circles is greater than their combined radius. + // circles is greater than their combined radius. We need the absolute + // position of B relative to A — both sides resolve their world position + // via ancestor.getAbsolutePosition(), so A's offset must be subtracted + // too, not added. (The bug was invisible when ancestor.absPos == 0, e.g. + // the level was the same size as the viewport; it manifests as missed + // circle collisions whenever the container is shifted, such as TMX + // auto-centering on a wider viewport.) const differenceV = T_VECTORS[--T_VECTORS_IDX] .copy(b.pos) .add(b.ancestor.getAbsolutePosition()) .add(ellipseB.pos) .sub(a.pos) - .add(a.ancestor.getAbsolutePosition()) + .sub(a.ancestor.getAbsolutePosition()) .sub(ellipseA.pos); const radiusA = ellipseA.radius; const radiusB = ellipseB.radius; @@ -350,13 +356,19 @@ export function testPolygonEllipse(a, polyA, b, ellipseB, response) { } // Fast path: ellipse is a circle - // Get the position of the circle relative to the polygon. + // Get the position of the circle relative to the polygon. Both A and B + // resolve their world position via ancestor.getAbsolutePosition(), so + // A's ancestor offset must be subtracted (matches the polygon/polygon + // path which builds absolute positions). The previous `+a.ancestor.abs` + // was masked whenever the level container was at the origin, but missed + // every circle-vs-polygon collision once the container was shifted (TMX + // auto-centering on a wider viewport, manually translated containers). const circlePos = T_VECTORS[--T_VECTORS_IDX] .copy(b.pos) .add(b.ancestor.getAbsolutePosition()) .add(ellipseB.pos) .sub(a.pos) - .add(a.ancestor.getAbsolutePosition()) + .sub(a.ancestor.getAbsolutePosition()) .sub(polyA.pos); const radius = ellipseB.radius; const radius2 = radius * radius; diff --git a/packages/melonjs/tests/sat.spec.js b/packages/melonjs/tests/sat.spec.js index 5d71a915b..9e54dd9a3 100644 --- a/packages/melonjs/tests/sat.spec.js +++ b/packages/melonjs/tests/sat.spec.js @@ -12,14 +12,16 @@ import { * Helper to create a mock renderable with position and ancestor * (SAT functions expect objects with .pos and .ancestor.getAbsolutePosition()) */ -function createMockRenderable(x, y) { +function createMockRenderable(x, y, ancestorOffset) { const renderable = new Renderable(x, y, 0, 0); renderable.anchorPoint.set(0, 0); - // ensure ancestor chain returns zero offset + const offset = + ancestorOffset instanceof Vector2d ? ancestorOffset : new Vector2d(0, 0); + // ensure ancestor chain returns the requested offset if (typeof renderable.ancestor === "undefined") { renderable.ancestor = { getAbsolutePosition() { - return new Vector2d(0, 0); + return offset; }, }; } @@ -711,4 +713,152 @@ describe("Physics : SAT (Separating Axis Theorem)", () => { } }); }); + + // Regression: SAT must respect a non-zero ancestor.getAbsolutePosition() + // the same way for both colliders. Both operands resolve their world + // position via ancestor.getAbsolutePosition(), so the relative-position + // vector must subtract A's ancestor offset (the polygon/polygon path + // builds two absolute positions and subtracts inside isSeparatingAxis, + // which is correct; the ellipse paths build the difference directly + // and used to ADD A's ancestor offset by mistake). This was invisible + // whenever ancestor.abs was (0, 0), so a TMX level the same size as + // the viewport hid the bug; it broke every circle collision the moment + // the level container was shifted (`level.load` auto-centering on a + // wider viewport, or any manually translated container). + describe("non-zero ancestor offset", () => { + let response; + beforeEach(() => { + response = new ResponseObject(); + }); + + // Each entry exercises one shape-type combination and asserts that + // (a) overlap detection still fires when both bodies share a + // non-zero ancestor offset, (b) the response overlap matches the + // zero-offset baseline (shift-invariance), and (c) the concrete + // near-miss geometry the platformer hits is detected. + const offset = new Vector2d(255, 0); + const makeCases = () => [ + { + name: "Polygon vs Polygon (Rect/Rect via testPolygonPolygon)", + fn: testPolygonPolygon, + shapeA: () => new Rect(0, 0, 40, 40).toPolygon(), + shapeB: () => new Rect(0, 0, 40, 40).toPolygon(), + // Polygon/polygon was already correct (isSeparatingAxis + // subtracts internally), so this entry guards against a + // future regression rather than reproducing today's bug. + preFixWouldMiss: false, + }, + { + name: "Polygon vs Polygon (custom Polygon/Rect via testPolygonPolygon)", + fn: testPolygonPolygon, + shapeA: () => + new Polygon(0, 0, [ + new Vector2d(0, 0), + new Vector2d(30, 0), + new Vector2d(15, 40), + ]), + shapeB: () => new Rect(0, 0, 40, 40).toPolygon(), + preFixWouldMiss: false, + }, + { + name: "Polygon vs Circle (Rect/Circle via testPolygonEllipse)", + fn: testPolygonEllipse, + shapeA: () => new Rect(0, 0, 40, 40).toPolygon(), + shapeB: () => new Ellipse(0, 0, 40, 40), + preFixWouldMiss: true, + }, + { + name: "Polygon vs true Ellipse (Rect/Ellipse via testPolygonEllipse)", + fn: testPolygonEllipse, + shapeA: () => new Rect(0, 0, 40, 40).toPolygon(), + // non-square radii → falls through to testPolygonPolygon + // internally, so this exercises the conversion path too. + shapeB: () => new Ellipse(0, 0, 40, 24), + preFixWouldMiss: false, + }, + { + name: "Circle vs Polygon (Circle/Rect via testEllipsePolygon)", + fn: testEllipsePolygon, + shapeA: () => new Ellipse(0, 0, 40, 40), + shapeB: () => new Rect(0, 0, 40, 40).toPolygon(), + preFixWouldMiss: true, + }, + { + name: "true Ellipse vs Polygon (Ellipse/Rect via testEllipsePolygon)", + fn: testEllipsePolygon, + shapeA: () => new Ellipse(0, 0, 40, 24), + shapeB: () => new Rect(0, 0, 40, 40).toPolygon(), + preFixWouldMiss: false, + }, + { + name: "Circle vs Circle (testEllipseEllipse fast path)", + fn: testEllipseEllipse, + shapeA: () => new Ellipse(0, 0, 40, 40), // radius 20 + shapeB: () => new Ellipse(0, 0, 40, 40), + preFixWouldMiss: true, + }, + { + name: "Circle vs true Ellipse (testEllipseEllipse mixed)", + fn: testEllipseEllipse, + shapeA: () => new Ellipse(0, 0, 40, 40), + shapeB: () => new Ellipse(0, 0, 40, 24), + preFixWouldMiss: true, + }, + { + name: "true Ellipse vs true Ellipse (testEllipseEllipse polygonized)", + fn: testEllipseEllipse, + shapeA: () => new Ellipse(0, 0, 40, 24), + shapeB: () => new Ellipse(0, 0, 40, 24), + preFixWouldMiss: false, + }, + ]; + + for (const c of makeCases()) { + it(`${c.name}: shift-invariant under a common ancestor offset`, () => { + const aRef = createMockRenderable(0, 0); + const bRef = createMockRenderable(15, 0); + const refResp = new ResponseObject(); + const refHit = c.fn(aRef, c.shapeA(), bRef, c.shapeB(), refResp); + expect(refHit).toBe(true); + + const aShifted = createMockRenderable(0, 0, offset); + const bShifted = createMockRenderable(15, 0, offset); + const shiftedHit = c.fn( + aShifted, + c.shapeA(), + bShifted, + c.shapeB(), + response, + ); + expect(shiftedHit).toBe(true); + expect(response.overlap).toBeCloseTo(refResp.overlap); + }); + + if (c.preFixWouldMiss) { + it(`${c.name}: pre-fix sign error mis-placed the circle by 2 * ancestor.abs`, () => { + // Geometry chosen so that the bodies overlap normally, + // but the buggy +offset would push them ~510 px apart + // (far beyond any reasonable shape extent). + const a = createMockRenderable(0, 0, offset); + const b = createMockRenderable(10, 0, offset); + expect(c.fn(a, c.shapeA(), b, c.shapeB(), response)).toBe(true); + }); + } + } + + // Concrete repro of the platformer's player-polygon vs coin-circle + // collision at world.pos (255, 0). Without the SAT fix the circle's + // computed position is +510 px from where it should be, missing + // the polygon entirely. + it("platformer regression: player Rect vs coin Circle inside a shifted level container", () => { + const offset = new Vector2d(255, 0); + const player = createMockRenderable(785, 790, offset); + const coin = createMockRenderable(840, 805, offset); + const playerPoly = new Rect(20, 8, 35, 88).toPolygon(); + const coinCircle = new Ellipse(17.5, 17.5, 35, 35); // radius 17.5 + expect( + testPolygonEllipse(player, playerPoly, coin, coinCircle, response), + ).toBe(true); + }); + }); }); From 669b52951d781f7d1d514dcf8c03c5832c557ee5 Mon Sep 17 00:00:00 2001 From: Olivier Biot Date: Tue, 12 May 2026 15:18:54 +0800 Subject: [PATCH 2/2] fix(test): wrap arrow bodies to satisfy arrow-body-style: always Co-Authored-By: Claude Opus 4.7 (1M context) --- packages/melonjs/tests/sat.spec.js | 187 +++++++++++++++++------------ 1 file changed, 112 insertions(+), 75 deletions(-) diff --git a/packages/melonjs/tests/sat.spec.js b/packages/melonjs/tests/sat.spec.js index 9e54dd9a3..6f6a5f233 100644 --- a/packages/melonjs/tests/sat.spec.js +++ b/packages/melonjs/tests/sat.spec.js @@ -737,81 +737,118 @@ describe("Physics : SAT (Separating Axis Theorem)", () => { // zero-offset baseline (shift-invariance), and (c) the concrete // near-miss geometry the platformer hits is detected. const offset = new Vector2d(255, 0); - const makeCases = () => [ - { - name: "Polygon vs Polygon (Rect/Rect via testPolygonPolygon)", - fn: testPolygonPolygon, - shapeA: () => new Rect(0, 0, 40, 40).toPolygon(), - shapeB: () => new Rect(0, 0, 40, 40).toPolygon(), - // Polygon/polygon was already correct (isSeparatingAxis - // subtracts internally), so this entry guards against a - // future regression rather than reproducing today's bug. - preFixWouldMiss: false, - }, - { - name: "Polygon vs Polygon (custom Polygon/Rect via testPolygonPolygon)", - fn: testPolygonPolygon, - shapeA: () => - new Polygon(0, 0, [ - new Vector2d(0, 0), - new Vector2d(30, 0), - new Vector2d(15, 40), - ]), - shapeB: () => new Rect(0, 0, 40, 40).toPolygon(), - preFixWouldMiss: false, - }, - { - name: "Polygon vs Circle (Rect/Circle via testPolygonEllipse)", - fn: testPolygonEllipse, - shapeA: () => new Rect(0, 0, 40, 40).toPolygon(), - shapeB: () => new Ellipse(0, 0, 40, 40), - preFixWouldMiss: true, - }, - { - name: "Polygon vs true Ellipse (Rect/Ellipse via testPolygonEllipse)", - fn: testPolygonEllipse, - shapeA: () => new Rect(0, 0, 40, 40).toPolygon(), - // non-square radii → falls through to testPolygonPolygon - // internally, so this exercises the conversion path too. - shapeB: () => new Ellipse(0, 0, 40, 24), - preFixWouldMiss: false, - }, - { - name: "Circle vs Polygon (Circle/Rect via testEllipsePolygon)", - fn: testEllipsePolygon, - shapeA: () => new Ellipse(0, 0, 40, 40), - shapeB: () => new Rect(0, 0, 40, 40).toPolygon(), - preFixWouldMiss: true, - }, - { - name: "true Ellipse vs Polygon (Ellipse/Rect via testEllipsePolygon)", - fn: testEllipsePolygon, - shapeA: () => new Ellipse(0, 0, 40, 24), - shapeB: () => new Rect(0, 0, 40, 40).toPolygon(), - preFixWouldMiss: false, - }, - { - name: "Circle vs Circle (testEllipseEllipse fast path)", - fn: testEllipseEllipse, - shapeA: () => new Ellipse(0, 0, 40, 40), // radius 20 - shapeB: () => new Ellipse(0, 0, 40, 40), - preFixWouldMiss: true, - }, - { - name: "Circle vs true Ellipse (testEllipseEllipse mixed)", - fn: testEllipseEllipse, - shapeA: () => new Ellipse(0, 0, 40, 40), - shapeB: () => new Ellipse(0, 0, 40, 24), - preFixWouldMiss: true, - }, - { - name: "true Ellipse vs true Ellipse (testEllipseEllipse polygonized)", - fn: testEllipseEllipse, - shapeA: () => new Ellipse(0, 0, 40, 24), - shapeB: () => new Ellipse(0, 0, 40, 24), - preFixWouldMiss: false, - }, - ]; + const makeCases = () => { + return [ + { + name: "Polygon vs Polygon (Rect/Rect via testPolygonPolygon)", + fn: testPolygonPolygon, + shapeA: () => { + return new Rect(0, 0, 40, 40).toPolygon(); + }, + shapeB: () => { + return new Rect(0, 0, 40, 40).toPolygon(); + }, + // Polygon/polygon was already correct (isSeparatingAxis + // subtracts internally), so this entry guards against a + // future regression rather than reproducing today's bug. + preFixWouldMiss: false, + }, + { + name: "Polygon vs Polygon (custom Polygon/Rect via testPolygonPolygon)", + fn: testPolygonPolygon, + shapeA: () => { + return new Polygon(0, 0, [ + new Vector2d(0, 0), + new Vector2d(30, 0), + new Vector2d(15, 40), + ]); + }, + shapeB: () => { + return new Rect(0, 0, 40, 40).toPolygon(); + }, + preFixWouldMiss: false, + }, + { + name: "Polygon vs Circle (Rect/Circle via testPolygonEllipse)", + fn: testPolygonEllipse, + shapeA: () => { + return new Rect(0, 0, 40, 40).toPolygon(); + }, + shapeB: () => { + return new Ellipse(0, 0, 40, 40); + }, + preFixWouldMiss: true, + }, + { + name: "Polygon vs true Ellipse (Rect/Ellipse via testPolygonEllipse)", + fn: testPolygonEllipse, + shapeA: () => { + return new Rect(0, 0, 40, 40).toPolygon(); + }, + // non-square radii → falls through to testPolygonPolygon + // internally, so this exercises the conversion path too. + shapeB: () => { + return new Ellipse(0, 0, 40, 24); + }, + preFixWouldMiss: false, + }, + { + name: "Circle vs Polygon (Circle/Rect via testEllipsePolygon)", + fn: testEllipsePolygon, + shapeA: () => { + return new Ellipse(0, 0, 40, 40); + }, + shapeB: () => { + return new Rect(0, 0, 40, 40).toPolygon(); + }, + preFixWouldMiss: true, + }, + { + name: "true Ellipse vs Polygon (Ellipse/Rect via testEllipsePolygon)", + fn: testEllipsePolygon, + shapeA: () => { + return new Ellipse(0, 0, 40, 24); + }, + shapeB: () => { + return new Rect(0, 0, 40, 40).toPolygon(); + }, + preFixWouldMiss: false, + }, + { + name: "Circle vs Circle (testEllipseEllipse fast path)", + fn: testEllipseEllipse, + shapeA: () => { + return new Ellipse(0, 0, 40, 40); + }, // radius 20 + shapeB: () => { + return new Ellipse(0, 0, 40, 40); + }, + preFixWouldMiss: true, + }, + { + name: "Circle vs true Ellipse (testEllipseEllipse mixed)", + fn: testEllipseEllipse, + shapeA: () => { + return new Ellipse(0, 0, 40, 40); + }, + shapeB: () => { + return new Ellipse(0, 0, 40, 24); + }, + preFixWouldMiss: true, + }, + { + name: "true Ellipse vs true Ellipse (testEllipseEllipse polygonized)", + fn: testEllipseEllipse, + shapeA: () => { + return new Ellipse(0, 0, 40, 24); + }, + shapeB: () => { + return new Ellipse(0, 0, 40, 24); + }, + preFixWouldMiss: false, + }, + ]; + }; for (const c of makeCases()) { it(`${c.name}: shift-invariant under a common ancestor offset`, () => {