diff --git a/packages/melonjs/CHANGELOG.md b/packages/melonjs/CHANGELOG.md index a93502e12..7f9040039 100644 --- a/packages/melonjs/CHANGELOG.md +++ b/packages/melonjs/CHANGELOG.md @@ -25,6 +25,11 @@ - **`moveTowards()` never reached its target — and returned the target instead of `this`** (all four vector classes) — the arrival test compared the remaining distance against `step * step` (wrong units: snapped way too early for steps above 1, dithered around the target forever for fractional steps), and on "arrival" it returned the **caller's target vector** without moving `this` at all — so the vector never landed, and chaining onto the result silently mutated the target. It now lands exactly on the target and always returns `this`, per its own documentation; negative-step flee semantics are unchanged. - **Every preloaded video was fetched with forced anonymous CORS** — the video parser unconditionally stamped the `crossorigin` attribute with the loader's `crossOrigin` setting, which defaults to `undefined` — and per the HTML spec an *invalid* value (the string `"undefined"`) maps to `anonymous`, unlike a *missing* attribute (no CORS). Cross-origin videos served without CORS headers failed to load outright when they would have played fine untainted. The attribute is now only set when `crossOrigin` is actually configured (the empty string remains a valid, honored value). - **Video preloading hung forever on autoplay-restricted browsers (e.g. iOS Safari) unless `autoplay: false` was spelled out** — the parser waited for `canplay`, which those browsers never fire for a video that isn't allowed to play, and only an *explicit* `autoplay: false` opted into the reliable `loadedmetadata` path — so the common manifest shape (no `autoplay` key) stalled the whole preloader and the game never started. Preloading now completes on `loadedmetadata` unless `autoplay: true` explicitly asks to wait for `canplay`. +- **`Path2D.arc()`/`arcTo()`/`ellipse()` appended to an open path started a new sub-path instead of connecting** — the native Path2D spec joins an arc to the current point with a straight line (`arcTo`'s own documentation even promises it), but all three called `moveTo()` unconditionally. Under this release's new multi-sub-path fill semantics that stray sub-path is treated as a **hole**, so an arc appended to a filled outline punched a hole in the shape. They now connect per the spec; the canonical circle-hole idiom (explicit `moveTo` before `arc()`) still registers a hole, and a virgin-path `arc()` still starts cleanly. +- **`Rect.right`/`Rect.bottom` returned the width/height when the edge sat exactly at coordinate 0** — the getters computed `this.left + w || w`, so an edge landing precisely on 0 (falsy) fell back to the size. Bounds and collision math around the origin came out wrong for such rectangles. +- **`Rect.toPolygon()` handed out the rectangle's live vertices** — `Polygon.setVertices` stores a `Vector2d[]` by reference, so the "new" polygon shared the rect's actual points array: transforming or mutating the polygon silently corrupted the rectangle. The vertices are now cloned. +- **`loader.load()` mutated the caller's asset descriptor, so retries double-prepended `baseURL`** — the resolved URL (fontface `url()` unwrap + `baseURL` prefix) was written back into `asset.src`; `loader.reload()` (which re-loads the same stored object) — or simply `load()`ing the same manifest entry twice — then fetched `base + base + src`. The URL is now resolved into a copy handed to the parser; the caller's object is never touched. +- **`loader.unload()` of a never-loaded fontface threw a TypeError** — the fontface case was the only one without a membership guard, and `FontFaceSet.delete(undefined)` is a WebIDL type error rather than a `false` return. It now returns `false` like every other asset type. ## [19.8.0] (melonJS 2) - _2026-06-26_ diff --git a/packages/melonjs/src/geometries/path2d.ts b/packages/melonjs/src/geometries/path2d.ts index 7ba380185..948b82953 100644 --- a/packages/melonjs/src/geometries/path2d.ts +++ b/packages/melonjs/src/geometries/path2d.ts @@ -59,6 +59,16 @@ class Path2D { */ subPaths: number[] = []; + /** + * whether the pen has been explicitly placed (via {@link moveTo}) since + * the last {@link beginPath} — distinguishes "virgin path" from "pen + * parked at (0, 0)" so {@link arc}/{@link ellipse}/{@link arcTo} know + * whether to start fresh or connect from the current point (the native + * Path2D behavior). + * @ignore + */ + private penMoved = false; + /** * space between interpolated points for quadratic and bezier curve approx. in pixels. * @default 2 @@ -158,8 +168,9 @@ class Path2D { ry, xAxisRotation, ); - // generate arc points inline using lineTo (not ellipse/arc - // which call moveTo and break path continuity) + // generate arc points inline using lineTo — the SVG + // endpoint parameterization (large-arc/sweep flags) + // needs custom math that ellipse() doesn't expose { let { startAngle, endAngle } = p; const { @@ -285,6 +296,7 @@ class Path2D { this.points.length = 0; this.subPaths.length = 0; this.startPoint.set(0, 0); + this.penMoved = false; } /** @@ -383,9 +395,27 @@ class Path2D { */ moveTo(x: number, y: number) { this.startPoint.set(x, y); + this.penMoved = true; this.isDirty = true; } + /** + * Start the outline of an arc/ellipse at (x, y): per the native Path2D + * spec, when the path already has content the arc is CONNECTED to the + * current point with a straight line — only a virgin path (no points, no + * explicit moveTo) starts fresh. An unconditional moveTo here would + * silently start a new sub-path, which fill() treats as a HOLE cut out + * of the shape. + * @ignore + */ + private connectTo(x: number, y: number) { + if (this.points.length === 0 && !this.penMoved) { + this.moveTo(x, y); + } else { + this.lineTo(x, y); + } + } + /** * connects the last point in the current sub-path to the (x, y) coordinates with a straight line. * @param x - the x-axis coordinate of the line's end point. @@ -490,7 +520,7 @@ class Path2D { const dangle = diff / nr_of_interpolation_points; const angleStep = dangle * direction; - this.moveTo( + this.connectTo( x + radius * Math.cos(startAngle), y + radius * Math.sin(startAngle), ); @@ -556,7 +586,7 @@ class Path2D { const tangent2_pointx = x1 + b0 * adj_l; const tangent2_pointy = y1 + b1 * adj_l; - this.moveTo(tangent1_pointx, tangent1_pointy); + this.connectTo(tangent1_pointx, tangent1_pointy); let bisec0 = (a0 + b0) / 2.0; let bisec1 = (a1 + b1) / 2.0; @@ -652,7 +682,7 @@ class Path2D { const sx = radiusX * Math.cos(startAngle); const sy = radiusY * Math.sin(startAngle); - this.moveTo( + this.connectTo( x + sx * cos_rotation - sy * sin_rotation, y + sx * sin_rotation + sy * cos_rotation, ); diff --git a/packages/melonjs/src/geometries/rectangle.ts b/packages/melonjs/src/geometries/rectangle.ts index 4782efecf..1611447db 100644 --- a/packages/melonjs/src/geometries/rectangle.ts +++ b/packages/melonjs/src/geometries/rectangle.ts @@ -54,8 +54,7 @@ export class Rect extends Polygon { * right coordinate of the Rectangle */ get right() { - const w = this.width; - return this.left + w || w; + return this.left + this.width; } /** @@ -69,8 +68,7 @@ export class Rect extends Polygon { * bottom coordinate of the Rectangle */ get bottom() { - const h = this.height; - return this.top + h || h; + return this.top + this.height; } /** @@ -290,7 +288,16 @@ export class Rect extends Polygon { * @returns a new Polygon that represents this rectangle. */ toPolygon() { - return polygonPool.get(this.left, this.top, this.points as PolygonVertices); + // clone the vertices: Polygon.setVertices stores a Vector2d[] by + // reference, and handing out this rect's live points would let any + // transform on the "new" polygon silently corrupt the rectangle + return polygonPool.get( + this.left, + this.top, + this.points.map((p) => { + return p.clone(); + }) as PolygonVertices, + ); } } diff --git a/packages/melonjs/src/loader/loader.js b/packages/melonjs/src/loader/loader.js index 27d91ba5d..8be750e15 100644 --- a/packages/melonjs/src/loader/loader.js +++ b/packages/melonjs/src/loader/loader.js @@ -545,24 +545,36 @@ export function load(asset, onload, onerror) { initParsers(); } + // Resolve the effective src WITHOUT mutating the caller's asset + // descriptor: load() used to write the transformed url back into + // asset.src, so retrying the same object — loader.reload() after a + // failure, or simply load()ing the same manifest entry twice — + // prepended baseURL onto an already-transformed src and fetched a + // mangled URL. Caches, events and failure tracking all key on the + // caller's original src; only the parser sees the resolved one. + let src = asset.src; + // strip url() wrapper for fontface assets so baseURL can be prepended to the raw path - if (asset.type === "fontface" && typeof asset.src === "string") { - const urlMatch = asset.src.match(/^url\(\s*['"]?(.*?)['"]?\s*\)$/); + if (asset.type === "fontface" && typeof src === "string") { + const urlMatch = src.match(/^url\(\s*['"]?(.*?)['"]?\s*\)$/); if (urlMatch) { - asset.src = urlMatch[1]; + src = urlMatch[1]; } } // transform the url if necessary (skip for local() font sources and data URIs) if ( typeof baseURL[asset.type] !== "undefined" && - typeof asset.src === "string" && - !asset.src.startsWith("local(") && - !asset.src.startsWith("data:") + typeof src === "string" && + !src.startsWith("local(") && + !src.startsWith("data:") ) { - asset.src = baseURL[asset.type] + asset.src; + src = baseURL[asset.type] + src; } + const resource = + src === asset.src ? asset : Object.assign({}, asset, { src }); + const parser = parsers.get(asset.type); if (typeof parser === "undefined") { @@ -584,7 +596,7 @@ export function load(asset, onload, onerror) { // if it splits into several); 0 means already cached → resolve now. const count = parser.call( this, - asset, + resource, () => { resolve(); }, @@ -598,7 +610,7 @@ export function load(asset, onload, onerror) { } // parser returns the amount of asset to be loaded (usually 1 unless an asset is splitted into several ones) - return parser.call(this, asset, onload, onerror, settings); + return parser.call(this, resource, onload, onerror, settings); } /** @@ -639,6 +651,11 @@ export function unload(asset) { return true; case "fontface": + // membership guard like every other type — FontFaceSet.delete is + // WebIDL-typed and THROWS on undefined instead of returning false + if (!(asset.name in fontList)) { + return false; + } if ( typeof globalThis.document !== "undefined" && typeof globalThis.document.fonts !== "undefined" diff --git a/packages/melonjs/tests/loader-asset-src.spec.js b/packages/melonjs/tests/loader-asset-src.spec.js new file mode 100644 index 000000000..65b313892 --- /dev/null +++ b/packages/melonjs/tests/loader-asset-src.spec.js @@ -0,0 +1,36 @@ +import { afterAll, describe, expect, it } from "vitest"; +import { loader } from "../src/index.js"; + +describe("loader asset descriptor hygiene (audit fixes)", () => { + afterAll(() => { + loader.setBaseURL("json", ""); + }); + + it("load() leaves asset.src untouched, so retrying the same object never double-prepends baseURL", async () => { + loader.setBaseURL("json", "missing-fixture-dir/"); + const asset = { + name: "src-mutation-probe", + type: "json", + src: "probe.json", + }; + + // pre-fix: load() wrote the baseURL-prepended url back into asset.src, + // so loader.reload() (which re-loads the SAME stored object) — or any + // caller retrying its own manifest entry — fetched base+base+src + await expect(loader.load(asset)).rejects.toThrow(); + expect(asset.src).toBe("probe.json"); + + await expect(loader.load(asset)).rejects.toThrow(); + expect(asset.src).toBe("probe.json"); + }); + + it("unload() of a never-loaded fontface returns false instead of throwing", () => { + // pre-fix: document.fonts.delete(fontList[name]) with undefined threw a + // WebIDL TypeError — every other asset type guards membership first + let result; + expect(() => { + result = loader.unload({ name: "never-loaded-ff", type: "fontface" }); + }).not.toThrow(); + expect(result).toBe(false); + }); +}); diff --git a/packages/melonjs/tests/path2d-arc-connect.spec.ts b/packages/melonjs/tests/path2d-arc-connect.spec.ts new file mode 100644 index 000000000..ef78d176c --- /dev/null +++ b/packages/melonjs/tests/path2d-arc-connect.spec.ts @@ -0,0 +1,91 @@ +import { describe, expect, it } from "vitest"; +import Path2D from "../src/geometries/path2d.ts"; + +/** + * Per the native Path2D spec, arc()/ellipse() appended to a non-empty path + * are CONNECTED to the current point with a straight line, and arcTo() is + * "connected to the previous point by a straight line" by definition (it + * says so in its own JSDoc). Our implementations instead called moveTo() + * unconditionally, silently starting a new sub-path. + * + * Written failing-first: with the 19.9 multi-sub-path fill semantics (every + * sub-path after the first is a HOLE), an appended arc didn't just miss its + * connecting line — it punched a hole in the filled shape. The SVG `A` + * parser even worked around this internally rather than calling arc(). + */ +describe("Path2D arc/arcTo/ellipse continuity (native Path2D semantics)", () => { + it("arc() appended to an open path connects with a line instead of starting a hole sub-path", () => { + const p = new Path2D(); + p.beginPath(); + p.moveTo(0, 0); + p.lineTo(100, 0); + // arc starting at (100, 40): the spec adds the segment (100,0) → (100,40) + p.arc(100, 50, 10, -Math.PI / 2, Math.PI / 2); + expect(p.subPaths.length).toBe(0); + // the connecting segment is part of the outline + expect(p.points[2].x).toBeCloseTo(100, 6); + expect(p.points[2].y).toBeCloseTo(0, 6); + expect(p.points[3].x).toBeCloseTo(100, 6); + expect(p.points[3].y).toBeCloseTo(40, 6); + }); + + it("ellipse() appended to an open path connects instead of starting a hole sub-path", () => { + const p = new Path2D(); + p.beginPath(); + p.moveTo(0, 0); + p.lineTo(50, 0); + // ellipse outline starts at (50, 20) + p.ellipse(50, 30, 20, 10, 0, -Math.PI / 2, Math.PI / 2); + expect(p.subPaths.length).toBe(0); + }); + + it("arcTo() appended off the tangent line connects instead of starting a hole sub-path", () => { + const p = new Path2D(); + p.beginPath(); + p.moveTo(0, 0); + p.lineTo(0, 20); + // pen (0,20) is NOT on the incoming tangent — the spec still connects + p.arcTo(50, 0, 50, 50, 20); + expect(p.subPaths.length).toBe(0); + }); + + it("arcTo() after a bare moveTo keeps the connecting line from the pen", () => { + const p = new Path2D(); + p.beginPath(); + p.moveTo(0, 0); + p.arcTo(50, 0, 50, 50, 20); + // native canvas draws (0,0) → tangent (30,0); the old moveTo dropped it + expect(p.points[0].x).toBeCloseTo(0, 6); + expect(p.points[0].y).toBeCloseTo(0, 6); + }); + + it("arc() on a virgin path still starts cleanly at the arc start (no spurious origin line)", () => { + const p = new Path2D(); + p.beginPath(); + p.arc(50, 50, 40, 0, Math.PI * 2); + expect(p.subPaths.length).toBe(0); + expect(p.points[0].x).toBeCloseTo(90, 6); + expect(p.points[0].y).toBeCloseTo(50, 6); + }); + + it("explicit moveTo sub-paths (holes) still register", () => { + const p = new Path2D(); + p.beginPath(); + // outer triangle + p.moveTo(0, 0); + p.lineTo(100, 0); + p.lineTo(50, 80); + // inner hole — an explicit moveTo must STILL start a sub-path + p.moveTo(40, 20); + p.lineTo(60, 20); + p.lineTo(50, 40); + expect(p.subPaths.length).toBe(1); + }); + + it("roundRect() stays a single sub-path", () => { + const p = new Path2D(); + p.beginPath(); + p.roundRect(10, 10, 80, 50, 12); + expect(p.subPaths.length).toBe(0); + }); +}); diff --git a/packages/melonjs/tests/path2d.spec.js b/packages/melonjs/tests/path2d.spec.js index 72a2c6fc9..c2d03c48f 100644 --- a/packages/melonjs/tests/path2d.spec.js +++ b/packages/melonjs/tests/path2d.spec.js @@ -724,10 +724,26 @@ describe("Path2D", () => { expect(path.points[path.subPaths[0]].y).toBe(200); }); - it("a far disjoint arc() mid-path records a pen-up boundary", () => { + it("a far arc() mid-path CONNECTS from the current point (native Path2D semantics)", () => { + // unlike rect() above, native arc() never starts a sub-path on its + // own — it always joins from the current point with a straight + // line. (This originally asserted the pre-fix auto-detach, which + // was the accident of an unconditional moveTo inside arc().) path.moveTo(0, 0); path.lineTo(50, 0); path.arc(300, 300, 20, 0, Math.PI); + expect(path.subPaths.length).toBe(0); + }); + + it("the canonical circle-hole idiom — explicit moveTo before arc() — records the boundary", () => { + // outer square + path.moveTo(0, 0); + path.lineTo(100, 0); + path.lineTo(100, 100); + path.lineTo(0, 100); + // pen-up to the arc start, then the hole circle + path.moveTo(70, 50); + path.arc(50, 50, 20, 0, Math.PI * 2); expect(path.subPaths.length).toBe(1); }); diff --git a/packages/melonjs/tests/rect-geometry.spec.ts b/packages/melonjs/tests/rect-geometry.spec.ts new file mode 100644 index 000000000..37f6b29c5 --- /dev/null +++ b/packages/melonjs/tests/rect-geometry.spec.ts @@ -0,0 +1,24 @@ +import { describe, expect, it } from "vitest"; +import { Rect } from "../src/index.js"; + +describe("Rect geometry (audit fixes)", () => { + it("right/bottom are exact when the edge sits at coordinate 0", () => { + // pre-fix: `this.left + w || w` — a right/bottom edge landing exactly + // on 0 is falsy, so the getter returned the width/height instead + const r = new Rect(-100, -50, 100, 50); + expect(r.right).toBe(0); + expect(r.bottom).toBe(0); + }); + + it("toPolygon() returns an independent polygon (no shared live vertices)", () => { + const r = new Rect(10, 20, 30, 40); + const p = r.toPolygon(); + // pre-fix: setVertices stored the rect's own Vector2d array by + // reference — mutating the polygon corrupted the rectangle + p.points[0].set(999, 999); + expect(r.points[0].x).toBe(0); + expect(r.points[0].y).toBe(0); + expect(r.width).toBe(30); + expect(r.height).toBe(40); + }); +});