fix: Path2D arc continuity, Rect geometry, loader asset hygiene (audit mediums)#1540
Merged
Merged
Conversation
…metry, loader hygiene Path2D (geometries/path2d.ts): arc()/arcTo()/ellipse() appended to an open path called moveTo() unconditionally, silently starting a new sub-path — which the 19.9 multi-sub-path fill semantics treat as a HOLE punched in the shape. Per the native Path2D spec they now CONNECT to the current point with a straight line (arcTo's own JSDoc promised it); a new penMoved flag distinguishes a virgin path (arc starts fresh) from an explicit moveTo (native connecting-line behavior), so the canonical circle-hole idiom (moveTo before arc) still registers a hole and roundRect stays a single sub-path. The SVG `A` parser had been working around this internally. One #1253 adversarial test codified the old auto-detach accident — updated to assert native semantics plus the moveTo hole idiom. Rect (geometries/rectangle.ts): - right/bottom computed `this.left + w || w` — an edge exactly at coordinate 0 is falsy and returned the size instead - toPolygon() passed the rect's live Vector2d array into the pooled polygon (setVertices stores Vector2d[] by reference) — transforming the polygon corrupted the rectangle; vertices are now cloned Loader (loader/loader.js): - load() wrote the resolved URL (fontface url() unwrap + baseURL prefix) back into asset.src, so loader.reload() — or load()ing the same manifest object twice — fetched base+base+src. The URL is now resolved into a copy handed to the parser; caches, events and failure tracking keep the caller's original src - unload() of a never-loaded fontface threw (FontFaceSet.delete is WebIDL-typed; undefined is a TypeError, not false) — now guarded like every other asset type Specs written failing-first: 8 red on the old code across path2d-arc-connect / rect-geometry / loader-asset-src, all green after. Full suite 4609 passed / 0 failed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes several correctness issues uncovered during a “bug-hunt” audit across Path2D arc continuity, Rect geometry utilities, and loader asset lifecycle behavior, and adds/updates tests to lock in the corrected semantics (notably aligning Path2D arc/arcTo/ellipse behavior with native Path2D continuity rules).
Changes:
- Fix Path2D
arc(),arcTo(), andellipse()to connect to the current point (via a newconnectTohelper +penMovedtracking) instead of incorrectly starting new sub-paths. - Fix
Rect.right/Rect.bottomorigin-edge correctness and ensureRect.toPolygon()returns an independent polygon (no shared live vertex array). - Prevent
loader.load()from mutating caller asset descriptors (avoids double-prependingbaseURLon retries) and makeloader.unload()for never-loadedfontfacereturnfalseinstead of throwing.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/melonjs/src/geometries/path2d.ts | Implements native-style arc/ellipse/arcTo continuity via connectTo + penMoved, preventing unintended hole sub-paths. |
| packages/melonjs/tests/path2d.spec.js | Updates an adversarial test to assert native arc connection semantics and adds a companion hole-idiom test. |
| packages/melonjs/tests/path2d-arc-connect.spec.ts | Adds focused spec coverage for arc/ellipse/arcTo continuity, virgin-path behavior, and hole/sub-path expectations. |
| packages/melonjs/src/geometries/rectangle.ts | Fixes right/bottom arithmetic and prevents toPolygon() from sharing rect vertices by reference. |
| packages/melonjs/tests/rect-geometry.spec.ts | Adds regression tests for right/bottom at coordinate 0 and toPolygon() vertex independence. |
| packages/melonjs/src/loader/loader.js | Resolves effective src without mutating the caller asset object; adds fontface unload membership guard. |
| packages/melonjs/tests/loader-asset-src.spec.js | Adds regression tests for load() not mutating asset.src and safe fontface unload behavior. |
| packages/melonjs/CHANGELOG.md | Documents the fixed behaviors for Path2D, Rect, and loader. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Batch 2 of the bug-hunt fixes — the loader/geometry mediums, all test-first (8 of 11 new tests red on the old code, plus one updated existing test).
Path2D.arc()/arcTo()/ellipse()started a hole instead of connectingPer the native Path2D spec, an arc appended to a non-empty path is connected to the current point with a straight line —
arcTo's own JSDoc even promises it. All three methods instead calledmoveTo()unconditionally. Under this release's multi-sub-path fill semantics (#1253), that stray sub-path is a hole: an arc appended to a filled outline punched a hole in the shape. Worth landing before 19.9 ships. (The SVGAparser had been quietly working around this — its comment even said "not ellipse/arc which call moveTo and break path continuity".)Fix: a
connectTohelper —lineTowhen the path has content or the pen was explicitly placed,moveToonly on a virgin path. A newpenMovedflag (reset bybeginPath) distinguishes "virgin path" from "pen parked at (0,0)", somoveTo(P); arc(...)keeps its native connecting line. Preserved behaviors, each with a test: virgin-patharc()starts cleanly (no spurious origin line), the canonical circle-hole idiom (moveTobeforearc()) still registers a hole vialineTo's gap detection,roundRect()stays a single sub-path, explicitmoveToholes still register.One existing #1253 adversarial test ("a far disjoint arc() mid-path records a pen-up boundary") codified the old auto-detach accident — updated to assert native connect semantics, with a new companion test for the moveTo hole idiom. (
rect()mid-path correctly keeps its boundary — nativerect()does open a sub-path.)Rectgeometryright/bottom:this.left + w || w— an edge sitting exactly at coordinate 0 is falsy, so the getter returned the width/height. Now plain arithmetic.toPolygon(): passed the rect's liveVector2d[]into the pooled polygon —Polygon.setVerticesstores vector arrays by reference, so transforming the "new" polygon corrupted the rectangle. Vertices are now cloned.Loader asset hygiene
load()mutated the caller's asset descriptor: the resolved URL (fontfaceurl()unwrap +baseURLprefix) was written back intoasset.src, soloader.reload()— which re-loads the same stored object — or simplyload()ing the same manifest entry twice fetchedbase + base + src. The URL is now resolved into a shallow copy handed to the parser; caches,LOADER_ERRORpayloads and failure tracking all keep the caller's originalsrc(which also makesreload(src)keys consistent).unload()of a never-loaded fontface threw:FontFaceSet.delete(undefined)is a WebIDL TypeError, not afalsereturn — the fontface case was the only one missing the membership guard every other type has.Test plan
🤖 Generated with Claude Code
https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A