Give the Vulcanus cliff-blocking tile set one definition per side (#364) - #368
Merged
Merged
Conversation
The set of Vulcanus tiles that refuse a cliff - `lava` and `lava-hot` - was
written out four times, and nothing asserted the four agreed. Two of them sat
on adjacent lines in `fixtures.rs` and said the same thing in two vocabularies,
one by enum and one by string literal, so they could drift apart without either
file changing.
| before | after |
| --- | --- |
| `cliffs/vulcanus_fields.rs` inlined `Lava \| LavaHot` | calls `tile.is_cliff_blocking()` |
| `fixtures.rs` `is_lava`, the same match again | `VulcanusTile::is_cliff_blocking` |
| `fixtures.rs` `want_lava`, by name | resolves through `VulcanusTile::from_name`, then asks the same predicate |
| `cliffCatalog.ts` | unchanged, still the only TypeScript definition |
`from_name` is the inverse of the existing `name()`, resolved through
`TILE_ORDER` so the two cannot disagree. It refuses a name Vulcanus does not
place, which turns a typo in a fixture into a failure rather than a silent
`false`.
## The two sides are compared now, not just tidied
`fmw-wasm` exports `vulcanus_cliff_blocking_names_fnv1a64()`, an FNV-1a 64 over
the blocking tiles' names, sorted and joined by newlines.
`test/wasmVulcanusParity.spec.ts` hashes `VULCANUS_CLIFF_BLOCKING_TILES`
through the module's own `fnv1a64` and compares.
Names rather than a count, because a count cannot tell `lava` from
`volcanic-folds` and a swap is the drift worth catching. The module's own hash
rather than a second FNV-1a in TypeScript, because a reimplementation is one
more thing that can disagree and its disagreement would look identical to real
drift. Sorted, because catalog order is ground truth for the argmax tie-break
but a `Set` of two strings does not carry it, so hashing in catalog order would
make this depend on a fact it is not trying to check.
## Planted, not predicted
Adding `VolcanicFolds` to the Rust set and rebuilding the module turns exactly
the new parity test red and leaves the other eleven in that file green. A
second test is the standing control: it hashes the set with a tile added, with
one removed and with one swapped, and asserts all three differ from the
module's answer. Without it, a bug that made both sides hash nothing would
leave the comparison green while comparing nothing.
## The module grew 1,289 bytes, measured rather than assumed
320,092 to 321,381. Stubbing the export to return 0 unconditionally and
rebuilding gives 320,135, so the export symbol and its body cost 43 bytes and
the name table, the sort and the hash cost 1,246. The cost is `name()`'s
19-arm match pulling every tile name string into the module. Nothing
unaccounted for got linked in, and the 512 KB tripwire in `wasmEngine.spec.ts`
did not fire.
## Two more sites, deliberately left alone
Sweeping for `Lava | LavaHot` rather than trusting the issue's table found two
the issue did not list: `rocks/vulcanus_placement.rs` and
`resources/vulcanus_geyser.rs`. Both refuse the same two tiles, and both keep
their own definition.
The three sets are equal by coincidence, not by a shared rule. The geyser's own
doc comment already says so - "the forbidden set coincides with the rock
overlay's while being reached by a completely different route" - and the three
routes are the cliff's collision mask holding `water_tile`, the rock
prototypes' `vulcanus_tiles_cold` / `vulcanus_tiles_hot` autoplace lists, and
`type = "resource"` defaulting to the mask `{resource = true}`. Change the tile
data on any one of those axes and the three come apart, and a shared predicate
would then be wrong in two places at once, silently. That warning now sits on
`is_cliff_blocking` itself.
Closes #364.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #364.
The set of Vulcanus tiles that refuse a cliff -
lavaandlava-hot- waswritten out four times, and nothing checked the four agreed. Two of the four sat
on adjacent lines in
fixtures.rsand said the same thing in differentvocabularies, one by enum and one by string literal, so they could drift apart
without either file changing.
Now there is one definition per side, and a test that the two sides agree.
What moved
cliffs/vulcanus_fields.rsinlinedLava | LavaHottile.is_cliff_blocking()fixtures.rsis_lava, the same match againVulcanusTile::is_cliff_blockingfixtures.rswant_lava, by name:name == "lava" || ...VulcanusTile::from_name, then asks the same predicatecliffCatalog.tsfrom_nameis the inverse of the existingname(), resolved throughTILE_ORDERso the two cannot disagree. It refuses a name Vulcanus does notplace, which is what turns a typo in a fixture into a failure rather than a
silent
false.The two sides are now compared, not just tidied
fmw-wasmexportsvulcanus_cliff_blocking_names_fnv1a64(), an FNV-1a 64 overthe blocking tiles' names, sorted and joined by newlines.
test/wasmVulcanusParity.spec.tshashesVULCANUS_CLIFF_BLOCKING_TILESthroughthe module's own
fnv1a64and compares.Three choices there are deliberate:
lavafromvolcanic-folds, anda swap is exactly the drift worth catching.
fnv1a64, not a second one in TypeScript. Areimplementation is one more thing that can disagree, and its disagreement
would look identical to real drift.
Setof two strings does not carry it, so hashing in catalog order would makethis depend on a fact it is not trying to check.
It can fail, and that was watched rather than assumed
Adding
VolcanicFoldsto the Rust set and rebuilding the module turns exactlythe new parity test red and leaves the other 11 in that file green. A second
test in the spec is the standing control: it hashes the set with a tile added,
with one removed, and with one swapped, and asserts all three differ from the
module's answer. Without it, a bug that made both sides hash nothing would leave
the comparison green while comparing nothing.
The module grew 1,289 bytes, measured rather than assumed
320,092 to 321,381. Stubbing the new export to return 0 unconditionally and
rebuilding gives 320,135, so:
The cost is
name()'s 19-arm match pulling every tile name string into themodule. Nothing unaccounted for got linked in. The 512 KB tripwire in
wasmEngine.spec.tsis not close and did not fire.One thing found on the way, which is NOT folded in
Sweeping the repo for
Lava | LavaHotrather than trusting the issue's tableturned up two sites the issue did not list:
rocks/vulcanus_placement.rsandresources/vulcanus_geyser.rs. Both refuse the same two tiles.They are deliberately left alone, because the three sets are equal by
coincidence rather than by a shared rule. The geyser's own doc comment already
says so - "the forbidden set coincides with the rock overlay's while being
reached by a completely different route" - and the three routes are:
water_tile, andtile_collision_masks.lava()sets itvulcanus_tiles_cold/vulcanus_tiles_hotautoplace lists, whose union is every tile but thesetype = "resource"defaults to the mask{resource = true}, whichtile_collision_masks.lava()also listsChange the tile data on any one of those axes and the three come apart. A shared
predicate would then be wrong in two places at once, and silently. That warning
is now written on
is_cliff_blockingitself, where the next person to try themerge will read it.
Gate
pnpm run verifygreen locally. The four new Rust unit tests coverfrom_namein both directions, freeze the set at the lava pair, and check the exported hash
is computed the long way rather than compared against a magic digest.