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..6f6a5f233 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,189 @@ 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 = () => { + 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`, () => { + 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); + }); + }); });