Skip to content

feat: WP-07 site and asset pipeline - #119

Merged
N1k4G merged 10 commits into
mainfrom
site/wp-07-asset-pipeline
Aug 23, 2026
Merged

feat: WP-07 site and asset pipeline#119
N1k4G merged 10 commits into
mainfrom
site/wp-07-asset-pipeline

Conversation

@N1k4G

@N1k4G N1k4G commented Aug 4, 2026

Copy link
Copy Markdown
Owner

Moves dive sites to validated, renderer-neutral resources with an asset manifest, layer factories, culling, pooling and quality tiers, per Codex_Review.md WP-07.

Both acceptance criteria have passing tests

"Invalid site data fails at build/test time." src/sites/site-resources.ts validates on import and throws, targeting the failures that are silent rather than loud:

  • lerpProfile walks points assuming ascending x. Unsorted input does not throw — it interpolates against the wrong segment and returns a plausible depth for the wrong place.
  • An inverted structure box can never be hit by solidAt, so a wall quietly stops existing.

"One asset can be replaced without changing simulation or collision data." Re-pointing an asset at a different atlas and frame leaves every gameplay value byte-identical, and the two documents are asserted structurally disjoint so an art edit cannot reach collision again.

The separation is structural, not documentary

src/sites.js mixed collision, air, spawn and current data with features, visual zones, atmosphere and decoration rules in one object — so "change how a site looks" and "change where the diver can swim" were the same edit.

The generator partitions them into two documents and refuses to run if a descriptor grows a field in neither list. A new field must be deliberately classified, never silently dropped or silently promoted into collision.

Parity is proven against the real legacy source loaded via ?raw, not a snapshot, replaying floorAt, ceilingAt, solidAt, overheadAt and badAirAt across a sampled grid of every site including beyond the profile ends where clamping applies.

The parity test earned its place immediately

The first implementation folded lerpProfile's division into its multiplication:

legacy:  t = (x - a.x) / (b.x - a.x);  return a.d + (b.d - a.d) * t
mine:    return a.d + ((b.d - a.d) * (x - a.x)) / (b.x - a.x)

Algebraically identical, numerically not. It moved results by one ULP and broke parity on two of four sites. The operation order is now preserved deliberately, with a comment saying why.

The typechecker caught a second assumption: SiteFeature was modelled as a uniform {kind, x, d}, but real features include depth spans (dTop/dBottom) and kind-specific extras.

Renderer

The Pixi renderer added children in a flat list, so draw order was an accident of call order. It now owns named layer containers and populates them from the layer factory, with pooled markers and resync skipped until the camera has actually travelled.

Two deliberate limits:

  • Markers are plain placeholder shapes. Production atlases are BLOCKED_EXTERNAL, so this draws something obviously provisional rather than art guessed at here. The manifest fixes the atlas/frame contract, so substituting real textures will not touch the renderer's structure.
  • The hand-authored hull, seabed and engine stay. They are bespoke art for one scene, not authored site content; replacing them with markers would degrade the slice to prove a point the decoration layer already proves.

Known gap, worth raising before WP-08

The visual risk in the renderer change is unguarded. compare-rendering.mjs gates the legacy client only, so nothing here would catch a Pixi visual regression. This was verified structurally — build, e2e, layer assignment — but not visually.

Extending the pixel comparison to the Pixi client is the natural next work, and I would want it before WP-08 starts moving real scenes.

Verification

npm ci, npm run lint, npm run typecheck, npm run build, and npm test all pass. No change to the legacy client, the numerical model or the golden traces.

N1k4G added 3 commits August 4, 2026 17:19
WP-07 begins by making the separation structural rather than documented. The
legacy descriptors mix collision, air, spawn and current data with features,
visual zones, atmosphere and decoration rules in one object, so "change how a
site looks" and "change where the diver can swim" were the same edit.

scripts/generate-site-resources.mjs partitions the descriptors into two
renderer-neutral documents and refuses to run if a descriptor grows a field
that is in neither list. A new field must be deliberately classified, never
silently dropped or silently promoted into collision.

src/sites/site-resources.ts validates on import and throws rather than
serving geometry that corrupts quietly:

- lerpProfile walks points assuming ascending x. Unsorted input does not
  throw; it interpolates against the wrong segment and returns a plausible
  depth for the wrong place.
- An inverted structure box can never be hit by solidAt, so a wall silently
  stops existing.

tests/parity/site-geometry.test.ts replays floorAt, ceilingAt, solidAt,
overheadAt and badAirAt against the real legacy source across a sampled grid
of every site, including beyond the profile ends where clamping applies.

That test earned its place immediately: the first implementation folded
lerpProfile's division into its multiplication, which is algebraically
identical and numerically is not, and it broke parity on two sites by one ULP.
The operation order is now preserved deliberately, with a comment saying why.
The Pixi slice built its wreck from hand-written Graphics calls and read no
site data at all, which is the scaling limit WP-07 exists to remove. This adds
the data path without yet rewiring the renderer.

The manifest maps every authored feature kind to one asset, with an atlas, a
layer and a minimum quality tier. The mapping is hand-authored because it is
art direction, but completeness is enforced by test in both directions: a new
feature kind fails until someone assigns it an asset, and an orphaned entry
fails until someone removes it. Neither can rot quietly.

The layer factory turns presentation data into an ordered, culled placement
list and builds no Pixi objects, so it is testable without a GPU and a second
renderer could consume the same output. Draw order is sorted rather than
inherited from authoring order, so the same inputs always produce the same
scene. Quality tiers drop decoration before the things a diver navigates by.

Feature shapes are modelled as authored rather than as assumed: some are
points with a `d`, others span `dTop`/`dBottom`, and kinds carry their own
extra fields. The first version assumed a uniform `{kind, x, d}` and the
typechecker rejected it against the real data.

WP-07's acceptance criterion now has a test: re-pointing an asset at a
different atlas and frame leaves every gameplay value byte-identical, and the
two documents are asserted structurally disjoint so an art edit cannot reach
collision again.
The renderer added its children in a flat list, so draw order was an accident
of call order and there was nowhere for authored content to go. It now owns
named layer containers matching the manifest, and populates them from the
layer factory.

Placement markers are pooled. Which features are visible changes many times
during a dive, and allocating a Graphics per feature per resync would churn
the heap for a scene whose contents barely change. Resync is also skipped
until the camera has actually travelled, rather than running every frame.

Markers are deliberately plain shapes. Production atlases are BLOCKED_EXTERNAL,
so this draws a placeholder rather than art guessed at here; the manifest
already fixes the atlas and frame contract they will load through, so
substituting real textures does not touch this file's structure.

The hand-authored hull, seabed and engine stay as they are. They are bespoke
art for one scene, not authored site content, and replacing them with markers
would degrade the slice to prove a point that the decoration layer already
proves.
@github-actions github-actions Bot added the release:minor Minor production release / user-visible feature label Aug 4, 2026
@N1k4G

N1k4G commented Aug 21, 2026

Copy link
Copy Markdown
Owner Author

One question worth settling before this merges: which side of the boundary do props live on?

Tracked as #123. Not a defect in this PR — a question about the contract it establishes, raised here because this is the PR that locks it in (the generator's classification lists, the schema, and the disjointness test all land in this diff).

What the split currently decides

gameplay.json     — wreck: [id, maxDepth, hasOverhead, entry, boatX, floor, ceiling, structures, badAir, …]
presentation.json — wreck: [id, name, surfaceMarker, features, visualZones, atmosphereProfiles, decorationRules]

solidAt() reads structures (gameplay). The vehicle deck's 12 cars and lorries, and the two cave cathedral columns, are features — so they land in presentation, and tests/unit/site-assets.test.ts:127 asserts they can never carry structures.

That is the PR working exactly as designed, and the guarantee is a good one. The consequence is just worth naming explicitly.

Why it matters now

Those props currently have no collision at all. Verified on main by driving the physics loop directly:

  • Cave cathedral column A (x:88, dTop:56, dBottom:98, wBot:11) — solidAt returns false across the entire footprint x = 82…94 at d = 75. The diver reaches x = 88.15, dead centre of an 11 m column, still reading open water.
  • Wreck vehicle deck — a probe grid over the car at x = 34 and lorry at x = 54, sampled at body height (d = 36.5…38.5, i.e. above the dTop:39 floor slab), is entirely open water. The diver swims from x = 30 straight through the car at x = 34 with no collision registered.

For the cave columns this is a documented choice — src/sites.js:671-672 says so outright:

// Neither introduces collision — solidAt() only tests the AABB
// structures list, and neither column is added there.

The two ways this resolves

  1. Props are decoration, permanently. Nothing changes in this PR; [Bug] Props (vehicle-deck cars, cave cathedral columns) have no collision #123 gets closed as a documented limitation rather than a bug.
  2. Some props should collide. That is an edit to the classification lists and schema in this PR — much cheaper than a data migration once the generator is in use and resources are generated.

Either answer is fine. I'd just rather it be a decision than a default, since after merge "give this car a hitbox" stops being a one-line edit next to the car and becomes a deliberate gameplay-side entry.

Unrelated note on the same subsystem

solidAt() is not reimplemented here — the parity test replays the legacy function against the new data, which is the right call. Worth knowing that it takes no diver-extent parameter at all, so its centre-point behaviour carries across unchanged, and the new validation (profile ordering, box inversion, depth reachability) doesn't cover it. Filed as #122; nothing for this PR to do about it.


Context: this came out of re-verifying the findings in .tmp/frontend-audit against source. Nothing else in that audit touches this PR — the other findings all live in the legacy client (renderer.js, style.css, physics.js, constants.js), which this diff correctly leaves alone.

The generator and the parity test each hardcoded MAX_DEPTH as 100. The real
value in src/constants.js is 300, and sites.js uses it for the reef
descriptor, so the generated resources were wrong: reef.maxDepth, both floor
abyss endpoints and the blue-water zone all landed at 100 instead of 300.

The parity test could not catch this. It injected the same literal into the
legacy source that its own reimplementation used, so both sides agreed at 100
and every assertion passed against data that does not match the running game.
Reverting reef to the old values now fails that test with 480 mismatches.

Both sites read the constant from constants.js and fail loudly if the
declaration moves. It cannot be evaluated into the vm sandbox — constants.js
declares MAX_DEPTH with const, which never becomes a property of the context
object — so the value is extracted from the source text.

Also adds `npm run sites:check`, wired into both workflows, so an edit to
sites.js that skips regeneration fails CI rather than leaving the resources
quietly stale. It compares only the `sites` payload; sourceCommit moves with
every commit and would otherwise fail on every run.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@N1k4G

N1k4G commented Aug 21, 2026

Copy link
Copy Markdown
Owner Author

Pushed a fix: MAX_DEPTH was hardcoded to 100, the real value is 300

Found while reviewing for merge-readiness. The generated resources carried wrong numbers, and the parity test was structurally unable to notice.

What was wrong

scripts/generate-site-resources.mjs:48 and tests/parity/site-geometry.test.ts:24 each hardcoded MAX_DEPTH = 100. src/constants.js:144 declares it as 300, and src/sites.js uses it in the reef descriptor — so the extraction produced:

Value Runtime Generated
reef.maxDepth 300 100
reef.floor endpoints (x = ±26, ±200) 300 100
reef_blue_water zone d2 300 100

Verified against the running game rather than inferred — DIVE_SITES.reef.maxDepth is 300 in the browser.

Why the parity test passed anyway

It injected the same literal into the legacy source that its own reimplementation used:

const MAX_DEPTH = 100;
const legacy = new Function("MAX_DEPTH", `…${legacySource}…`)(MAX_DEPTH);
function floorAt(site, x) { return Math.min(MAX_DEPTH, lerp(site.floor, x) ?? MAX_DEPTH); }

Both sides agreed at 100, so every assertion passed against data that does not match the running game. This is the one class of bug the suite exists to catch, so it seemed worth fixing properly rather than just correcting the number.

The fix

Both sites now read the constant from constants.js and throw if the declaration moves or becomes computed.

Worth noting for anyone touching this later: it cannot simply be evaluated into the vm sandbox. constants.js declares MAX_DEPTH with const, which never becomes a property of the context object — sandbox.MAX_DEPTH comes back undefined. I confirmed that before picking source-text extraction.

Also added: npm run sites:check

There was no guard that the committed resources match a fresh regeneration. Added a --check mode to the generator, wired into pr.yml and deploy.yml, so editing sites.js without regenerating fails CI instead of leaving the resources quietly stale.

It compares only the sites payload — sourceCommit moves with every commit, so a plain git diff --exit-code would have failed on every run.

Verification

Checked both directions, since a guard that only passes proves nothing:

Check Result
sites:check on corrected data passes
sites:check with sites.js perturbed (mast dBottom 18→19) fails, exit 1
test:parity on corrected data 28 passed
test:parity with reef reverted to the old values fails, 480 mismatches

That last row is the point — it's the exact data that passed before this commit.

Full suite green locally: typecheck, lint, build, test:unit (67), test:parity (28), test:e2e (22).

Left alone

Object.freeze(sites) at site-resources.ts:108 is shallow — nested structures / floor arrays stay mutable. Real but minor, and a deep freeze risks breaking something in an otherwise-green PR, so I didn't widen the diff for it. Worth a follow-up.

The sampling grid in the parity test is fine as-is — I initially suspected it was too narrow, but x = 26 is inside the sampled range and carries a MAX_DEPTH floor point, so it already exercises the corrected values. The shared constant was the whole problem.

Two defects in the new site modules, both found while reviewing for merge.

Culling compared the camera window against `featureDepth()`, which is a
feature's anchor — for a span it returns `dTop`. The cave columns span
dTop=56..dBottom=98 and dTop=60..dBottom=95, so a diver swimming toward the
cathedral floor left the anchor's window while the columns still filled the
screen: at depth 90 one of the two disappeared, at 95 both did. Culling now
does an overlap test against the whole extent via `featureDepthRange()`.
Anchoring is unchanged; it was only ever culling that needed the span.

`Object.freeze` is shallow, so freezing the site documents left every nested
`structures`, `floor` and `features` array writable. The immutability both
resource modules advertise was nominal — a consumer holding a reference could
have mutated collision geometry in place for everyone else. `deepFreeze` is
its own module because the gameplay and presentation resources must not import
each other.

Both are covered by tests that fail without the fix: the span tests read 1 and
0 columns against the expected 2, and the immutability tests see unfrozen
nested arrays.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@N1k4G

N1k4G commented Aug 21, 2026

Copy link
Copy Markdown
Owner Author

Two more fixes pushed

Found on a closer read of layer-factory.ts and site-resources.ts.

1. Tall features were culled by their anchor, not their extent

buildSceneLayers compared the camera window against featureDepth(), which returns the anchord for a point, dTop for a span. The cave columns are spans:

caveColumn x=88  dTop=56 dBottom=98   (42 m tall)
caveColumn x=112 dTop=60 dBottom=95   (35 m tall)

With CULL_HALF_HEIGHT_M = 20 and the default 10 m margin, a diver swimming toward the cathedral floor leaves the anchor's window long before leaving the column's. Measured against the shipped data:

Diver depth Columns rendered (before) Expected
75 2 2
85 2 2
90 1 2
95 0 2

At 95 m both 40 m columns had vanished while the diver was alongside them — they extend to 98 and 95.

Culling is now an overlap test against the full extent via a new featureDepthRange(). Anchoring is untouched; only culling ever needed the span. The helper also normalises an inverted dTop/dBottom rather than letting it silently disable a feature.

Latent today, since only wreck is wired to Pixi and its features are all points — but this is renderer-neutral library code and the cave is the obvious next scene.

2. Object.freeze on the resources was shallow

Both SITE_GAMEPLAY and SITE_PRESENTATION were frozen at the top level only, leaving every nested structures, floor and features array writable. The immutability both modules advertise was nominal — a consumer holding a reference could have mutated collision geometry in place and every other consumer would have seen it.

Now deep-frozen. deepFreeze lives in its own module because the gameplay and presentation resources must not import each other, and neither should have to restate it.

Verification

Both fixes have tests that fail without them — I checked the negative direction rather than just that the suite is green:

Test Without the fix
keeps a tall feature while any part of it is on screen fails: 1 and 0 columns vs expected 2
refuses in-place mutation of nested gameplay geometry fails: nested array not frozen
refuses in-place mutation of nested presentation features fails: nested array not frozen

Full suite green locally: typecheck, lint, build, sites:check, test:unit (72), test:parity (28), test:e2e (22).

One thing I did not change

Horizontal culling still uses feature.x alone, ignoring wTop/wBot on the cave columns (9–11 m wide). With a 34 m half-window plus 10 m margin, a 5.5 m half-width is far inside the margin, so it cannot cause the same popping. Inventing a general width convention from two kind-specific fields felt like the wrong call for this PR — flagging it rather than doing it.

Unrelated red check

Terraform Cloud/Infrastruktur flipped to failure on my first commit. This PR changes no terraform files (git diff main...HEAD -- terraform/ is empty), the same check passes on main, and nothing in terraform/ references anything I touched — so it looks transient or provider-drift related rather than caused here. Possibly connected to the pending Cloudflare v5 migration in #23. Worth a re-run.

…t order

Addresses four review findings on WP-07.

[P1] The committed JSON Schema had no caller, and presentation data had no
schema at all. validateSiteGameplay() checks geometry relationships, not
types, so a descriptor typo producing hasOverhead: "false" passed generation,
parity, typecheck and CI while changing behaviour — the string is truthy, so
an overhead site silently becomes one the diver can surface from. Adds a
presentation schema and a suite that compiles both with Ajv and asserts they
reject stringly-typed values, negative depths, unknown fields and bad
provenance. This is what the "invalid site data fails at build/test time"
criterion needed.

[P2] sourceCommit was false provenance and could not be otherwise: generation
stamps `git rev-parse HEAD`, which always names the commit before the one
carrying the generated file. The shipped resources claimed 4046bda, whose
sites.js produces the pre-fix MAX_DEPTH values. Replaced with sourceDigest, a
sha256 over the actual inputs (sites.js plus the MAX_DEPTH it reads), which is
a claim that can be true — and sites:check now verifies it instead of skipping
it, so stale provenance fails CI like stale data does.

[P3] The asset-replacement acceptance test built a `swapped` entry and never
passed it anywhere; buildSceneLayers() resolved the unmodified manifest, so
the test passed regardless of whether replacement worked. Rewritten in its own
file with the manifest module mocked, so the swap actually reaches the layer
factory, plus a guard that fails if the mock stops taking effect.

[P4] The layer refactor put silt in `terrain`, below `structure`, where it had
previously been drawn after the hull and route. Silt occupies y=34.2..35.4 and
the hull's opaque fill covers down to y=35 across x=14..103, hiding 31 of its
48 particles. Moved to `foreground` between route and diver, restoring the
pre-refactor draw order exactly.

Each fix is verified by reintroducing the bug: the schema suite fails on an
injected string, sites:check fails on a stale digest, the replacement test
fails when placements stop reading the resolved asset.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@N1k4G

N1k4G commented Aug 21, 2026

Copy link
Copy Markdown
Owner Author

All four review findings confirmed and fixed — 38b26b7

I reproduced each against the source before changing anything. All four hold; two of them were mine.


[P1] The JSON Schema is never executed — confirmed

site.schema.json had no caller. Ajv is used in tests/baseline.spec.js, but only for trace.schema.json and performance.schema.json. Presentation data had no schema at all.

The consequence is exactly as described: validateSiteGameplay() checks geometry relationships — profile ordering, inverted boxes, structures below maxDepth — and says nothing about types. hasOverhead: "false" is a truthy string, so an overhead site would silently become one the diver can surface from, and it would pass generation, parity, typecheck and CI.

Fixed by adding site-presentation.schema.json and tests/unit/site-schema.test.ts, which compiles both schemas with Ajv and validates the shipped documents. Six negative cases pin the classes of error relationship checks cannot see: stringly-typed hasOverhead and maxDepth, a negative structure depth, an unknown top-level site field, a non-numeric feature x, and malformed provenance.


[P2] sourceCommit contains false provenance — confirmed

Verified concretely: the resources claimed 4046bda, and git show 4046bda:src/sites/resources/gameplay.json has reef.maxDepth = 100 where HEAD has 300.

Worth stating plainly — this is not just a stale value, it is unfixable as designed. Generation stamps git rev-parse HEAD, which by construction names the commit before the one carrying the generated file. A commit id there is always wrong. My excluding it from the freshness check papered over that rather than addressing it.

Fixed by replacing it with sourceDigest: a sha256 over the actual inputs — src/sites.js plus the MAX_DEPTH it reads from constants.js. That is a claim that can be true, and sites:check now verifies it rather than skipping it:

site resources are out of date:
  gameplay.json claims sourceDigest sha256:0000…, inputs hash to sha256:9c6a4125…
Run `npm run sites:generate` and commit the result.

SITE_GAMEPLAY_SOURCE_COMMIT is renamed to SITE_GAMEPLAY_SOURCE_DIGEST (no other consumers), and both schemas now require the digest format.


[P3] The asset-replacement acceptance test is ineffective — confirmed

swapped was built and then used only in expect(swapped.atlas).not.toBe(original?.atlas) — comparing a literal against itself. buildSceneLayers() resolves ASSET_MANIFEST directly, so the test could not fail if replacement broke.

Fixed in tests/unit/asset-replacement.test.ts, in its own file because vi.mock is hoisted module-wide and the other suites need the real table. The manifest module is mocked so assetFor("tableCoral") returns a re-pointed entry — which is what an art swap actually is — and the test asserts placements carry the new atlas and frame, gameplay stays byte-identical, collision geometry stays valid, and neighbouring kinds are undisturbed. A first assertion guards the mock itself, so it cannot go vacuous the way the original did.

The weaker "building a scene touches no gameplay data" property stays in site-assets.test.ts, honestly relabelled.


[P4] The layer refactor moves silt behind the opaque hull — confirmed

Before: distantHull, seabed, hull, rooms, engine, route, silt, diver — silt drawn last but one. After: terrain: [seabed, silt], below structure: [hull, rooms, engine].

Quantified against the actual geometry. Silt occupies y = 34.2…35.4; the hull's outer polygon [14,35, 22,23, 82,21, 108,29, 103,35] has an opaque fill (0x33484a, no alpha) whose bottom edge runs along y = 35 from x = 14 to x = 103. 31 of the 48 silt particles (65%) fall inside it.

Fixed by moving silt to foreground between route and diver, restoring the pre-refactor order exactly. Silt is suspended particulate hanging between camera and wreck, not ground cover, so foreground is also where it belongs semantically.


Verification

Every fix was checked by reintroducing the bug, not just by the suite going green:

Reintroduced bug Result
hasOverhead: "false" injected into gameplay.json schema suite fails
sourceDigest replaced with a stale value sites:check exits 1
placements stop reading the resolved asset replacement test fails
silt returned to terrain 31/48 particles inside the hull polygon

Green locally: typecheck, lint, build, sites:check, 84 unit (was 72), 28 parity, 22 e2e.

Not addressed

The screenshot gate still would not have caught P4, as you noted — it only covers the legacy client. That is the PR's own declared gap, and extending pixel comparison to the Pixi client is follow-up work before WP-08 rather than something to bolt on here.

The digest I added in 38b26b7 hashed raw file bytes, which made it depend on
the checkout platform. With core.autocrlf a Windows working tree holds CRLF
where Linux CI holds LF — identical content, different bytes — so the value
generated on Windows (9c6a4125…) disagreed with the one CI computed
(58e238f5…) and sites:check failed the build for a machine difference rather
than a data one.

Normalise CRLF to LF before hashing. The generated resources never depended on
line endings, so their provenance must not either. The regenerated digest now
equals the value CI computed, and hashing all-CRLF and all-LF copies of
sites.js produces the same result.

Only the digest line changes; the sites payload is untouched.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@N1k4G

N1k4G commented Aug 21, 2026

Copy link
Copy Markdown
Owner Author

CI caught a flaw in my own fix — corrected in d69a88b

verify failed on 38b26b7 at the sites:check step. The failure was mine, and the guard working as intended.

I hashed raw file bytes for sourceDigest. With core.autocrlf, a Windows working tree holds CRLF where Linux CI holds LF — identical content, different bytes:

generated on Windows : sha256:9c6a4125d63037fb…
computed by CI       : sha256:58e238f55504ff06…

So the check failed for a machine difference rather than a data one. The payload comparison passed throughout, which localised it to the digest.

Fixed by normalising CRLF to LF before hashing. The generated resources never depended on line endings, so their provenance must not either.

Verified rather than assumed:

  • the regenerated digest is sha256:58e238f5… — byte-identical to what CI computed
  • hashing an all-CRLF and an all-LF copy of sites.js now produces the same digest
  • only the digest line changed; the sites payload is untouched

Local: typecheck, lint, build, sites:check, 84 unit, 28 parity, 22 e2e — all green.

Worth noting the failure mode this would have had if I'd used a commit id as originally written: it would have "passed" everywhere while remaining false. The digest failing loudly on a platform difference is the less comfortable but more useful behaviour.

Two schema gaps from review.

[P1] `entry` was declared as a bare `{ "type": "object" }`, was not required,
and was missing from SiteGameplay entirely — so `entry.x = "not-a-number"`
passed Ajv, typecheck and CI. Parity would not catch it either: the suite
compares floor, ceiling, solid, overhead and bad air, never entry points. Now
a closed `{ x: number }`, required on every site, and exposed on the
TypeScript contract as SiteEntryPoint.

[P2] The decoration rule was left open and declared 7 of the 14 fields
renderer.js actually reads. This matters more than an absent field would,
because every guard is shaped `rule.f != null && x < rule.f`: a string makes
the comparison NaN-false, so the guard stops guarding and props render at
every depth. The wreck already authors minDepth: 20, which is exactly that
path. Declared minDepth, maxDepth, minScale, maxScale, maxPerScreen,
rotationJitter and alpha with numeric constraints, and closed the rule, its
props entries, visual zones and atmosphere profiles against unknown fields so
a typo'd name fails validation instead of being read as undefined and
defaulted away.

Verified by re-running the probe that demonstrated both gaps: every case that
previously passed Ajv now fails, and the shipped documents still validate.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@N1k4G

N1k4G commented Aug 21, 2026

Copy link
Copy Markdown
Owner Author

Both confirmed and fixed — e20c058

Reproduced each before changing anything. Both hold, and P2 turned out to be broader than the field you named.

[P1] entry is unvalidated and missing from the contract — confirmed

entry.x = "not-a-number" passes gameplay schema : true
entry removed entirely passes                   : true
entry = {totallyBogus:true} passes              : true

SiteGameplay omits it, so TypeScript offers nothing either, and the parity suite compares floor, ceiling, solid, overhead and bad air — never entry points. Nothing in the pipeline would catch it.

Fixed: closed { x: number } via an entryPoint def, entry added to the site required list (all four sites author it), and SiteEntryPoint exposed on the TypeScript contract.

One correction to the framing, for the record rather than as a disagreement: entry currently has no consumer — no read of it anywhere in src/*.js. So the impact today is latent rather than active. The finding is still right on substance: it is gameplay data, in the gameplay document, unvalidated and unexposed, and the moment WP-08 wires spawning to it a string coordinate becomes a real defect. Pinning it now is cheaper than pinning it then.

[P2] The decoration rule omits renderer-consumed fields — confirmed, and it is 7 fields, not 1

minDepth is the one the shipped data authors (wreck_exterior_scraps, minDepth: 20), but enumerating what renderer.js actually reads off a rule gives 14 fields; my schema declared 7:

declared : id zone spacing density seed surface props
missing  : minDepth maxDepth minScale maxScale maxPerScreen rotationJitter alpha

Your NaN analysis is exactly right, and I verified the runtime consequence against renderer.js:2549:

minDepth = "not-a-number"
  visualD=5  : guard skips decoration? false   (numeric 20 would give true)
  visualD=20 : guard skips decoration? false
  visualD=60 : guard skips decoration? false

Every guard there is shaped rule.f != null && x < rule.f, so a string is worse than an absent value: != null passes, the comparison is NaN-false, and the guard silently stops guarding. That shape repeats for maxDepth, and the same class applies to the scale and alpha fields.

Fixed: all seven declared with numeric constraints (maxPerScreen as an integer, alpha bounded 0–1), and additionalProperties: false on the rule, its props entries, visual zones and atmosphere profiles. Closing matters as much as declaring — a typo'd minDepht would otherwise validate and then be read as undefined and defaulted away, which is how a guard goes missing with nothing to show for it.

I declared the fields the renderer reads rather than only those the data authors today, so that authoring maxScale tomorrow does not fail validation for being legitimate.

Verification

Re-ran the exact probe that demonstrated both gaps. Every case that previously passed Ajv now fails:

Case Before After
entry.x = "not-a-number" passes fails
entry removed passes fails
entry = {totallyBogus:true} passes fails
minDepth = "not-a-number" passes fails
unknown field on a rule passes fails

Shipped documents still validate. Schema tests 8 → 24; unit total 100 (was 84). typecheck, lint, build, sites:check, 28 parity, 22 e2e all green locally.

Follows a full read of pixi-renderer.ts, which I had only skimmed before.

The layer refactor said draw order was "a declared property of the scene rather
than an accident of call order", but it was a sequence of addChild calls that
nothing could read and nothing could assert — which is how silt ended up below
the hull. RETAINED_LAYER_ASSIGNMENT moves it into data, and site-layers.test.ts
asserts the invariants: things between the camera and the wreck are painted
after it, the wreck is painted after the seabed it rests on, and the layer list
runs furthest to nearest. Moving silt back to `terrain` fails two of them.

Also fixes a latent ordering defect found in the same pass. buildSceneLayers
sorts placements within a layer so the same data always yields the same scene,
but the renderer skipped the re-add for a pooled marker reused in the container
it already occupied. Pixi's addChild splices an existing child out and pushes it
to the end, so that guard was skipping the reorder: the sort stopped being
reflected in the display list after the first resync. Invisible while every
marker is the same provisional circle, and would surface as soon as real atlas
frames overlap.

Unit tests 100 -> 114.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@N1k4G

N1k4G commented Aug 21, 2026

Copy link
Copy Markdown
Owner Author

Deep review of pixi-renderer.ts, structural layer guard, props decision — 509f5e6

Props: resolved, no change to this PR

Decision is props stay decoration. #123 closed as a documented limitation, so the question I raised at the top of this PR is settled and nothing here needs to change.

I corrected an overstatement of mine on that issue: reversing it later is an ordinary edit plus a regenerate, not a migration. The real cost is that position data would live in two places — move a car, forget its hitbox. If props ever get collision, the thing to build is a feature-declared hitbox the generator lifts into gameplay, not hand-paired AABBs.

Structural layer guard (your P4 coverage point)

P4 was a real regression that lint, typecheck, unit, parity and e2e all passed. The gap was that draw order — described in the code as "a declared property of the scene rather than an accident of call order" — was still a sequence of addChild calls that nothing could read and nothing could assert.

RETAINED_LAYER_ASSIGNMENT in src/render/layer-assignment.ts makes it data; tests/unit/site-layers.test.ts asserts the invariants:

  • things between the camera and the wreck (silt, route, diver, bubbles) are painted after the opaque hull
  • the wreck is painted after the seabed it rests on, and both after the parallax silhouette
  • the layer list runs furthest-to-nearest, and every asset and retained element maps to a real layer

Verified by reintroducing P4 — moving silt back to terrain fails two assertions. Milliseconds, no GPU. It cannot see that a scene looks right, but it catches the class of mistake that actually happened.

One real defect from the deep read

buildSceneLayers sorts placements within a layer — shallowest first, then x — "so the same data always produces the same scene regardless of authoring order". The renderer was discarding that sort:

if (marker.parent !== container) {
  container.addChild(marker);
}

Pixi v8's addChild on a child that is already in the container splices it out and pushes it to the end — that call is the reorder. Guarding it means a pooled marker reused in the container it already occupies keeps its stale child index, so after the first resync the display list no longer reflects the sort. Now unconditional.

Invisible today because every marker is the same provisional circle. It would surface the moment real atlas frames overlap — i.e. exactly when the manifest starts doing its job.

Two things I did not change

setQualityTier() and placementCount have no callers. Neither is on the SceneRenderer interface, neither is referenced anywhere in src or tests. tierAllows is tested as a pure function, but the renderer's tier path never executes, so quality tiers ship unexercised. That is a scope question rather than a bug, so I have left it — flagging it because the PR lists quality tiers as a deliverable.

Pooling is otherwise correct. Markers are recycled without double-listing, the pool is bounded, and destroy() releases cleanly. Recycled-but-unused markers stay as hidden children of their old container, which is harmless.

Verification

typecheck, lint, build, sites:check, 114 unit (was 100), 28 parity, 22 e2e — all green locally.

Handing back to you for review and merge, as agreed. I have written a large share of this diff, so an approval from me would not mean much.

…erences

[P1] `ceiling` was absent from the schema's required list although
SiteGameplay requires it and every site declares it. Ajv accepted the cave
with the field deleted, which drops ceilingAt() through to 0 — the cave roof
goes from 14 m to 0 m at x=50 and the diver swims up through it. Parity cannot
see this: legacy and generated data lose the field together and agree on 0.
Now required, with null spelled out for open water.

[P2] Culling used a fixed 20 m half-height. The camera fits a constant 58 m of
WIDTH, so visible height is viewport.height / scale and scales with aspect
ratio: at 390x844 the screen shows ~125 m of depth against a 30 m cull window
including margin. Four on-screen placements were dropped at any camera
position — the engine row at d=61 and the anchor at d=66 — while the diver was
looking at them. Desktop and landscape hid it because their windows are
shorter than the constant. The window is now derived from the camera, and
resize invalidates the sync so an orientation change re-populates instead of
waiting for 4 m of travel.

[P3] The schema could say `rule.zone` is a string but not that the zone
exists. A misspelled reference matched no candidate, so an entire scatter of
props silently never rendered. validateSitePresentation() checks zone
references, atmosphere-profile keys (also zone ids), duplicate zone and rule
ids, and inverted zone extents, and throws at import time like the gameplay
resource does.

The first version of the P2 test passed with the bug reintroduced: it computed
"on screen" from the same half-extents it culled with, so the two agreed with
each other. The cull window now comes from a shared helper while the visible
rectangle is computed from viewport arithmetic, and pinning the helper back to
the constants fails four assertions instead of one.

Unit tests 114 -> 133.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@N1k4G

N1k4G commented Aug 23, 2026

Copy link
Copy Markdown
Owner Author

All three confirmed and fixed — 9e807aa

Reproduced each before changing anything.

[P1] ceiling optional — confirmed

cave with `ceiling` deleted passes Ajv : true
cave ceilingAt(30) : 15.0 m  ->  0.0 m
cave ceilingAt(50) : 14.0 m  ->  0.0 m
cave ceilingAt(88) : 12.5 m  ->  0.0 m

The whole roof, not one point. And your note on parity is the important half: legacy ceilingAt returns 0 for a missing profile and the test's reimplementation returns 0 for an empty one, so both sides lose the field together and agree.

Fixed: ceiling added to required, with a description making explicit that null must be spelled out rather than omitted.

[P2] Fixed vertical culling — confirmed, and it is every portrait size

The camera fits a constant 58 m of width, so visible height is viewport.height / scale:

Viewport Visible depth Culled to (incl. margin)
390x844 -37.8…87.8 m -5…55 m
320x568 -26.5…76.5 m -5…55 m
1280x800 6.9…43.1 m -5…55 m
844x390 11.6…38.4 m -5…55 m

Your three examples land at focus.x=20. Sampling across the wreck, it is 4 on-screen placements dropped at any camera position — the engine row at d=61 plus the anchor at d=66 near the bow, engines further aft elsewhere. Desktop and landscape hid it because their windows are shorter than the constant, which is presumably how 20 m looked reasonable.

Fixed both parts you named: the window is derived from the camera via a shared visibleHalfExtentM(), and resize() now clears #lastSyncFocus so an orientation change re-syncs instead of waiting for 4 m of travel. mount() also sizes before its first sync, which previously ran against the placeholder 1x1 viewport.

[P3] Cross-references unvalidated — confirmed

"typo-zone" passes Ajv; the zone does not exist. Shipped data is currently clean (no dangling refs, no duplicate ids, no inverted extents), so this guards future edits rather than fixing present ones.

Fixed with validateSitePresentation(), throwing at import time like the gameplay resource. It covers the three you listed plus one more: atmosphereProfiles is keyed by zone id — all 27 keys across the four sites resolve to zones — so an unresolved key is an atmosphere that silently never applies.

A correction worth flagging

My first version of the P2 test passed with the bug reintroduced. It computed "on screen" from the same half-extents it used for culling, so the two agreed with each other regardless of what either was. That is the same self-consistency failure as the original MAX_DEPTH bug — I caught it only because I ran the negative pass.

Rewritten so the cull window comes from the shared helper while the visible rectangle is computed from viewport arithmetic. Pinning the helper back to 34/20 now fails four assertions:

× covers everything on screen at 390x844
× covers everything on screen at 320x568
× never culls inside the visible rectangle, at any aspect ratio
× grows the window as a portrait viewport gets taller

Verification

Reintroduced Result
ceiling removed from required schema suite fails
cull window pinned to the old constants 4 layer assertions fail
rule pointing at typo-zone import throws, naming the rule and the zone

Unit tests 114 -> 133. typecheck, lint, build, sites:check, 28 parity, 22 e2e green locally.

presentation.json shows in the diff with zero changed lines — line-ending normalisation only. The digest hashes normalised endings, so it is unaffected.

@N1k4G
N1k4G merged commit a515e50 into main Aug 23, 2026
6 checks passed
@N1k4G
N1k4G deleted the site/wp-07-asset-pipeline branch August 23, 2026 10:47
N1k4G added a commit that referenced this pull request Aug 24, 2026
Closes #127. Required before WP-08 starts moving real scenes.

compare-rendering.mjs gates the legacy canvas client only, so nothing watched
the Pixi one. The worked example is the silt regression in #119: silt assigned
to the `terrain` layer, below `structure`, put 31 of its 48 particles behind the
hull's opaque fill and passed lint, typecheck, unit, parity and e2e. It was
found by reading the diff.

Reference frames AND scene statistics, because neither is sufficient. Verified
rather than assumed: reintroducing the silt regression fails all three scenes on
frame delta (max channel delta 35 against a budget of 24) while no statistic
breaches at all. Silt behind the hull and silt drawn correctly produce nearly
identical frame-wide numbers, which is exactly the case statistics cannot see.

Committed PNG references are viable here because Playwright's headless Chromium
rasterises through SwiftShader — software, not the GPU — so captures are
byte-identical across runs and across browser processes. Measured before
building anything: 0 differing bytes. Whether that holds across operating
systems is what CI will now tell us; if it does not, the guard says so rather
than quietly passing.

Determinism needed more than seeding Math.random. GameController.#tick takes the
requestAnimationFrame timestamp and advances both the diver's position and
`elapsedRealS`, which drives the bubbles, by however long the frame really took
— so waiting a fixed number of milliseconds does not reproduce a scene. The
first version of this guard failed against its own freshly recorded references
with a max channel delta of 128. rAF is now replaced by a queue the harness
steps explicitly, with a virtual clock advancing exactly one 60 Hz frame per
step, and scenes are driven in frames rather than milliseconds.

Two things worth knowing for anyone extending this:

- Statistics are computed from the decoded screenshot, not by reading the live
  canvas back. Drawing a WebGL canvas into a 2D one returns transparent black
  without preserveDrawingBuffer, and the first version recorded meanLuma 0 for
  every scene — zero compares equal to zero, so that half of the guard would
  have passed whatever the renderer did. There is now an explicit check that
  refuses to record a uniformly black frame.
- The capture pins locale to en-US. The boundary copy is localised and the
  button is matched by name, so without it the run fails on a German desktop
  and passes in CI.

References live in tests/fixtures/reference-frames/pixi/ and are re-recorded
deliberately with `npm run pixi:visual-update`, so an intended change lands in
the diff as new frames for review rather than as a raised threshold.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
N1k4G added a commit that referenced this pull request Aug 24, 2026
Closes #127. Required before WP-08 starts moving real scenes.

compare-rendering.mjs gates the legacy canvas client only, so nothing watched
the Pixi one. The worked example is the silt regression in #119: silt assigned
to the `terrain` layer, below `structure`, put 31 of its 48 particles behind
the hull's opaque fill and passed lint, typecheck, unit, parity and e2e. It was
found by reading the diff.

Reference frames AND scene statistics, because neither is sufficient. Measured
rather than assumed: reintroducing the silt regression fails all three scenes
on frame delta (max channel delta 35 against a budget of 24) while no statistic
breaches at all. Silt behind the hull and silt drawn correctly produce nearly
identical frame-wide numbers.

References are keyed by platform. The first version assumed SwiftShader made
captures portable, since it is a software rasteriser and captures were
byte-identical across runs and browser processes on one machine. CI disproved
that: win32-recorded frames differed from linux by a max channel delta of 230
across 10.6% of pixels. Loosening the budget to absorb that would take it past
the silt regression's delta of 35 — a guard reporting green while the defect it
exists for ships — so each environment compares against frames recorded where
it runs. The linux set was recorded by CI itself.

Determinism needed more than seeding Math.random. GameController.#tick takes
the requestAnimationFrame timestamp and advances both the diver's position and
elapsedRealS, which drives the bubbles, by however long the frame really took,
so waiting a fixed number of milliseconds does not reproduce a scene. The first
version failed against its own freshly recorded references at a delta of 128.
rAF is now a queue the harness steps explicitly on a virtual 60 Hz clock, and
scenes are driven in frames.

Four silent-failure traps found and closed while building it:

- Statistics read back from the WebGL canvas returned transparent black
  without preserveDrawingBuffer, recording meanLuma 0 for every scene. Zero
  compares equal to zero, so that half of the guard would have passed whatever
  the renderer did. Statistics now come from the decoded screenshot, and a
  uniformly black frame is refused outright.
- A scene named `wreck-torch-on` captured the torch OFF, because the controller
  starts with it on and pressing `t` disabled it. Scenes now declare the state
  they exercise and assert it before capturing; a mislabelled one fails instead
  of recording cleanly forever.
- Both commands consumed dist/ without building it, so a check could report 0%
  against an older build and an update could record references from stale code.
  Both have build prehooks now.
- A breach writes <scene>.actual.png and tells the reviewer to look at it, but
  CI discarded it with the runner. Both workflows upload it on failure, which
  matters because platform-keyed pixels cannot be reproduced on another OS.

Locale is pinned to en-US: the boundary copy is localised and the button is
matched by name, so without it the run fails on a German desktop and passes in
CI.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release:minor Minor production release / user-visible feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant