From b6a261fb6168e15cb5c18b87bc8f52ed09ac276c Mon Sep 17 00:00:00 2001 From: Shen Yiming Date: Tue, 29 Sep 2026 21:34:25 +0800 Subject: [PATCH 1/4] fix: make clone() copy its rects instead of sharing them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `MaxRectsBin.clone()` and `OversizedElementBin.clone()` returned a bin holding the **same** rect objects as the source, so the two bins were never really separate: `clone.rects[0].width = 999` changed the source bin's rect too, and the source's `rects` then described a rect the bin had not placed. Measured on both classes before this change: clone.rects[i] === bin.rects[i] [true, true, true, true] clone.rects[0].width = 999 -> source width 999 clone.tag (source tag "one") undefined `MaxRectsBin` also dropped the source's `tag` and `data`, so a clone was not even a faithful copy of the bin's metadata. A copy operation that hands out aliases is what made this surprising; the only in-library caller is `addArray()`'s tag-grouping probe, which itself wants a bin that cannot touch the one it is probing. - `Rectangle.Clone(rect)` (new static) copies a rect without going through its setters: the prototype and every own property are kept, so a `Rectangle` stays a `Rectangle`, a custom rect class keeps its identity and extra fields, and a plain `{width, height}` object stays plain. The copy is shallow, so the object in `rect.data` is shared — documented on the static, on both `clone()`s and in `AGENTS.md` (invariant 8). - `MaxRectsBin.clone()` re-packs the copies and carries `tag`/`data` over; `OversizedElementBin.clone()` copies its rect and carries `tag` over. - The published declaration of the new static is pinned by the type fixture in `scripts/verify-package.mjs`; narrowing `Clone` back to `(rect: IRectangle): IRectangle` fails the gate with the fixture line as the only error. The copy is still produced by re-packing rather than by a state copy, and that replay had to stay faithful: with the rects now being copies, a replayed input that differed from the original — a rect whose dimensions rotation had already swapped, say — would land elsewhere. Measured before writing the specs: identical placements on a rotation-heavy fixture and on 80 seeded bins (30 rects each, both rotation modes). `test/maxrects-bin.spec.js` now pins one rotated fixture and sweeps the 80 seeded bins, so the property is a gate rather than a measurement. Behaviour change to call out: code that relied on mutating a rect through one bin to reach the other stops working. Read-only uses (cloning a bin to test whether something still fits) see the same placements as before. Verified against the old implementation: the six new/rewritten assertions fail there — identity on both classes, cross-bin mutation, custom-class identity, and the missing `tag`. Tests 98 -> 105 (2 skipped), coverage still 100% (441/441 statements, 340/340 branches, 76/76 functions, 397/397 lines). --- AGENTS.md | 10 ++- DEFERRED_WORK.md | 15 ++--- scripts/verify-package.mjs | 5 +- src/geom/Rectangle.ts | 13 ++++ src/maxrects-bin.ts | 11 ++- src/oversized-element-bin.ts | 9 ++- test/maxrects-bin.spec.js | 103 +++++++++++++++++++++++++++++ test/oversized-element-bin.spec.js | 22 ++++-- vitest.config.js | 2 +- 9 files changed, 168 insertions(+), 22 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 5a9f883..a55bde0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -27,7 +27,7 @@ possible, each staying within `maxWidth × maxHeight` (sprite sheets / texture a | --- | --- | | `src/index.ts` | The only barrel export. Runtime values: `Rectangle / MaxRectsPacker / PACKING_LOGIC / Bin / MaxRectsBin / OversizedElementBin`; types: `IRectangle / IOption / IBin`. The surface is pinned by `test/index.spec.js` — adding or renaming a public value means updating that list in the same commit — and what consumers can actually import is checked by the type fixture in `npm run verify:package` against `package.json`'s `types` entry | | `src/types.ts` | `IOption`, `PACKING_LOGIC` (MAX_AREA/MAX_EDGE/FILL_WIDTH), `EDGE_MAX_VALUE=4096`, `EDGE_MIN_VALUE=128` (**never used inside the library, but re-exported by `src/maxrects-packer.ts` and part of the public `.d.ts` — not dead code, do not delete**) | -| `src/geom/Rectangle.ts` | `IRectangle` interface + `Rectangle`: `width/height/x/y/rot/data/allowRotation` all go through getters/setters and bump `_dirty` on every mutation; the `rot` setter swaps width/height, the `data` setter syncs `data.allowRotation` | +| `src/geom/Rectangle.ts` | `IRectangle` interface + `Rectangle`: `width/height/x/y/rot/data/allowRotation` all go through getters/setters and bump `_dirty` on every mutation; the `rot` setter swaps width/height, the `data` setter syncs `data.allowRotation`; the `Clone` static copies a rect for the bins' `clone()` (prototype and own properties, shallow) | | `src/abstract-bin.ts` | `IBin` / abstract `Bin`: the `dirty` semantics and `setDirty()`; `add/reset/repack/clone` are left to subclasses | | `src/maxrects-bin.ts` | Core single-bin algorithm: `place → findNode(scoring) → updateBinSize(expand) → splitNode(split) → pruneFreeList` | | `src/oversized-element-bin.ts` | Placeholder bin for one oversized element (`rect.oversized = true`, `add()` always returns `undefined`) | @@ -72,6 +72,12 @@ caller's rects swaps width/height itself for objects whose `rot` has no setter. 7. **Generics**: `MaxRectsPacker` / `MaxRectsBin` accept instances of custom classes and preserve their extra properties as-is. +8. **`clone()` isolates the two bins**: both implementations copy the rects (`Rectangle.Clone`, so a + custom class keeps its prototype and its extra fields) and carry over `tag` and `data`. Mutating a + rect one bin holds, or adding to one bin, therefore never reaches the other. The copy is **shallow** + — the object in `rect.data` is shared — and it is produced by re-packing the copies, which + reproduced the source's placements in every fixture measured (`test/maxrects-bin.spec.js` pins one + rotated fixture and sweeps 80 seeded bins). ## Commands @@ -115,7 +121,7 @@ npx vitest run test/maxrects-packer.spec.js # run a single spec (no rebuild ne extension-less) and take `describe / test / expect / beforeEach` from `vitest` explicitly instead of from globals — **they do not test `dist`**. A broken build or a broken artifact is invisible to them, so compare `dist` by hand whenever you touch the build. -- Baseline: `7 spec files / 98 passed / 2 skipped`; v8 coverage is 100% on statements, branches, +- Baseline: `7 spec files / 105 passed / 2 skipped`; v8 coverage is 100% on statements, branches, functions and lines — removing the dead code recorded in `DEFERRED_WORK.md` took the last uncovered range with it, so no file has a gap left to read. Coverage is **opt-in**: only `npm run cover` collects it and writes `test/coverage/` (gitignored), so a plain `npm test` or a single-spec run diff --git a/DEFERRED_WORK.md b/DEFERRED_WORK.md index c7bd3a5..ae462ae 100644 --- a/DEFERRED_WORK.md +++ b/DEFERRED_WORK.md @@ -8,17 +8,10 @@ tree is adjusted; the reason it cannot move yet is the last section below. ## Code changes (found while closing the test-coverage gaps, PR #73) -One code path has no test coverage because it aliases instead of copying, plus one behaviour question -the measurements raised while the dead code around it was removed. The measurements come from the -coverage report and from deleting the code in question and re-running the suite. - -- **Both `clone()` implementations hand out the same rect objects** (`src/oversized-element-bin.ts:52` - and `src/maxrects-bin.ts:118`). Measured on both classes: `clone.rects[0].width = 100` changes the - original's rect as well, so mutating a clone silently mutates the source bin. `clone()` reads like a - copy operation, which is what makes this surprising. Either document the sharing as intended or copy - the rects — but only `OversizedElementBin` has a spec pinning the identity - (`expect(clone.rects[0]).toBe(bin.rects[0])`); `MaxRectsBin` has no clone spec at all, so a change - there would be caught by nothing. +One behaviour question the measurements raised while the dead code around it was removed. The +measurements came from the coverage report and from deleting the code in question and re-running the +suite. + - **`add()` tags a non-exclusive bin with only the first rect's tag** (`src/maxrects-packer.ts:78`). In non-exclusive mode one bin may hold several tag groups — `test/maxrects-packer.spec.js` pins a bin whose rects carry `one`, `one`, `two`, `two` — so that tag names just one of them, and `save()` diff --git a/scripts/verify-package.mjs b/scripts/verify-package.mjs index d93b2af..7e4233c 100644 --- a/scripts/verify-package.mjs +++ b/scripts/verify-package.mjs @@ -31,7 +31,10 @@ const oversized = new OversizedElementBin(128, 128, null); // somebody narrows it back to a required parameter. const oversizedWithoutData = new OversizedElementBin(128, 128); const rect: IRectangle = new Rectangle(8, 8); -export const summary = [saved.length, bins.length, bin.width, oversized.width, oversizedWithoutData.width, rect.width]; +// The copy helper the bins' clone() uses is part of the public surface, so its declaration is pinned +// too: a static that lost its generic, or its parameter, fails on this line. +const copied: Rectangle = Rectangle.Clone(new Rectangle(4, 4)); +export const summary = [saved.length, bins.length, bin.width, oversized.width, oversizedWithoutData.width, rect.width, copied.width]; `; const root = fileURLToPath(new URL("..", import.meta.url)); const workdir = mkdtempSync(join(tmpdir(), "maxrects-packer-verify-")); diff --git a/src/geom/Rectangle.ts b/src/geom/Rectangle.ts index 75b78e7..621e402 100644 --- a/src/geom/Rectangle.ts +++ b/src/geom/Rectangle.ts @@ -61,6 +61,19 @@ export class Rectangle implements IRectangle { return first.contain(second); } + /** + * Copy a rect object without going through its setters: the prototype and every own property are + * kept, so a `Rectangle` stays a `Rectangle`, a custom rect class keeps its identity and its extra + * fields, and a plain `{width, height}` object stays plain. The copy is shallow — an object held in + * `rect.data`, or in any custom field, is shared with the original. + * + * @param rect - the rect to copy + * @returns a new object carrying the same own properties + */ + public static Clone(rect: T): T { + return Object.assign(Object.create(Object.getPrototypeOf(rect)), rect) as T; + } + /** * Get the area (w * h) of the rectangle * diff --git a/src/maxrects-bin.ts b/src/maxrects-bin.ts index e941677..58dad6b 100644 --- a/src/maxrects-bin.ts +++ b/src/maxrects-bin.ts @@ -115,11 +115,20 @@ export class MaxRectsBin extends Bin { this._dirty = 0; } + /** + * Copy this bin. The copy packs copies of the same rects, so neither bin can reach the other's + * state: mutating a rect one of them holds, or adding to one bin, leaves the other alone. The rect + * copies are shallow (`Rectangle.Clone`), so a payload stored in `rect.data` is still shared. + * + * @returns a bin with the same size, options, tag, data and rects + */ public clone(): MaxRectsBin { let clonedBin: MaxRectsBin = new MaxRectsBin(this.maxWidth, this.maxHeight, this.padding, this.options); for (let rect of this.rects) { - clonedBin.add(rect); + clonedBin.add(Rectangle.Clone(rect)); } + clonedBin.tag = this.tag; + clonedBin.data = this.data; return clonedBin; } diff --git a/src/oversized-element-bin.ts b/src/oversized-element-bin.ts index 9260563..1f6537b 100644 --- a/src/oversized-element-bin.ts +++ b/src/oversized-element-bin.ts @@ -62,8 +62,15 @@ export class OversizedElementBin extends Bin { - let clonedBin: OversizedElementBin = new OversizedElementBin(this.rects[0]); + let clonedBin: OversizedElementBin = new OversizedElementBin(Rectangle.Clone(this.rects[0])); + clonedBin.tag = this.tag; return clonedBin; } } diff --git a/test/maxrects-bin.spec.js b/test/maxrects-bin.spec.js index 7cfc48a..0581bfc 100644 --- a/test/maxrects-bin.spec.js +++ b/test/maxrects-bin.spec.js @@ -294,6 +294,109 @@ describe("no padding", () => { }); }); +describe("clone", () => { + const geometry = (rects) => rects.map((rect) => [rect.x, rect.y, rect.width, rect.height]); + const placements = (target) => target.rects.map((rect) => [rect.x, rect.y, rect.width, rect.height, rect.rot]); + + test("copies the state and gives the copy its own rects", () => { + const bin = new MaxRectsBin(256, 128, 0, { ...opt, allowRotation: true }); + bin.add(new Rectangle(100, 100)); + bin.add(new Rectangle(80, 120)); + + const clone = bin.clone(); + expect(clone.width).toBe(bin.width); + expect(clone.height).toBe(bin.height); + expect(clone.options).toEqual(bin.options); + expect(geometry(clone.freeRects)).toEqual(geometry(bin.freeRects)); + expect(clone.rects).toEqual(bin.rects); + expect(clone.rects[0]).not.toBe(bin.rects[0]); + expect(clone.freeRects[0]).not.toBe(bin.freeRects[0]); + }); + + test("keeps the two bins out of each other's state", () => { + const bin = new MaxRectsBin(256, 128, 0, opt); + bin.add(new Rectangle(100, 100)); + const clone = bin.clone(); + + // Each bin only reaches the rects it holds itself. + clone.rects[0].width = 999; + expect(bin.rects[0].width).toBe(100); + bin.rects[0].height = 777; + expect(clone.rects[0].height).toBe(100); + + // Growing one bin leaves the other where it was. + clone.add(new Rectangle(10, 10)); + expect(clone.rects).toHaveLength(2); + expect(bin.rects).toHaveLength(1); + }); + + test("keeps the prototype and the extra properties of a custom rect", () => { + class Atlas { + constructor(width, height, name) { + this.width = width; + this.height = height; + this.name = name; + } + } + const bin = new MaxRectsBin(256, 128, 0, opt); + const atlas = new Atlas(100, 100, "sheet"); + bin.add(atlas); + + const clone = bin.clone(); + expect(clone.rects[0]).not.toBe(atlas); + expect(clone.rects[0]).toBeInstanceOf(Atlas); + expect(clone.rects[0].name).toBe("sheet"); + expect(clone.rects[0].x).toBe(atlas.x); + expect(clone.rects[0].y).toBe(atlas.y); + // The copy is shallow: the payload object itself is shared. + expect(clone.rects[0].data).toBe(atlas.data); + }); + + test("copies the tag and the data", () => { + const bin = new MaxRectsBin(256, 128, 0, opt); + bin.add(new Rectangle(100, 100)); + bin.tag = "one"; + bin.data = { name: "sheet" }; + + const clone = bin.clone(); + expect(clone.tag).toBe("one"); + expect(clone.data).toBe(bin.data); + }); + + test("reproduces the placements of a bin with rotated rects", () => { + // The copy is re-packed rather than memcpy'd, so this is the check that the replay lands where + // the source did — rotation included. + const bin = new MaxRectsBin(256, 256, 0, { ...opt, allowRotation: true }); + bin.add(new Rectangle(200, 100)); + bin.add(new Rectangle(100, 200)); + expect(bin.rects[1].rot).toBe(true); // the fixture has to rotate a rect, or this proves nothing + + expect(placements(bin.clone())).toEqual(placements(bin)); + }); + + test("reproduces the source placements over seeded bins", () => { + // The single fixture above samples the property; this sweeps it. A copy strategy that changed + // the replayed input — a rect's dimensions swapped by rotation, say — would show up here as a + // different placement, while a shrunken user count would hide it in one fixture. + let seed = 42; + const random = () => { + seed = (seed * 1103515245 + 12345) % 2147483648; + return seed / 2147483648; + }; + for (const allowRotation of [false, true]) { + for (let round = 0; round < 40; round++) { + const bin = new MaxRectsBin(1024, 1024, 1, { ...opt, allowRotation }); + for (let i = 0; i < 30; i++) { + bin.add(new Rectangle(50 + Math.floor(random() * 900), 50 + Math.floor(random() * 900))); + } + expect(placements(bin.clone()), `allowRotation=${allowRotation} round=${round}`).toEqual( + placements(bin) + ); + } + } + }); +}); + const padding = 4; describe("constructor", () => { diff --git a/test/oversized-element-bin.spec.js b/test/oversized-element-bin.spec.js index a681ff2..99e0f77 100644 --- a/test/oversized-element-bin.spec.js +++ b/test/oversized-element-bin.spec.js @@ -47,15 +47,27 @@ describe("OversizedElementBin", () => { expect(bin.repack()).toBeUndefined(); }); - test("#clone holds the same rect object, not a copy", () => { + test("#clone holds a copy of the rect, not the rect itself", () => { const bin = new OversizedElementBin(2000, 2000, { foo: "bar" }); const clone = bin.clone(); expect(clone.width).toBe(2000); expect(clone.rects[0]).toEqual(bin.rects[0]); - // Identity is asserted on purpose, not as an implementation detail: clone() passes the same - // object into the new bin, so `clone.rects[0].width = 100` is visible through the original as - // well. Pinning it means a change to isolate the clone has to be deliberate. - expect(clone.rects[0]).toBe(bin.rects[0]); + // clone() used to hand out the same object, so mutating the clone silently mutated the source + // bin. It copies now: neither bin reaches the other's rect, and only the payload is shared, + // because the copy is shallow. + expect(clone.rects[0]).not.toBe(bin.rects[0]); + expect(clone.rects[0].data).toBe(bin.rects[0].data); + + clone.rects[0].width = 100; + expect(bin.rects[0].width).toBe(2000); + bin.rects[0].height = 50; + expect(clone.rects[0].height).toBe(2000); + }); + + test("#clone copies the tag", () => { + const bin = new OversizedElementBin(2000, 2000, { foo: "bar" }); + bin.tag = "one"; + expect(bin.clone().tag).toBe("one"); }); test("constructor rejects a non-object single argument", () => { diff --git a/vitest.config.js b/vitest.config.js index 8e2ea26..c9cd1f9 100644 --- a/vitest.config.js +++ b/vitest.config.js @@ -15,7 +15,7 @@ export default defineConfig({ include: ["src/**/*.ts"], exclude: ["src/**/*.d.ts"], reporter: ["text", "json", "lcov", "html"], - // Measured: 437/437 statements, 340/340 branches, 75/75 functions, 393/393 lines — every + // Measured: 441/441 statements, 340/340 branches, 76/76 functions, 397/397 lines — every // metric at 100%, because removing the dead code recorded in DEFERRED_WORK.md took the last // uncovered range with it (the counts were identical on Node 22 and 24 when the thresholds // were set). The thresholds stay a notch below the measurement on purpose: they are a From e0264d98a4c02b5d90310e1f790a1d5e3fcbd6d7 Mon Sep 17 00:00:00 2001 From: Shen Yiming Date: Tue, 29 Sep 2026 21:48:05 +0800 Subject: [PATCH 2/4] fix: copy a tagged bin and a private-state rect in clone() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two failures from the review of the previous commit, both reproduced first. **A tagged bin cloned empty.** With `exclusiveTag` in play, `place()` refuses a rect whose tag does not match the bin's, and the copy was built before it had the source's tag — so cloning a tagged bin handed back a bin with no rects at all. Measured on the previous commit: a 1024x1024 bin holding one tagged rect reports `source rects 1, clone rects 0`. Also measured on `HEAD~1`, i.e. before this branch: the same, so the ordering has been wrong for as long as the tag gate has been in `place()`. The obvious repair — put the tag on the copy first — breaks the mirror case, which the existing "copies the tag and the data" spec covers: a bin tagged *after* it was filled holds untagged rects, and the gate then refuses those. So the replay now runs with the tag gate **off** (`{...this.options, exclusiveTag: false}`) and `options`, `tag` and `data` are restored from the source afterwards. The source already settled which rects it accepts; re-deciding that while copying is what dropped them. Both orderings are pinned by specs. **A rect class with ECMAScript `#private` fields could not be copied.** Such a rect can be packed — `add()` stores `x`/`y`/`rot` as own properties and reads `width`/`height` through getters — but `Object.assign` cannot reach a private slot, so the copy threw `TypeError: Cannot read private member #width from an object whose class did not declare it` deep inside the replay. `Rectangle.Clone` now uses the rect's own `clone()` method when its class provides one, and otherwise reports the limitation where the copy is made, naming the fix, instead of leaving a TypeError to surface from a bin that is only trying to place it. Sharing the rect instead of copying it was the other option and is exactly the aliasing this branch removed. **Third finding, from measuring the above:** the replay silently dropped a rect it could not place. A placed rect the caller resizes out of its bin (a 100x100 rect taken to 4000 wide in a 256x128 bin) made `clone()` hand back a bin holding fewer rects than the original, with nothing said. That now throws and asks for a repack. All four new assertions fail on the previous commit with the symptoms above (`expected [] to have a length of 1 but got +0`, the private-member TypeError, the unmatched error message, and the missing throw); all pass here. Tests 105 -> 109 (2 skipped), coverage still 100% (452/452 statements, 344/344 branches, 76/76 functions, 407/407 lines). --- AGENTS.md | 12 +++-- src/geom/Rectangle.ts | 20 +++++++- src/maxrects-bin.ts | 25 ++++++++-- src/oversized-element-bin.ts | 3 +- test/maxrects-bin.spec.js | 96 ++++++++++++++++++++++++++++++++++++ vitest.config.js | 2 +- 6 files changed, 148 insertions(+), 10 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index a55bde0..c80e455 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -27,7 +27,7 @@ possible, each staying within `maxWidth × maxHeight` (sprite sheets / texture a | --- | --- | | `src/index.ts` | The only barrel export. Runtime values: `Rectangle / MaxRectsPacker / PACKING_LOGIC / Bin / MaxRectsBin / OversizedElementBin`; types: `IRectangle / IOption / IBin`. The surface is pinned by `test/index.spec.js` — adding or renaming a public value means updating that list in the same commit — and what consumers can actually import is checked by the type fixture in `npm run verify:package` against `package.json`'s `types` entry | | `src/types.ts` | `IOption`, `PACKING_LOGIC` (MAX_AREA/MAX_EDGE/FILL_WIDTH), `EDGE_MAX_VALUE=4096`, `EDGE_MIN_VALUE=128` (**never used inside the library, but re-exported by `src/maxrects-packer.ts` and part of the public `.d.ts` — not dead code, do not delete**) | -| `src/geom/Rectangle.ts` | `IRectangle` interface + `Rectangle`: `width/height/x/y/rot/data/allowRotation` all go through getters/setters and bump `_dirty` on every mutation; the `rot` setter swaps width/height, the `data` setter syncs `data.allowRotation`; the `Clone` static copies a rect for the bins' `clone()` (prototype and own properties, shallow) | +| `src/geom/Rectangle.ts` | `IRectangle` interface + `Rectangle`: `width/height/x/y/rot/data/allowRotation` all go through getters/setters and bump `_dirty` on every mutation; the `rot` setter swaps width/height, the `data` setter syncs `data.allowRotation`; the `Clone` static copies a rect for the bins' `clone()` — prototype and own properties, shallow, or the rect's own `clone()` when its class has one | | `src/abstract-bin.ts` | `IBin` / abstract `Bin`: the `dirty` semantics and `setDirty()`; `add/reset/repack/clone` are left to subclasses | | `src/maxrects-bin.ts` | Core single-bin algorithm: `place → findNode(scoring) → updateBinSize(expand) → splitNode(split) → pruneFreeList` | | `src/oversized-element-bin.ts` | Placeholder bin for one oversized element (`rect.oversized = true`, `add()` always returns `undefined`) | @@ -77,7 +77,13 @@ caller's rects rect one bin holds, or adding to one bin, therefore never reaches the other. The copy is **shallow** — the object in `rect.data` is shared — and it is produced by re-packing the copies, which reproduced the source's placements in every fixture measured (`test/maxrects-bin.spec.js` pins one - rotated fixture and sweeps 80 seeded bins). + rotated fixture and sweeps 80 seeded bins). That replay runs with the tag gate **off** and restores + the source's `options`/`tag`/`data` afterwards: a bin can hold rects its own gate would now refuse + — it was tagged after it was filled, or it carries several tags in non-exclusive mode — and + re-running the gate would give back a copy with fewer rects than the original. Two cases fail + loudly instead of silently: `Rectangle.Clone` reports a rect class that hides its width or height + behind an ECMAScript `#private` field and offers no `clone()` of its own, and `MaxRectsBin.clone()` + reports a bin holding a rect it can no longer place. ## Commands @@ -121,7 +127,7 @@ npx vitest run test/maxrects-packer.spec.js # run a single spec (no rebuild ne extension-less) and take `describe / test / expect / beforeEach` from `vitest` explicitly instead of from globals — **they do not test `dist`**. A broken build or a broken artifact is invisible to them, so compare `dist` by hand whenever you touch the build. -- Baseline: `7 spec files / 105 passed / 2 skipped`; v8 coverage is 100% on statements, branches, +- Baseline: `7 spec files / 109 passed / 2 skipped`; v8 coverage is 100% on statements, branches, functions and lines — removing the dead code recorded in `DEFERRED_WORK.md` took the last uncovered range with it, so no file has a gap left to read. Coverage is **opt-in**: only `npm run cover` collects it and writes `test/coverage/` (gitignored), so a plain `npm test` or a single-spec run diff --git a/src/geom/Rectangle.ts b/src/geom/Rectangle.ts index 621e402..9fa45a6 100644 --- a/src/geom/Rectangle.ts +++ b/src/geom/Rectangle.ts @@ -67,11 +67,27 @@ export class Rectangle implements IRectangle { * fields, and a plain `{width, height}` object stays plain. The copy is shallow — an object held in * `rect.data`, or in any custom field, is shared with the original. * + * A rect class that keeps its state out of reach of `Object.assign` — behind an ECMAScript + * `#private` field — cannot be copied this way, and has to hand out a `clone()` method instead, + * which is used whenever it exists. Without one the copy would throw on the first getter reading + * such a field, so that case is reported here rather than from inside a bin trying to place it. + * * @param rect - the rect to copy - * @returns a new object carrying the same own properties + * @returns a new object carrying the same own properties, or whatever `rect.clone()` returns */ public static Clone(rect: T): T { - return Object.assign(Object.create(Object.getPrototypeOf(rect)), rect) as T; + const copier = (rect as { clone?: () => T }).clone; + if (typeof copier === "function") return copier.call(rect); + const copy = Object.assign(Object.create(Object.getPrototypeOf(rect)), rect) as T; + try { + void copy.width; + void copy.height; + } catch { + throw new Error( + "Rectangle.Clone(): the rect keeps its width or height out of reach of a copy (an ECMAScript #private field, say) — give its class a clone() method" + ); + } + return copy; } /** diff --git a/src/maxrects-bin.ts b/src/maxrects-bin.ts index 58dad6b..6b72845 100644 --- a/src/maxrects-bin.ts +++ b/src/maxrects-bin.ts @@ -118,15 +118,34 @@ export class MaxRectsBin extends Bin { /** * Copy this bin. The copy packs copies of the same rects, so neither bin can reach the other's * state: mutating a rect one of them holds, or adding to one bin, leaves the other alone. The rect - * copies are shallow (`Rectangle.Clone`), so a payload stored in `rect.data` is still shared. + * copies are shallow (`Rectangle.Clone`), so a payload stored in `rect.data` is still shared, and a + * rect class that cannot be copied that way reports it from `Rectangle.Clone()`. + * + * It throws when the copy cannot hold a rect this bin holds — only reachable when a placed rect has + * been resized into something the bin can no longer place. Reporting it beats handing back a bin + * that quietly holds fewer rects than the original. * * @returns a bin with the same size, options, tag, data and rects */ public clone(): MaxRectsBin { - let clonedBin: MaxRectsBin = new MaxRectsBin(this.maxWidth, this.maxHeight, this.padding, this.options); + // The replay runs with the tag gate off. The source already settled which rects it accepts — in + // exclusive mode that is its own tag, but a bin tagged after it was filled, or one carrying + // several tags in non-exclusive mode, holds rects its own gate would now refuse, and re-running + // it would give back a copy with fewer rects than the original. `options`, `tag` and `data` are + // restored to the source's below. + let clonedBin: MaxRectsBin = new MaxRectsBin(this.maxWidth, this.maxHeight, this.padding, { + ...this.options, + exclusiveTag: false + }); for (let rect of this.rects) { - clonedBin.add(Rectangle.Clone(rect)); + const copy = Rectangle.Clone(rect); + if (clonedBin.add(copy) === undefined) { + throw new Error( + "MaxRectsBin.clone(): the bin holds a rect it can no longer place, so the copy would be incomplete — repack the bin before cloning it" + ); + } } + clonedBin.options = { ...this.options }; clonedBin.tag = this.tag; clonedBin.data = this.data; return clonedBin; diff --git a/src/oversized-element-bin.ts b/src/oversized-element-bin.ts index 1f6537b..eac985c 100644 --- a/src/oversized-element-bin.ts +++ b/src/oversized-element-bin.ts @@ -64,7 +64,8 @@ export class OversizedElementBin extends Bin { test("copies the tag and the data", () => { const bin = new MaxRectsBin(256, 128, 0, opt); bin.add(new Rectangle(100, 100)); + // The tag lands on an already-filled bin, so the copy has to carry a tagged bin whose rects are + // untagged: the replay must not re-run the tag gate, or it would drop them. bin.tag = "one"; bin.data = { name: "sheet" }; @@ -363,6 +365,100 @@ describe("clone", () => { expect(clone.data).toBe(bin.data); }); + test("copies a tagged bin under exclusiveTag", () => { + // The tag has to be on the copy before the rects are replayed, or `place()` refuses every tagged + // rect and the copy comes back empty — which is what it did until now. + const bin = new MaxRectsBin(1024, 1024, 0, { ...opt, exclusiveTag: true }); + bin.tag = "one"; + const rect = new Rectangle(100, 100); + rect.data = { tag: "one" }; + expect(bin.add(rect)).toBeDefined(); + + const clone = bin.clone(); + expect(clone.rects).toHaveLength(1); + expect(clone.tag).toBe("one"); + expect(clone.rects[0]).not.toBe(rect); + }); + + test("uses the rect's own clone() when its class has one", () => { + // A class holding its state behind `#private` fields cannot be copied property by property, so it + // gets to say how — and the copy still ends up independent of the original. + class PrivateRect { + #width; + #height; + constructor(width, height) { + this.#width = width; + this.#height = height; + } + get width() { + return this.#width; + } + set width(value) { + this.#width = value; + } + get height() { + return this.#height; + } + set height(value) { + this.#height = value; + } + clone() { + return new PrivateRect(this.#width, this.#height); + } + } + const bin = new MaxRectsBin(256, 256, 0, opt); + const rect = new PrivateRect(100, 100); + bin.add(rect); + + const clone = bin.clone(); + expect(clone.rects[0]).toBeInstanceOf(PrivateRect); + expect(clone.rects[0]).not.toBe(rect); + expect(clone.rects[0].width).toBe(100); + + clone.rects[0].width = 42; + expect(rect.width).toBe(100); + }); + + test("reports a rect class it cannot copy", () => { + class PrivateRect { + #width; + #height; + constructor(width, height) { + this.#width = width; + this.#height = height; + } + get width() { + return this.#width; + } + set width(value) { + this.#width = value; + } + get height() { + return this.#height; + } + set height(value) { + this.#height = value; + } + } + const bin = new MaxRectsBin(256, 256, 0, opt); + bin.add(new PrivateRect(100, 100)); + + // With no `clone()` to fall back on, the copy would throw on the first getter reading `#width`, so + // the message says what to add. Sharing the rect instead is the aliasing this change removed. + expect(() => bin.clone()).toThrow("give its class a clone() method"); + }); + + test("reports a rect it can no longer place", () => { + // A placed rect the caller resized out of the bin would make the copy incomplete; an error beats + // a bin that quietly holds fewer rects than the original. + const bin = new MaxRectsBin(256, 128, 0, opt); + const rect = new Rectangle(100, 100); + bin.add(rect); + rect.width = 4000; + + expect(() => bin.clone()).toThrow("repack the bin before cloning it"); + }); + test("reproduces the placements of a bin with rotated rects", () => { // The copy is re-packed rather than memcpy'd, so this is the check that the replay lands where // the source did — rotation included. diff --git a/vitest.config.js b/vitest.config.js index c9cd1f9..493c884 100644 --- a/vitest.config.js +++ b/vitest.config.js @@ -15,7 +15,7 @@ export default defineConfig({ include: ["src/**/*.ts"], exclude: ["src/**/*.d.ts"], reporter: ["text", "json", "lcov", "html"], - // Measured: 441/441 statements, 340/340 branches, 76/76 functions, 397/397 lines — every + // Measured: 452/452 statements, 344/344 branches, 76/76 functions, 407/407 lines — every // metric at 100%, because removing the dead code recorded in DEFERRED_WORK.md took the last // uncovered range with it (the counts were identical on Node 22 and 24 when the thresholds // were set). The thresholds stay a notch below the measurement on purpose: they are a From c8decb258bd145fc8af04a76c62f85367ea61257 Mon Sep 17 00:00:00 2001 From: Shen Yiming Date: Tue, 29 Sep 2026 22:05:51 +0800 Subject: [PATCH 3/4] fix: copy every own property in Rectangle.Clone() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of e0264d9 found the copy contract was only half kept: `Object.assign` copies *enumerable* own properties, so anything a rect keeps non-enumerable was silently dropped. Measured on that commit, a rect class with non-enumerable `width`/`height` did not merely lose fields — the copy read `width: undefined`, and cloning its bin blew up with `RangeError: Maximum call stack size exceeded` after 162s of growth probing, i.e. the review's "misreported as a rect it can no longer place" case was in practice a stack overflow. The private-state guard was also size-only: a class with plain `width`/`height` and `#private` `x`/`y` passed the guard and then died inside the replay with `Cannot write private member #x to an object whose class did not declare it`, instead of the actionable error this branch promises. - The copy now takes `Object.getOwnPropertyDescriptors()` and applies them with `Object.defineProperties()`, so non-enumerable fields — extra ones and the backing fields behind `width`/`height` — arrive intact. - The guard reads every field the replay touches (`width`, `height`, `x`, `y`, `rot`, `data`), not just the size, so the private-state failure is the documented "give its class a clone() method" one. - A class with its own `clone()` is now only the *base* of the copy: the original's own property descriptors are applied on top. Review also flagged that a `clone()` rebuilding just the dimensions (the tempting implementation) dropped the payload, the tag and everything added since construction — `clone.rects[0].data` came back `undefined`, and the same applied to an `OversizedElementBin`'s payload. That is fixed by the same descriptor pass. Verified against e0264d9 with the three new specs, which fail there with exactly the symptoms above: `copies non-enumerable own properties` (RangeError), `reports a rect whose placement is out of reach` (private-brand TypeError instead of the message), `keeps what a narrow custom clone() leaves out` (`expected undefined to be { sheet: 'atlas-1' }`). Also pinned the generic in the published types, as the review suggested: the `verify:package` fixture now copies a `SheetRect extends Rectangle` carrying an extra field, and narrowing `Clone` to `(rect: IRectangle): IRectangle` fails the gate with two errors before the fix is restored. Tests 109 -> 112 (2 skipped), coverage still 100% (455/455 statements, 344/344 branches, 76/76 functions, 411/411 lines). --- AGENTS.md | 15 ++++--- scripts/verify-package.mjs | 8 +++- src/geom/Rectangle.ts | 31 ++++++++++----- test/maxrects-bin.spec.js | 81 ++++++++++++++++++++++++++++++++++++++ vitest.config.js | 2 +- 5 files changed, 120 insertions(+), 17 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index c80e455..a843bfa 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -80,10 +80,15 @@ caller's rects rotated fixture and sweeps 80 seeded bins). That replay runs with the tag gate **off** and restores the source's `options`/`tag`/`data` afterwards: a bin can hold rects its own gate would now refuse — it was tagged after it was filled, or it carries several tags in non-exclusive mode — and - re-running the gate would give back a copy with fewer rects than the original. Two cases fail - loudly instead of silently: `Rectangle.Clone` reports a rect class that hides its width or height - behind an ECMAScript `#private` field and offers no `clone()` of its own, and `MaxRectsBin.clone()` - reports a bin holding a rect it can no longer place. + re-running the gate would give back a copy with fewer rects than the original. + `Rectangle.Clone` copies *own property descriptors* rather than assigned values, so a rect keeping + extra fields, or the backing fields behind `width`/`height`, non-enumerable still arrives intact; + a class with its own `clone()` supplies the base object and the original's own properties are + applied on top, so a `clone()` that rebuilds only the dimensions does not drop the payload, the + placement or anything added since construction. Two cases fail loudly instead of silently: a rect + class that hides state the copy cannot reach (an ECMAScript `#private` field behind `width`, + `height`, `x`, `y`, `rot` or `data`) and offers no `clone()` of its own, and a bin holding a rect it + can no longer place. ## Commands @@ -127,7 +132,7 @@ npx vitest run test/maxrects-packer.spec.js # run a single spec (no rebuild ne extension-less) and take `describe / test / expect / beforeEach` from `vitest` explicitly instead of from globals — **they do not test `dist`**. A broken build or a broken artifact is invisible to them, so compare `dist` by hand whenever you touch the build. -- Baseline: `7 spec files / 109 passed / 2 skipped`; v8 coverage is 100% on statements, branches, +- Baseline: `7 spec files / 112 passed / 2 skipped`; v8 coverage is 100% on statements, branches, functions and lines — removing the dead code recorded in `DEFERRED_WORK.md` took the last uncovered range with it, so no file has a gap left to read. Coverage is **opt-in**: only `npm run cover` collects it and writes `test/coverage/` (gitignored), so a plain `npm test` or a single-spec run diff --git a/scripts/verify-package.mjs b/scripts/verify-package.mjs index 7e4233c..052c03a 100644 --- a/scripts/verify-package.mjs +++ b/scripts/verify-package.mjs @@ -34,7 +34,13 @@ const rect: IRectangle = new Rectangle(8, 8); // The copy helper the bins' clone() uses is part of the public surface, so its declaration is pinned // too: a static that lost its generic, or its parameter, fails on this line. const copied: Rectangle = Rectangle.Clone(new Rectangle(4, 4)); -export const summary = [saved.length, bins.length, bin.width, oversized.width, oversizedWithoutData.width, rect.width, copied.width]; +// The generic has to survive: a subclass must come back as that subclass, extra field included, or +// this line stops compiling. +class SheetRect extends Rectangle { + label = "sheet"; +} +const copiedSheet: SheetRect = Rectangle.Clone(new SheetRect(4, 4)); +export const summary = [saved.length, bins.length, bin.width, oversized.width, oversizedWithoutData.width, rect.width, copied.width, copiedSheet.label]; `; const root = fileURLToPath(new URL("..", import.meta.url)); const workdir = mkdtempSync(join(tmpdir(), "maxrects-packer-verify-")); diff --git a/src/geom/Rectangle.ts b/src/geom/Rectangle.ts index 9fa45a6..c92683e 100644 --- a/src/geom/Rectangle.ts +++ b/src/geom/Rectangle.ts @@ -63,28 +63,39 @@ export class Rectangle implements IRectangle { /** * Copy a rect object without going through its setters: the prototype and every own property are - * kept, so a `Rectangle` stays a `Rectangle`, a custom rect class keeps its identity and its extra - * fields, and a plain `{width, height}` object stays plain. The copy is shallow — an object held in - * `rect.data`, or in any custom field, is shared with the original. + * kept — non-enumerable ones included — so a `Rectangle` stays a `Rectangle`, a custom rect class + * keeps its identity and its extra fields, and a plain `{width, height}` object stays plain. The + * copy is shallow — an object held in `rect.data`, or in any custom field, is shared. * - * A rect class that keeps its state out of reach of `Object.assign` — behind an ECMAScript - * `#private` field — cannot be copied this way, and has to hand out a `clone()` method instead, - * which is used whenever it exists. Without one the copy would throw on the first getter reading - * such a field, so that case is reported here rather than from inside a bin trying to place it. + * A rect class that keeps its state out of reach of a copy — behind an ECMAScript `#private` field — + * cannot be copied this way and has to hand out a `clone()` method instead, which is used whenever + * it exists. Its result is only the base: the original's own properties are applied on top, because + * a `clone()` that rebuilds just the dimensions would otherwise drop the payload, the placement and + * anything added since construction. When no `clone()` exists the copy would throw on the first + * field it cannot reach, so that is reported here rather than from inside a bin placing the copy. * * @param rect - the rect to copy * @returns a new object carrying the same own properties, or whatever `rect.clone()` returns */ public static Clone(rect: T): T { const copier = (rect as { clone?: () => T }).clone; - if (typeof copier === "function") return copier.call(rect); - const copy = Object.assign(Object.create(Object.getPrototypeOf(rect)), rect) as T; + const copy = + typeof copier === "function" ? copier.call(rect) : (Object.create(Object.getPrototypeOf(rect)) as T); + // Descriptors rather than `Object.assign`: a rect may keep extra fields, or the backing fields + // behind `width`/`height`, non-enumerable, and assignment would drop those silently. + Object.defineProperties(copy, Object.getOwnPropertyDescriptors(rect)); try { + // Every field the packing replay reads or writes, not just the size: a class backing `x`, `y`, + // `rot` or `data` with a `#private` field passes a size-only check and then dies inside a bin. void copy.width; void copy.height; + void copy.x; + void copy.y; + void copy.rot; + void copy.data; } catch { throw new Error( - "Rectangle.Clone(): the rect keeps its width or height out of reach of a copy (an ECMAScript #private field, say) — give its class a clone() method" + "Rectangle.Clone(): the rect keeps its state out of reach of a copy (an ECMAScript #private field, say) — give its class a clone() method" ); } return copy; diff --git a/test/maxrects-bin.spec.js b/test/maxrects-bin.spec.js index 81e5172..cca91de 100644 --- a/test/maxrects-bin.spec.js +++ b/test/maxrects-bin.spec.js @@ -459,6 +459,87 @@ describe("clone", () => { expect(() => bin.clone()).toThrow("repack the bin before cloning it"); }); + test("copies non-enumerable own properties", () => { + // A rect is free to keep its extra fields — or the backing fields behind width/height — out of + // enumeration. `Object.assign` would drop those, and a hidden `width` would then read as + // `undefined` and be blamed on a rect that "can no longer be placed". + class HiddenRect { + constructor(width, height, label) { + Object.defineProperty(this, "width", { value: width, enumerable: false, writable: true }); + Object.defineProperty(this, "height", { value: height, enumerable: false, writable: true }); + Object.defineProperty(this, "label", { value: label, enumerable: false, writable: true }); + } + } + const bin = new MaxRectsBin(256, 256, 0, opt); + const rect = new HiddenRect(100, 100, "hidden"); + expect(bin.add(rect)).toBeDefined(); + + const clone = bin.clone(); + expect(clone.rects).toHaveLength(1); + expect(clone.rects[0]).not.toBe(rect); + expect(clone.rects[0].label).toBe("hidden"); + expect(Object.getOwnPropertyDescriptor(clone.rects[0], "label").enumerable).toBe(false); + expect([clone.rects[0].x, clone.rects[0].y]).toEqual([rect.x, rect.y]); + }); + + test("reports a rect whose placement is out of reach", () => { + // The size is a plain field here and only `x`/`y` sit behind `#private`, so a size-only check + // would wave this copy through and then die on the setter the replay uses. + class PrivatePlacementRect { + #x = 0; + #y = 0; + constructor(width, height) { + this.width = width; + this.height = height; + } + get x() { + return this.#x; + } + set x(value) { + this.#x = value; + } + get y() { + return this.#y; + } + set y(value) { + this.#y = value; + } + } + const bin = new MaxRectsBin(256, 256, 0, opt); + bin.add(new PrivatePlacementRect(100, 100)); + + expect(() => bin.clone()).toThrow("give its class a clone() method"); + }); + + test("keeps what a narrow custom clone() leaves out", () => { + // A class whose clone() rebuilds only the dimensions is the tempting implementation; the copy + // still has to carry the payload, the placement and the extra fields the bin holds it with. + class MinimalRect { + constructor(width, height) { + this.width = width; + this.height = height; + } + clone() { + const copy = new MinimalRect(this.width, this.height); + copy.cloneWasCalled = true; + return copy; + } + } + const bin = new MaxRectsBin(256, 256, 0, opt); + const rect = new MinimalRect(100, 100); + // A payload without a `tag`: `opt` tags exclusively, and an untagged bin refuses a tagged rect. + rect.data = { sheet: "atlas-1" }; + rect.extra = "custom metadata"; + expect(bin.add(rect)).toBeDefined(); + + const copy = bin.clone().rects[0]; + expect(copy.cloneWasCalled).toBe(true); // the class's own clone() is still what builds the base + expect(copy).not.toBe(rect); + expect(copy.data).toBe(rect.data); + expect(copy.extra).toBe("custom metadata"); + expect([copy.x, copy.y, copy.rot]).toEqual([rect.x, rect.y, rect.rot]); + }); + test("reproduces the placements of a bin with rotated rects", () => { // The copy is re-packed rather than memcpy'd, so this is the check that the replay lands where // the source did — rotation included. diff --git a/vitest.config.js b/vitest.config.js index 493c884..cf763f7 100644 --- a/vitest.config.js +++ b/vitest.config.js @@ -15,7 +15,7 @@ export default defineConfig({ include: ["src/**/*.ts"], exclude: ["src/**/*.d.ts"], reporter: ["text", "json", "lcov", "html"], - // Measured: 452/452 statements, 344/344 branches, 76/76 functions, 407/407 lines — every + // Measured: 455/455 statements, 344/344 branches, 76/76 functions, 411/411 lines — every // metric at 100%, because removing the dead code recorded in DEFERRED_WORK.md took the last // uncovered range with it (the counts were identical on Node 22 and 24 when the thresholds // were set). The thresholds stay a notch below the measurement on purpose: they are a From 9d6ad9525287e1f137ecdb002205edb7ae4a8188 Mon Sep 17 00:00:00 2001 From: Shen Yiming Date: Tue, 29 Sep 2026 22:15:09 +0800 Subject: [PATCH 4/4] fix: keep an uncopyable bin from failing the whole addArray() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The grouping probe in non-exclusive tag mode (`addArray()`) copies each candidate bin. `clone()` now throws rather than returning an incomplete copy, and the probe had no handler, so one uncopyable bin aborted the entire call: the later bins were never tried even when one could hold the group, and the rects after the current group were left unpacked. Reachable in two ways, both real: - a placed rect the caller resized out of its bin, which `clone()` reports on purpose; - a rect class whose state a copy cannot reach (an ECMAScript `#private` field, no `clone()` of its own): such a rect packs fine through `add()`, but its bin cannot be copied, so the clone API's limitation leaked into the packing call. The probe now treats a bin that cannot be copied as one the group does not fit — `catch { return false }` — so the search continues and the existing "no bin fits, open a new one" path handles the rest. Calling `clone()` directly still throws, with its own message. Spec `does not let an uncopyable bin abort the tag grouping` covers the reported shape: the first bin holds a rect whose class hides its size behind `#private` fields, the second bin has room, and the group has to land there. On the previous commit it fails with `Error: Rectangle.Clone(): the rect keeps its state out of reach of a copy …` escaping `addArray()`; here the call completes, the packer keeps two bins and the group's two rects join the second one. Tests 112 -> 113 (2 skipped), coverage still 100% (457/457 statements, 344/344 branches, 76/76 functions, 413/413 lines). --- AGENTS.md | 6 +++-- src/maxrects-packer.ts | 12 +++++++++- test/maxrects-packer.spec.js | 43 ++++++++++++++++++++++++++++++++++++ vitest.config.js | 2 +- 4 files changed, 59 insertions(+), 4 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index a843bfa..5ee0630 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -52,7 +52,9 @@ caller's rects the same tag, and an untagged bin refuses tagged rects. The check lives in `MaxRectsBin.place()`, which every path reaches — `MaxRectsBin.add()` deliberately does not pre-check, so a refused rect is simply the `undefined` `place()` returns. `exclusiveTag: false` takes the grouping + recursion - path inside `addArray()`. + path inside `addArray()`, which probes a candidate bin by **copying** it: a bin whose `clone()` + throws (invariant 8) counts as one the group does not fit, so a single uncopyable bin can never + fail the whole `addArray()` call. 3. **`next()` only affects what comes after**: it sets `_currentBinIndex = bins.length`, so earlier bins stop accepting new elements and every lookup starts at that index. 4. **Dirty propagation**: mutating a `Rectangle` property increments `_dirty`; `Bin.dirty` is true @@ -132,7 +134,7 @@ npx vitest run test/maxrects-packer.spec.js # run a single spec (no rebuild ne extension-less) and take `describe / test / expect / beforeEach` from `vitest` explicitly instead of from globals — **they do not test `dist`**. A broken build or a broken artifact is invisible to them, so compare `dist` by hand whenever you touch the build. -- Baseline: `7 spec files / 112 passed / 2 skipped`; v8 coverage is 100% on statements, branches, +- Baseline: `7 spec files / 113 passed / 2 skipped`; v8 coverage is 100% on statements, branches, functions and lines — removing the dead code recorded in `DEFERRED_WORK.md` took the last uncovered range with it, so no file has a gap left to read. Coverage is **opt-in**: only `npm run cover` collects it and writes `test/coverage/` (gitignored), so a plain `npm test` or a single-spec run diff --git a/src/maxrects-packer.ts b/src/maxrects-packer.ts index 35069a8..c06b885 100644 --- a/src/maxrects-packer.ts +++ b/src/maxrects-packer.ts @@ -146,7 +146,17 @@ export class MaxRectsPacker { let currentTag: any; let currentIdx: number = 0; let targetBin = this.bins.slice(this._currentBinIndex).find((bin) => { - let testBin = bin.clone(); + let testBin: Bin; + try { + testBin = bin.clone(); + } catch { + // A bin that cannot be copied cannot be probed either, and `clone()` is deliberate + // about throwing (a rect it can no longer place, or a rect class whose state a copy + // cannot reach). Treat it as "this group does not fit this bin": the search goes on, + // and a new bin is opened below if nothing else takes the group. Letting it through + // would turn one un-copyable bin into a failed `addArray()` call. + return false; + } for (let i = currentIdx; i < rects.length; i++) { const rect = rects[i]; const tag = rect.data && rect.data.tag ? rect.data.tag : rect.tag ? rect.tag : undefined; diff --git a/test/maxrects-packer.spec.js b/test/maxrects-packer.spec.js index 447cd6f..4ae5b8e 100644 --- a/test/maxrects-packer.spec.js +++ b/test/maxrects-packer.spec.js @@ -325,6 +325,49 @@ describe("#addArray", () => { expect(packer.rects).toHaveLength(1); expect(packer.rects[0].oversized).toBe(true); }); + + test("does not let an uncopyable bin abort the tag grouping", () => { + // Grouping probes a candidate bin by copying it, and `clone()` now throws rather than returning + // an incomplete copy. An uncopyable bin — here one holding a rect class whose state no shallow + // copy can reach — must count as "this group does not fit it", so the search reaches the next + // bin instead of failing the whole call. + class PrivateRect { + #width; + #height; + constructor(width, height) { + this.#width = width; + this.#height = height; + } + get width() { + return this.#width; + } + set width(value) { + this.#width = value; + } + get height() { + return this.#height; + } + set height(value) { + this.#height = value; + } + } + packer = new MaxRectsPacker(1024, 1024, 0, { ...opt, tag: true, exclusiveTag: false }); + packer.add(new PrivateRect(900, 900)); + packer.add(new Rectangle(200, 200)); // opens the second bin + expect(packer.bins).toHaveLength(2); + expect(() => packer.bins[0].clone()).toThrow("give its class a clone() method"); + + expect(() => + packer.addArray([ + { width: 300, height: 300, data: { tag: "one" } }, + { width: 300, height: 300, data: { tag: "one" } } + ]) + ).not.toThrow(); + + expect(packer.bins).toHaveLength(2); + expect(packer.bins[0].rects).toHaveLength(1); + expect(packer.bins[1].rects).toHaveLength(3); + }); }); describe("#save & load", () => { diff --git a/vitest.config.js b/vitest.config.js index cf763f7..c0d2507 100644 --- a/vitest.config.js +++ b/vitest.config.js @@ -15,7 +15,7 @@ export default defineConfig({ include: ["src/**/*.ts"], exclude: ["src/**/*.d.ts"], reporter: ["text", "json", "lcov", "html"], - // Measured: 455/455 statements, 344/344 branches, 76/76 functions, 411/411 lines — every + // Measured: 457/457 statements, 344/344 branches, 76/76 functions, 413/413 lines — every // metric at 100%, because removing the dead code recorded in DEFERRED_WORK.md took the last // uncovered range with it (the counts were identical on Node 22 and 24 when the thresholds // were set). The thresholds stay a notch below the measurement on purpose: they are a