fix: make clone() copy its rects instead of sharing them

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, or the rect's own `clone()` when its class has one |
 | `src/abstract-bin.ts` | `IBin` / abstract `Bin<T>`: 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`) |
@@ -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
@@ -72,6 +74,23 @@ caller's rects
    swaps width/height itself for objects whose `rot` has no setter.
 7. **Generics**: `MaxRectsPacker<T extends IRectangle>` / `MaxRectsBin<T>` 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). 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.
+   `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
 
@@ -115,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 / 98 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

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()`

scripts/verify-package.mjs

@@ -31,7 +31,16 @@ 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));
+// 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-"));

src/geom/Rectangle.ts

@@ -61,6 +61,46 @@ 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 — 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 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<T extends IRectangle>(rect: T): T {
+        const copier = (rect as { clone?: () => T }).clone;
+        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 state out of reach of a copy (an ECMAScript #private field, say) — give its class a clone() method"
+            );
+        }
+        return copy;
+    }
+
     /**
      * Get the area (w * h) of the rectangle
      *

src/maxrects-bin.ts

@@ -115,11 +115,39 @@ export class MaxRectsBin<T extends IRectangle = Rectangle> extends Bin<T> {
         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, 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<T> {
-        let clonedBin: MaxRectsBin<T> = new MaxRectsBin<T>(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<T> = new MaxRectsBin<T>(this.maxWidth, this.maxHeight, this.padding, {
+            ...this.options,
+            exclusiveTag: false
+        });
         for (let rect of this.rects) {
-            clonedBin.add(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;
     }
 

src/maxrects-packer.ts

@@ -146,7 +146,17 @@ export class MaxRectsPacker<T extends IRectangle = Rectangle> {
             let currentTag: any;
             let currentIdx: number = 0;
             let targetBin = this.bins.slice(this._currentBinIndex).find((bin) => {
-                let testBin = bin.clone();
+                let testBin: Bin<T>;
+                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;

src/oversized-element-bin.ts

@@ -62,8 +62,16 @@ export class OversizedElementBin<T extends IRectangle = Rectangle> extends Bin<T
     repack(): T[] | undefined {
         return undefined;
     }
+    /**
+     * Copy this bin around a copy of its rect, so mutating one bin's rect leaves the other's alone. The
+     * copy is 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 there.
+     *
+     * @returns a bin holding a copy of the same rect, with the same size, data and tag
+     */
     clone(): Bin<T> {
-        let clonedBin: OversizedElementBin<T> = new OversizedElementBin<T>(this.rects[0]);
+        let clonedBin: OversizedElementBin<T> = new OversizedElementBin<T>(Rectangle.Clone(this.rects[0]));
+        clonedBin.tag = this.tag;
         return clonedBin;
     }
 }