From b754acf2a593a42c0e55ec96ab4f036f14ed222a Mon Sep 17 00:00:00 2001 From: Eric J Date: Thu, 30 Jul 2026 16:03:20 -0700 Subject: [PATCH] fix(cliffs): port fixImpossibleCells' self-retry (#18) Decompiling CellEdgeCliffCrossingArray::fixImpossibleCells whole shows its `bool` is not a caller-supplied mode but a retry flag the function sets on ITSELF. On reaching a cell it cannot fix, the tail does uVar10 = param_2 & 1; param_2 = 1; if (uVar10 != 0) { log(...); return; } goto ; turning the flag on and restarting the entire pass, which now begins by zeroing the eight outer edges of the chunk's four corner cells; a second failure abandons the rest of the chunk. The restart re-sweeps the arrays as already mutated by the abandoned pass, not the raw crossings. The note in cliffs-NOTES.md said this step "never runs in this path at all", reasoning from crossingsForChunk passing `false`. That is the general error worth naming: a parameter's value at the call site says what the caller wants, not what the function does with it. Everything else in the sweep already matched. The engine counts non-zero edges and consults the orientation table only when the count is below 3; that is pure optimisation, since all 20 placing codes have one or two crossings, so checking the table directly is equivalent. DOES NOT FIX #18, and the numbers say so rather than an argument. The retry fires once in 512 chunks over the committed captures - one chunk of Vulcanus [1500,1500], zero across both Nauvis seeds and the other two Vulcanus regions - and changes not one placed cell: 335 / 1065 / 375 before and after, recall, precision and the orientation count all identical. This is correctness, not progress. Because the integration fixtures barely execute the branch, it gets a dedicated spec that constructs a stuck corner directly (L = +1, T = -1 at cell (0,0) is code 0x4C, illegal, with both crossings on edges the sweep may not clear). Disabling the retry fails 3 of its 5 tests; the other 2 are the setup assertion and a control that the corner step does NOT fire on a healthy chunk, since it clears eight edges unconditionally and would delete real cliffs. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013eczw9uoWDfN6Wc6kA7UBY --- docs/noise/cliffs-NOTES.md | 33 ++++++- src/noise/cliffs/cliffPlacement.ts | 82 +++++++++++++---- test/fixImpossibleCellsRetry.spec.ts | 127 +++++++++++++++++++++++++++ 3 files changed, 220 insertions(+), 22 deletions(-) create mode 100644 test/fixImpossibleCellsRetry.spec.ts diff --git a/docs/noise/cliffs-NOTES.md b/docs/noise/cliffs-NOTES.md index 0d8577c6..889974b6 100644 --- a/docs/noise/cliffs-NOTES.md +++ b/docs/noise/cliffs-NOTES.md @@ -783,9 +783,36 @@ and compared against `CLIFF_PLACED_TABLE`: **the accepted set is exactly Two corrections to the paragraph this replaced: - It does **not** zero the whole chunk border. The `bool` parameter gates zeroing - the outer edges of the four CORNER cells only (8 edges) - and - `crossingsForChunk` passes **`false`** (`mov w1, #0x0` at `0x10160d0c8`), so - that step never runs in this path at all. + the outer edges of the four CORNER cells only (8 edges). +- ~~`crossingsForChunk` passes **`false`**, so that step never runs in this path + at all.~~ **WRONG, corrected 2026-07-30 by decompiling the function whole.** + The caller does pass `false`, but the `bool` is not a caller-supplied mode - it + is a **retry flag the function sets on itself.** On reaching a cell it cannot + fix, the tail does + + ``` + uVar10 = param_2 & 1; param_2 = 1; + if (uVar10 != 0) { log("Unable to remove excess cliff cell edge crossings"); return; } + goto ; + ``` + + so it turns the flag on and **restarts the entire pass**, which now begins by + zeroing those eight corner edges; a second failure abandons the rest of the + chunk. Note the restart re-sweeps the arrays **as already mutated** by the + abandoned pass, not the raw crossings. + + This is the error the earlier note made, in its general form: reading a + parameter's value at the call site says what the caller wants, not what the + function does with it. Ported 2026-07-30 with `test/fixImpossibleCellsRetry.spec.ts`. + + **Do not read it as a fix for issue #18.** Measured over the committed + captures, the retry fires **once in 512 chunks** - one chunk of Vulcanus + `[1500,1500]`, zero across both Nauvis seeds and the other two Vulcanus regions + - and changes not one placed cell: `[0,0]` 335, `[1500,1500]` 1065, + `[-1200,800]` 375 before and after, with recall, precision and the orientation + count all identical. It is correctness, not progress. Because the integration + fixtures barely execute the branch, it has a dedicated unit spec that builds a + stuck corner directly; disabling the retry fails 3 of its 5 tests. - The binary is a **universal** Mach-O. Raw byte reads of those jump tables need the arm64 slice offset (115654656 here) added, or they silently return x86_64 bytes - which is exactly what happened on the first extraction attempt and diff --git a/src/noise/cliffs/cliffPlacement.ts b/src/noise/cliffs/cliffPlacement.ts index 47dfe6b6..a0df68be 100644 --- a/src/noise/cliffs/cliffPlacement.ts +++ b/src/noise/cliffs/cliffPlacement.ts @@ -157,30 +157,74 @@ function cellCode(l: number, r: number, t: number, b: number): number { * ported. An earlier note in cliffs-NOTES.md described this pass as zeroing the * whole chunk border; it does not, and it does not run at all here. */ -function fixImpossibleCellsSweep(v: Int8Array, h: Int8Array, w: number, hh: number): void { +export function fixImpossibleCellsSweep(v: Int8Array, h: Int8Array, w: number, hh: number): void { const vIndex = (cx: number, cy: number): number => cy * (w + 1) + cx; const hIndex = (cx: number, cy: number): number => cy * w + cx; - for (let cy = 0; cy < hh; cy++) { - for (let cx = 0; cx < w; cx++) { - const li = vIndex(cx, cy); - const ri = vIndex(cx + 1, cy); - const ti = hIndex(cx, cy); - const bi = hIndex(cx, cy + 1); - - for (;;) { - const code = cellCode(v[li], v[ri], h[ti], h[bi]); - if (code === 0 || isCliffPlaced(code)) break; - if (v[li] !== 0 && cx !== 0) v[li] = 0; - else if (h[ti] !== 0 && cy !== 0) h[ti] = 0; - else if (v[ri] !== 0 && cx < w - 1) v[ri] = 0; - else if (h[bi] !== 0 && cy < hh - 1) h[bi] = 0; - // The game logs "Unable to remove excess cliff cell edge crossings" and - // gives up here; every remaining crossing is on the chunk boundary and - // is not ours to clear. - else break; + /** + * The `bool` parameter, and it is a **retry flag the function sets on + * itself** - not a caller-supplied mode, which is how it was read until + * 2026-07-30. `crossingsForChunk` passes `false`, and an earlier note here + * concluded from that alone that the corner step "never runs in this path". + * It does. When the sweep reaches a cell it cannot fix, the disassembly does + * + * uVar10 = param_2 & 1; param_2 = 1; + * if (uVar10 != 0) { log(...); return; } + * goto ; + * + * i.e. it turns the flag on and **restarts the whole pass**, which this time + * begins by zeroing the eight outer edges of the chunk's four corner cells. + * A second failure logs "Unable to remove excess cliff cell edge crossings" + * and abandons the rest of the chunk outright. + * + * Note the restart re-sweeps the arrays **as already mutated** by the + * abandoned pass - it is not a fresh start from the raw crossings. + */ + for (let retry = 0; ; retry++) { + if (retry > 0) { + // The eight edges: the two outer edges of each corner cell. Zeroing these + // is what can make an otherwise unfixable corner cell legal, since its + // only remaining crossings were the ones the sweep is forbidden to clear. + v[vIndex(0, 0)] = 0; + h[hIndex(0, 0)] = 0; + v[vIndex(w, 0)] = 0; + h[hIndex(w - 1, 0)] = 0; + v[vIndex(0, hh - 1)] = 0; + h[hIndex(0, hh)] = 0; + v[vIndex(w, hh - 1)] = 0; + h[hIndex(w - 1, hh)] = 0; + } + + let stuck = false; + for (let cy = 0; cy < hh && !stuck; cy++) { + for (let cx = 0; cx < w && !stuck; cx++) { + const li = vIndex(cx, cy); + const ri = vIndex(cx + 1, cy); + const ti = hIndex(cx, cy); + const bi = hIndex(cx, cy + 1); + + for (;;) { + const code = cellCode(v[li], v[ri], h[ti], h[bi]); + // The engine first counts non-zero edges and only consults the table + // when the count is below 3. That is pure optimisation: every one of + // the 20 placing codes has one or two crossings, so a count of 3 or 4 + // can never be legal. Checking the table directly is equivalent. + if (code === 0 || isCliffPlaced(code)) break; + if (v[li] !== 0 && cx !== 0) v[li] = 0; + else if (h[ti] !== 0 && cy !== 0) h[ti] = 0; + else if (v[ri] !== 0 && cx < w - 1) v[ri] = 0; + else if (h[bi] !== 0 && cy < hh - 1) h[bi] = 0; + else { + stuck = true; + break; + } + } } } + + // Not stuck -> the pass completed. Stuck on the retry -> the engine logs and + // abandons the chunk, leaving the arrays as they are. + if (!stuck || retry > 0) return; } } diff --git a/test/fixImpossibleCellsRetry.spec.ts b/test/fixImpossibleCellsRetry.spec.ts new file mode 100644 index 00000000..154b7a02 --- /dev/null +++ b/test/fixImpossibleCellsRetry.spec.ts @@ -0,0 +1,127 @@ +import { describe, expect, it } from "vite-plus/test"; + +import { fixImpossibleCellsSweep } from "../src/noise/cliffs/cliffPlacement"; +import { isCliffPlaced } from "../src/noise/cliffs/cliffCatalog"; + +const W = 8; +const H = 8; +const vIndex = (cx: number, cy: number): number => cy * (W + 1) + cx; +const hIndex = (cx: number, cy: number): number => cy * W + cx; +const codeOf = (v: Int8Array, h: Int8Array, cx: number, cy: number): number => + ((v[vIndex(cx, cy)] & 3) << 6) | + ((v[vIndex(cx + 1, cy)] & 3) << 4) | + ((h[hIndex(cx, cy)] & 3) << 2) | + (h[hIndex(cx, cy + 1)] & 3); + +/** + * `CellEdgeCliffCrossingArray::fixImpossibleCells`' **retry**, which the port did + * not have until 2026-07-30 (issue #18). + * + * The `bool` parameter was read as a caller-supplied mode, and since + * `crossingsForChunk` passes `false` an earlier note concluded the corner step + * "never runs in this path". It does - the function sets the flag on **itself**: + * + * ``` + * uVar10 = param_2 & 1; param_2 = 1; + * if (uVar10 != 0) { log("Unable to remove excess cliff cell edge crossings"); return; } + * goto ; + * ``` + * + * So on reaching a cell it cannot fix, it restarts the whole pass, this time + * first zeroing the eight outer edges of the chunk's four corner cells; a second + * failure abandons the chunk. + * + * **This spec exists because the integration fixtures barely exercise it.** + * Measured over the committed captures, the retry fires **once in 512 chunks** + * (one chunk of Vulcanus `[1500,1500]`; zero across both Nauvis seeds and the + * other two Vulcanus regions) and changes no placed cell. That is a real + * behaviour and worth having right, but it is nowhere near issue #18's residual + * - do not read this as a fix for it. Without a direct test the branch would be + * effectively dead code. + */ +describe("fixImpossibleCells retry", () => { + /** + * A corner cell whose only crossings are the two chunk-boundary edges it is + * forbidden to clear. `L = +1`, `T = -1` gives code `0x4C`, which is not a + * placing code; `R` and `B` are zero, so the L/T/R/B search finds nothing + * clearable and the first pass is stuck. + */ + const stuckCorner = (): { v: Int8Array; h: Int8Array } => { + const v = new Int8Array((W + 1) * H); + const h = new Int8Array(W * (H + 1)); + v[vIndex(0, 0)] = 1; + h[hIndex(0, 0)] = -1; + return { v, h }; + }; + + it("the constructed cell really is stuck and illegal, or this spec proves nothing", () => { + const { v, h } = stuckCorner(); + const code = codeOf(v, h, 0, 0); + expect(code).toBe(0x4c); + expect(isCliffPlaced(code)).toBe(false); + // Both crossings are on the chunk boundary: L at cx 0, T at cy 0. Neither is + // clearable, and the other two edges are already zero. + expect(v[vIndex(1, 0)]).toBe(0); + expect(h[hIndex(0, 1)]).toBe(0); + }); + + it("restarts and zeroes the corner cell's outer edges, making it legal", () => { + const { v, h } = stuckCorner(); + fixImpossibleCellsSweep(v, h, W, H); + // The retry's corner step is the only thing that can clear these two. + expect(v[vIndex(0, 0)]).toBe(0); + expect(h[hIndex(0, 0)]).toBe(0); + const code = codeOf(v, h, 0, 0); + expect(code).toBe(0); + expect(isCliffPlaced(code) || code === 0).toBe(true); + }); + + it("leaves every cell of the chunk legal", () => { + const { v, h } = stuckCorner(); + fixImpossibleCellsSweep(v, h, W, H); + for (let cy = 0; cy < H; cy++) + for (let cx = 0; cx < W; cx++) { + const code = codeOf(v, h, cx, cy); + expect(code === 0 || isCliffPlaced(code)).toBe(true); + } + }); + + /** + * The corner step must not fire when nothing is stuck - it clears eight edges + * unconditionally, so running it on a healthy chunk would delete real cliffs. + */ + it("does NOT touch the corner edges when the pass completes normally", () => { + const v = new Int8Array((W + 1) * H); + const h = new Int8Array(W * (H + 1)); + // `L = +1`, `T = +1` is code 0x44, which IS a placing code, so cell (0,0) is + // legal as it stands and the sweep has nothing to do anywhere. + v[vIndex(0, 0)] = 1; + h[hIndex(0, 0)] = 1; + expect(isCliffPlaced(codeOf(v, h, 0, 0))).toBe(true); + fixImpossibleCellsSweep(v, h, W, H); + expect(v[vIndex(0, 0)]).toBe(1); + expect(h[hIndex(0, 0)]).toBe(1); + }); + + /** + * Two stuck corners at once. The restart re-sweeps the arrays **as already + * mutated** by the abandoned pass rather than starting from the raw crossings, + * and one retry clears all eight corner edges, so both are fixed in the single + * permitted restart - the second failure would abandon the chunk. + */ + it("fixes two stuck corners in one restart", () => { + const v = new Int8Array((W + 1) * H); + const h = new Int8Array(W * (H + 1)); + v[vIndex(0, 0)] = 1; + h[hIndex(0, 0)] = -1; + v[vIndex(W, H - 1)] = 1; + h[hIndex(W - 1, H)] = -1; + expect(isCliffPlaced(codeOf(v, h, W - 1, H - 1))).toBe(false); + fixImpossibleCellsSweep(v, h, W, H); + for (let cy = 0; cy < H; cy++) + for (let cx = 0; cx < W; cx++) { + const code = codeOf(v, h, cx, cy); + expect(code === 0 || isCliffPlaced(code)).toBe(true); + } + }); +});