From 623eb0ca3b679f254f51fa0d5d7282d795a3cfaf Mon Sep 17 00:00:00 2001 From: Richard Higgins Date: Sat, 3 Oct 2026 00:59:36 +0200 Subject: [PATCH 1/2] Expose explicit-layer tile heights for map validation --- docs/shared-engine-upstream-review.md | 101 ++++++++++++++++++++++++++ src/polyworld/pathing.nim | 8 +- tests/test_tile_paths.nim | 23 ++++++ 3 files changed, 130 insertions(+), 2 deletions(-) create mode 100644 docs/shared-engine-upstream-review.md diff --git a/docs/shared-engine-upstream-review.md b/docs/shared-engine-upstream-review.md new file mode 100644 index 00000000..f22ba43c --- /dev/null +++ b/docs/shared-engine-upstream-review.md @@ -0,0 +1,101 @@ +# Shared engine API review + +Inventory date: October 3, 2026. Upstream baseline: `449ad184052567c30fa54c269ef45ff8c9e8e29b`. +This review extracts one CPU API; downstream integrations remain with their owners. + +## Selected extraction + +`tileTop(QuadLayer, x, z)` samples a supplied layer without replacing installed +pathing state. The existing `tileTop(layerIndex, x, z)` delegates to it. Packed +height units, integer operation order, and rounding toward negative infinity +are unchanged. This does not change terrain generation or rendering. + +Cogcraft already uses this overload in +`native/src/cogcraft/polyworld_hollowdeep_client_frame.nim`: +`polyworldHollowdeepSurfaceHeight(source, tile)` validates effects, workshops, +sites, and gates against the supplied map before installing that map. Its +explicit layer access avoids consulting another map's global pathing context. +The overload is retained in Cogcraft's vendored `pathing.nim` at reviewed head +`422a1cb907e754367f7cd8fc7841baeaea7fe2c9`. Available history reaches the +`89b8ca7` boundary; that boundary is not claimed as the original authoring commit. + +## Consumer and history inventory + +Cogcraft's current `native/vendor/polyworld` is a tracked source tree, **not a +Git submodule**. `native/vendor/PROVENANCE.md` records upstream base +`49e6d49cfa661d941254557fda7eb5e851afe6b5` and local compatibility patches. +Review its game history and source diffs rather than treating the whole vendor +tree as a cherry-pickable engine branch. + +Puzzle Pirates uses `vendor/polyworld` as a submodule. Its reviewed game head +and submodule record retain `f866e2fddabce9df909ad79de31fe2e2d73592b9`, already +reachable from the upstream baseline. Reviewed game head: +`b23c513b0ef5915f5c40b69e075ec48668b4044e`. Game history includes `be0501b0` +(follow main during dependency setup/CI) and `acf43526` (advance the pin). +`tools/deps.py` follows main unless `--locked` is supplied, so a checked-in pin +alone does not establish the engine used in a build. Its presentation clock, +motion, and vessel motion remain local game consumers. No vendor pins changed. + +## Existing PRs and disposition + +The open Polyworld PR census contains **49, 50, 51, 53, 89, 92**. GotA controls, +practice sessions, cast feedback, neural heads, and PR92's source qualification +runner remain author-owned. This extraction does not duplicate their files. + +- [PR52](https://github.com/Metta-AI/polyworld/pull/52) is merged. Its + `98d6814` commit is reachable from main; it changes GotA release discovery + through Coworld summaries, not shared graphics or pathing APIs. +- [Cogcraft PR52](https://github.com/Metta-AI/coworld-of-cogcraft/pull/52) + is also merged; it adds configurable client keybinds/settings and remains + outside this shared engine API extraction. +- PR3's immutable pathing contexts, PR6's posed picking, PR40's character + playback/attachments, and PR70's resource lifecycle are already merged. + Review current implementations before replaying preserved relh branches. +- [PR61](https://github.com/Metta-AI/polyworld/pull/61) was explicitly closed + because no Polyworld caller adopted the clock and Pirates retains a richer + local clock. No standalone clock resubmission is justified here. +- PR62–66 (redraw scheduling, floating poses, custom toon shaders, grid motion, + and WebGL context callbacks) are closed. Preserve their branches; any future + extraction needs a current adopting consumer and the appropriate owner's + runtime proof, not a wholesale vendor import. +- PR59's BASIC pause extension and PR60's game-bound host refactor were closed + after PR55's trainer redesign removed their consumers. PR7's frustum query + remains deferred without a measured bottleneck/caller. PR5/8's attachment + and animation work is superseded by PR40; composed static scenes remain + a separate future use case. + +## Deferred surfaces and ownership + +The four-host census found active game render, terrain, UI, camera, persistence, +and QA work. MBP also has the PR92 qualification worktree. Shared checkouts and +all those task worktrees remain untouched; this PR uses a fresh Git worktree. +No repository worktree helper or nested AGENTS/LESSONS file was found in the +upstream tree. Global synchronization was attempted, then the explicit +fetch/isolation instruction preserved the divergent local main. + +Cogcraft's caller-supplied crossing neighbors, topology cache changes, and +explicit-source ray queries need separate integration review. Crossing edges +must preserve admissible A* heuristics, callback ordering, tie breaks, and +smoothing semantics; copying the entire pathing fork would also import unrelated +cache and renderer-facing behavior. Toon/unlit/shadow changes, terrain paint, +wind, water, cutout mips, character crossfades, and HUD/input patches stay with +their active owners. This document is the source handoff, not an acceptance +claim for those patches. + +## Proof and remaining gaps + +The CPU `test_tile_paths` contract checks every possible four-corner sum +(-131072 through 131068), supplied-layer indexing, unchanged installed state, +negative rounding, extrema, and compatibility with the indexed API. The +existing `test_pathing` contract covers context reuse, path smoothing, occupancy, +and mirrored tie ordering. Full games and browser/graphics acceptance are +outside this proof. Private compiler logs and source snapshots are retained +outside Git. Installed proof dependencies are recorded there; no locked-cohort +or graphics-performance claim follows from these small CPU checks. + +Cogcraft main continued advancing during the inventory; the reviewed snapshot +is pinned above. A later API read reported `0dceefde`, but retrieving its source +timed out. Recheck later downstream commits before adopting this extraction. + +Richard reviews the unmerged PR. Downstream adoption, broader vendor history +reconstruction, and runtime proofs for deferred surfaces remain open. diff --git a/src/polyworld/pathing.nim b/src/polyworld/pathing.nim index 1a453ad8..65e396fe 100644 --- a/src/polyworld/pathing.nim +++ b/src/polyworld/pathing.nim @@ -566,14 +566,14 @@ proc tileCenter*(layerIndex, x, z: int): Vec3 = (layer.originZ + z).float32 - HalfGrid + 0.5 ) -proc tileTop*(layerIndex, x, z: int): int32 = +proc tileTop*(layer: QuadLayer; x, z: int): int32 = + ## Samples the supplied layer without installing it as global pathing state. ## Mean height of a tile's four top corners, in the same 1/8-tile integer ## steps that `Tile.tops` stores. Integer throughout, so a simulation can ## price a ramp step without ever touching `unpack` and its float32. ## Division floors toward negative infinity so the result is stable for ## tiles below y = 0 rather than biased toward zero. let - layer = layers[layerIndex] tops = layer.tiles[z * layer.width + x].tops total = int32(tops[0]) + int32(tops[1]) + int32(tops[2]) + int32(tops[3]) if total >= 0: @@ -581,6 +581,10 @@ proc tileTop*(layerIndex, x, z: int): int32 = else: -((-total + 3) div 4) +proc tileTop*(layerIndex, x, z: int): int32 = + ## Samples an installed layer with the same integer rounding. + layers[layerIndex].tileTop(x, z) + proc pathPoint*(layerIndex, x, z: int): PathPoint = ## Returns one exact integer tile center for authoritative game setup. let index = nodeIndex(layerIndex, x, z) diff --git a/tests/test_tile_paths.nim b/tests/test_tile_paths.nim index 92e2729a..b62cbe8c 100644 --- a/tests/test_tile_paths.nim +++ b/tests/test_tile_paths.nim @@ -194,6 +194,29 @@ block: layers[0].tiles[4].tops = [1'i16, 1'i16, 1'i16, 0'i16] doAssert tileTop(0, 1, 1) == 0, "mean of 3/4 truncates to 0" +echo "Testing explicit tile heights before installing a map" +block: + let + installed = QuadLayer(width: 1, depth: 1, + tiles: @[Tile(tops: [8'i16, 8, 8, 8])]) + candidate = QuadLayer(width: 2, depth: 2, tiles: newSeq[Tile](4)) + layers = @[installed] + # Every possible corner sum, including both int16 extrema and negative + # fractional means. No floating point enters the expected value. + for total in -131072'i32 .. 131068'i32: + var remaining = total + for corner in 0 .. 3: + let value = min(32767'i32, max(-32768'i32, remaining)) + candidate.tiles[3].tops[corner] = int16(value) + remaining -= value + doAssert remaining == 0 + let expected = (total + 131072) div 4 - 32768 + doAssert candidate.tileTop(1, 1) == expected + doAssert tileTop(0, 0, 0) == 8 + doAssert layers.len == 1 and layers[0] == installed + layers = @[candidate] + doAssert tileTop(0, 1, 1) == candidate.tileTop(1, 1) + echo "Testing tileTop matches a ramp's rise per tile" block: const Rise = 8'i16 From 8050b69f6a84f8ff0ca0294e244102ee7d204822 Mon Sep 17 00:00:00 2001 From: Richard Higgins Date: Mon, 5 Oct 2026 17:51:53 -0700 Subject: [PATCH 2/2] Keep only the explicit-layer tile height API --- docs/shared-engine-upstream-review.md | 101 -------------------------- tests/test_tile_paths.nim | 23 ------ 2 files changed, 124 deletions(-) delete mode 100644 docs/shared-engine-upstream-review.md diff --git a/docs/shared-engine-upstream-review.md b/docs/shared-engine-upstream-review.md deleted file mode 100644 index f22ba43c..00000000 --- a/docs/shared-engine-upstream-review.md +++ /dev/null @@ -1,101 +0,0 @@ -# Shared engine API review - -Inventory date: October 3, 2026. Upstream baseline: `449ad184052567c30fa54c269ef45ff8c9e8e29b`. -This review extracts one CPU API; downstream integrations remain with their owners. - -## Selected extraction - -`tileTop(QuadLayer, x, z)` samples a supplied layer without replacing installed -pathing state. The existing `tileTop(layerIndex, x, z)` delegates to it. Packed -height units, integer operation order, and rounding toward negative infinity -are unchanged. This does not change terrain generation or rendering. - -Cogcraft already uses this overload in -`native/src/cogcraft/polyworld_hollowdeep_client_frame.nim`: -`polyworldHollowdeepSurfaceHeight(source, tile)` validates effects, workshops, -sites, and gates against the supplied map before installing that map. Its -explicit layer access avoids consulting another map's global pathing context. -The overload is retained in Cogcraft's vendored `pathing.nim` at reviewed head -`422a1cb907e754367f7cd8fc7841baeaea7fe2c9`. Available history reaches the -`89b8ca7` boundary; that boundary is not claimed as the original authoring commit. - -## Consumer and history inventory - -Cogcraft's current `native/vendor/polyworld` is a tracked source tree, **not a -Git submodule**. `native/vendor/PROVENANCE.md` records upstream base -`49e6d49cfa661d941254557fda7eb5e851afe6b5` and local compatibility patches. -Review its game history and source diffs rather than treating the whole vendor -tree as a cherry-pickable engine branch. - -Puzzle Pirates uses `vendor/polyworld` as a submodule. Its reviewed game head -and submodule record retain `f866e2fddabce9df909ad79de31fe2e2d73592b9`, already -reachable from the upstream baseline. Reviewed game head: -`b23c513b0ef5915f5c40b69e075ec48668b4044e`. Game history includes `be0501b0` -(follow main during dependency setup/CI) and `acf43526` (advance the pin). -`tools/deps.py` follows main unless `--locked` is supplied, so a checked-in pin -alone does not establish the engine used in a build. Its presentation clock, -motion, and vessel motion remain local game consumers. No vendor pins changed. - -## Existing PRs and disposition - -The open Polyworld PR census contains **49, 50, 51, 53, 89, 92**. GotA controls, -practice sessions, cast feedback, neural heads, and PR92's source qualification -runner remain author-owned. This extraction does not duplicate their files. - -- [PR52](https://github.com/Metta-AI/polyworld/pull/52) is merged. Its - `98d6814` commit is reachable from main; it changes GotA release discovery - through Coworld summaries, not shared graphics or pathing APIs. -- [Cogcraft PR52](https://github.com/Metta-AI/coworld-of-cogcraft/pull/52) - is also merged; it adds configurable client keybinds/settings and remains - outside this shared engine API extraction. -- PR3's immutable pathing contexts, PR6's posed picking, PR40's character - playback/attachments, and PR70's resource lifecycle are already merged. - Review current implementations before replaying preserved relh branches. -- [PR61](https://github.com/Metta-AI/polyworld/pull/61) was explicitly closed - because no Polyworld caller adopted the clock and Pirates retains a richer - local clock. No standalone clock resubmission is justified here. -- PR62–66 (redraw scheduling, floating poses, custom toon shaders, grid motion, - and WebGL context callbacks) are closed. Preserve their branches; any future - extraction needs a current adopting consumer and the appropriate owner's - runtime proof, not a wholesale vendor import. -- PR59's BASIC pause extension and PR60's game-bound host refactor were closed - after PR55's trainer redesign removed their consumers. PR7's frustum query - remains deferred without a measured bottleneck/caller. PR5/8's attachment - and animation work is superseded by PR40; composed static scenes remain - a separate future use case. - -## Deferred surfaces and ownership - -The four-host census found active game render, terrain, UI, camera, persistence, -and QA work. MBP also has the PR92 qualification worktree. Shared checkouts and -all those task worktrees remain untouched; this PR uses a fresh Git worktree. -No repository worktree helper or nested AGENTS/LESSONS file was found in the -upstream tree. Global synchronization was attempted, then the explicit -fetch/isolation instruction preserved the divergent local main. - -Cogcraft's caller-supplied crossing neighbors, topology cache changes, and -explicit-source ray queries need separate integration review. Crossing edges -must preserve admissible A* heuristics, callback ordering, tie breaks, and -smoothing semantics; copying the entire pathing fork would also import unrelated -cache and renderer-facing behavior. Toon/unlit/shadow changes, terrain paint, -wind, water, cutout mips, character crossfades, and HUD/input patches stay with -their active owners. This document is the source handoff, not an acceptance -claim for those patches. - -## Proof and remaining gaps - -The CPU `test_tile_paths` contract checks every possible four-corner sum -(-131072 through 131068), supplied-layer indexing, unchanged installed state, -negative rounding, extrema, and compatibility with the indexed API. The -existing `test_pathing` contract covers context reuse, path smoothing, occupancy, -and mirrored tie ordering. Full games and browser/graphics acceptance are -outside this proof. Private compiler logs and source snapshots are retained -outside Git. Installed proof dependencies are recorded there; no locked-cohort -or graphics-performance claim follows from these small CPU checks. - -Cogcraft main continued advancing during the inventory; the reviewed snapshot -is pinned above. A later API read reported `0dceefde`, but retrieving its source -timed out. Recheck later downstream commits before adopting this extraction. - -Richard reviews the unmerged PR. Downstream adoption, broader vendor history -reconstruction, and runtime proofs for deferred surfaces remain open. diff --git a/tests/test_tile_paths.nim b/tests/test_tile_paths.nim index b62cbe8c..92e2729a 100644 --- a/tests/test_tile_paths.nim +++ b/tests/test_tile_paths.nim @@ -194,29 +194,6 @@ block: layers[0].tiles[4].tops = [1'i16, 1'i16, 1'i16, 0'i16] doAssert tileTop(0, 1, 1) == 0, "mean of 3/4 truncates to 0" -echo "Testing explicit tile heights before installing a map" -block: - let - installed = QuadLayer(width: 1, depth: 1, - tiles: @[Tile(tops: [8'i16, 8, 8, 8])]) - candidate = QuadLayer(width: 2, depth: 2, tiles: newSeq[Tile](4)) - layers = @[installed] - # Every possible corner sum, including both int16 extrema and negative - # fractional means. No floating point enters the expected value. - for total in -131072'i32 .. 131068'i32: - var remaining = total - for corner in 0 .. 3: - let value = min(32767'i32, max(-32768'i32, remaining)) - candidate.tiles[3].tops[corner] = int16(value) - remaining -= value - doAssert remaining == 0 - let expected = (total + 131072) div 4 - 32768 - doAssert candidate.tileTop(1, 1) == expected - doAssert tileTop(0, 0, 0) == 8 - doAssert layers.len == 1 and layers[0] == installed - layers = @[candidate] - doAssert tileTop(0, 1, 1) == candidate.tileTop(1, 1) - echo "Testing tileTop matches a ramp's rise per tile" block: const Rise = 8'i16