fix(physics, tilemap): ellipse SAT sign + stale bounds on centered level#1446
Merged
Conversation
…nds on level centering 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) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes two offset-related bugs that appear when a level/container is translated (notably TMX auto-centering when the viewport is larger than the map): SAT ellipse collision tests were computing relative positions with the wrong sign, and TMX children could keep stale absolute bounds after the parent container position changed.
Changes:
- Physics/SAT: correct relative-position computation in
testEllipseEllipseandtestPolygonEllipseby subtractinga.ancestor.getAbsolutePosition()instead of adding it. - TMX: after auto-centering the level container, recursively refresh descendants’ absolute bounds so debug overlay + viewport culling use correct world-space bounds.
- Tests/Changelog: add regression tests covering non-zero ancestor offsets across SAT shape combinations and document both fixes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/melonjs/tests/sat.spec.js | Adds regression coverage ensuring SAT collision results are invariant under a shared non-zero ancestor offset (and reproduces the historical sign bug). |
| packages/melonjs/src/physics/sat.js | Fixes the sign error in ellipse-related SAT relative-position vectors when ancestors have non-zero absolute position. |
| packages/melonjs/src/level/tiled/TMXTileMap.js | Refreshes cached absolute bounds for TMX-added children after the level container is re-positioned by auto-centering. |
| packages/melonjs/CHANGELOG.md | Documents the SAT ancestor-offset collision fix and the TMX stale-bounds-on-centering fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This was referenced May 12, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two long-standing bugs that surfaced together once a level is auto-centered (
level.loadon a viewport wider/taller than the map).SAT —
physics/sat.jstestEllipseEllipseandtestPolygonEllipsebuilt the relative-position vector by addinga.ancestor.getAbsolutePosition()where they should have subtracted it. Both operands resolve their world position viaancestor.getAbsolutePosition(), so the relative vector needs both terms with opposing signs.ancestor.absPos == (0, 0)the wrong-sign term is zero and the bug is arithmetically invisible — every existing SAT unit test (all mocked at zero) passed.2 * ancestor.absPosand ellipse-vs-anything collisions silently miss. In the platformer at a 2400×600 viewport (255 px centering), every coin / circular collider was getting "telepoted" 510 px to the right.isSeparatingAxisdo the subtraction internally.TMX —
level/tiled/TMXTileMap.jsTMXTileMap.addTosetscontainer.posafter adding children, so each child's cached absolute bounds (computed ataddChildtime) didn't include the centering offset. Children that moved on their own refreshed via theposobserver, but TMX layers, Tiled collision shapes, triggers, and decorative sprites stayed pinned at their pre-centering bounds.inViewportculling for anything outside the pre-centering box._setBoundsnow walks the container subtree and refreshes absolute bounds — only when the position actually moved, so the no-op case (viewport ≤ map) skips the walk.Tests —
tests/sat.spec.js14-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 the buggy
+offsetwould push out of range. Against the revertedsat.js, exactly the 9 bug-affected assertions fail; the 5 polygon-polygonized paths (true-ellipse cases that flow throughtestPolygonPolygon) pass — confirming the matrix tracks which SAT paths the sign error actually breaks.Test plan
pnpm vitest run tests/sat.spec.js— 61 / 61 passsat.js, re-ran tests — 9 / 14 new ancestor-offset assertions fail, rest pass (proves the new tests bite)🤖 Generated with Claude Code