Skip to content

fix(#768,#769): ground a static model ON its stored z, not a model height above it - #773

Merged
djhenry merged 3 commits into
mainfrom
fix-768-ground-lift
Jul 28, 2026
Merged

fix(#768,#769): ground a static model ON its stored z, not a model height above it#773
djhenry merged 3 commits into
mainfrom
fix-768-ground-lift

Conversation

@djhenry

@djhenry djhenry commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Closes #768
Closes #769

Both issues came out of #761's review of static_placement, so they are fixed together in the one function.

Round 2 (2026-07-28). The round-1 reviewer found that the "other half" pin shipped in 3305320 covered one of two channels, and requested no code change. This body has been rewritten where it was wrong — the corrections are marked [R2] — and every figure below is re-measured on the pushed head 01e96a3.

#768 — the grounded arm lifted a model a full model height above its stored z

entity_model_matrix_heading lifts by visual_scale * 0.5 + y_bottom * mesh_scale (camera.rs:85). The grounded arm passed visual_scale = 2 * y_extent * mesh_scale, so the real lift was (y_extent + y_bottom) * mesh_scale. The lowest vertex sits at local y = -y_bottom, so the two y_bottom terms cancel and the model's bottom landed exactly y_extent * mesh_scale above the stored z. The correct lift is y_bottom * mesh_scale alone.

Measured, by mapping boat.glb's measured bounds through the production matrix (static_placemententity_model_matrix_heading), stored z 4, heading 0:

drawn origin z drawn lowest vertex z
grounded, before 17.927488 13.945171
grounded, after 7.982317 4.000000
floating (unchanged, #756) 4.000000 0.017683

Over-lift 9.945171 = y_extent * mesh_scale exactly (y_extent 9.945171, y_bottom 3.982317, archetype_scale("boat") 1.000000).

Fix shape. Rather than passing 0.0, the visual_scale field is removed from StaticPlacement and y_extent from static_placement's parameter list. The four pass.rs call sites pass a literal 0.0 for entity_model_matrix_heading's visual_scale.

[R2] What that does and does not buy. An earlier version of this section said the second lift term was "now inexpressible" and that there was "no y_extent in scope at the call sites to rebuild one from". Both were false. Removing the field constrains the struct, not the callers, and there are two ways back to the old lift, both measured:

  1. Out of the placement, into the matrix. model is in scope at all four call sites and GpuStaticModel::y_extent is public, so 2.0 * model.y_extent * p.mesh_scale can be handed to entity_model_matrix_heading.
  2. Into the placement. static_placement(archetype, model.y_bottom + model.y_extent, …) restores the identical (y_extent + y_bottom) * mesh_scale. Reproduced independently of the reviewer before anything was written down: the crate stayed GREEN at 215 passed / 0 failed / 11 ignored, 10 of 10 test result: lines ok., 3305320's own pin included — it read the matrix call, not the placement call.

01e96a3 renames that pin every_static_entity_matrix_in_pass_rs_passes_a_zero_visual_scaleevery_static_placement_in_pass_rs_is_written_exactly_as_reviewed and grades both channels. Channel 2 is pinned by requiring each of the four calls to match one of two reviewed argument lists exactly, because source text cannot bound an expression (model.y_bottom vs model.y_bottom + 0.0 vs a helper call are all "the second argument"). The consequence is deliberate and is the loud direction: renaming the model binding, or adding a fifth site with any other argument spelling, turns this RED on correct code and asks for a review.

[R2] 3305320's commit message is retracted, not amended. Its subject and body call the term "Closed with a device-free source-text pin" and "the other half". No rebase or force-push is available here, so the retraction is the body of 01e96a3.

[R2] Why not make it unrepresentable. This repo's verification hierarchy prefers an impossible bad state over a documented one, so it was costed rather than waved off. The unrepresentable form is static_placement taking the model's measured bounds as an opaque newtype only the GLB loader can mint, so model.y_bottom + model.y_extent would not type-check. That is a cross-crate API change wider than #768: GpuStaticModel is a plain public-field struct constructed field-by-field outside the render path (src/bin/render_model.rs:853), so the newtype's constructor would have to be public-but-unforgeable, every construction site would have to change, and the device-free integration tests would need a way to mint one. Declined here, on scope. What is shipped instead is stated as what it is: a source-text pin over one file, with its holes listed.

No constant changed. The issue body suggested archetype_scale's constants might need rescaling. They do not: visual_scale is consumed at exactly two expressions in this crate (camera.rs:59, camera.rs:85), both visual_scale * 0.5 inside the translation — it is a Z offset at every use site and never reaches the scale matrix. Verified by git grep -n visual_scale crates/eqoxide-renderer.

Mutation check. Reverting the fix (restoring the visual_scale term) turns the behavioural tests RED: measured 9 passed; 2 failed in tests/floating_placement.rs. Under that same mutant the test #761 shipped for this arm, a_grounded_spawn_with_the_same_bounds_is_still_lifted, stays green — direct evidence that #768 was invisible to the existing suite, and the reason the new test grades the model's lowest vertex rather than the lift scalar or the drawn origin.

Both channel mutations were re-measured against the shipped pin:

mutation at the entity call site result
channel 1 — visual_scale = 2.0 * model.y_extent * p.mesh_scale crate --no-fail-fast: 214 passed / 1 failed / 11 ignored; the single failure is the pin
channel 2 — y_bottom = model.y_bottom + model.y_extent before the pin was extended: crate GREEN 215 / 0; after: 10 passed / 1 failed in floating_placement, printing the offending call

(The channel-1 run needs --no-fail-fast: without it cargo stops after the failing binary and only 2 of the 10 binaries report, which is no evidence about the other 8.)

The behavioural test sweeps headings [0, 90, 137, 180, 270, 359.5] because the lift and the horizontal recentre are applied on opposite sides of the heading yaw. It also asserts the matrix's third row before reading a bbox corner, so "the lowest AABB corner is a real vertex" is checked, not assumed.

#768 — blast radius, measured [R2, corrected mechanism]

No shipped pixel moves today — but the earlier version of this section gave the wrong reason. It said the static arm is taken by models with skins == 0. The real gate is renderer.rs:663-664: the skinned path is chosen when 0 < joint_count <= 128, so the static arm is !(0 < joint_count <= 128) — no skin, a skin with zero joints, or a skin with more than 128 joints. skins == 0 is sufficient, not necessary, and a scan that only reads len(skins) does not establish which arm a model takes.

Re-scanned reading len(skins[0].joints) as well. Loadable names = the 18 distinct archetypes race_to_archetype can return + the 29 race_* player models + the 3 <key>_f.glb female variants that exist (humanoid_f, elf_f, dwarf_f; preferred for gender == 1, renderer.rs:610) = 50 files, all present. Exactly one lands on the static arm: boat.glb, skins == 0, 0 joints. Nothing is above the cap. boat is selected only for race code "SHP", which eq_race_to_code returns exactly when is_boat_race(race_id), which sets Entity::is_boat, which makes Entity::floating() true (game_state.rs:192). So the grounded static arm has no live consumer for entity spawns today, and #768 has no live before/after to show.

The margin is one joint, on a directory this repo does not bake. race_pcfroglok.glb sits at 127 of 128; 11 rigs are at ≥ 109 (next highest 110). A two-joint rebake of that one file moves a PC race — for which floating() is false — onto the grounded arm. The arm being unreached is a fact about the current asset bake, not a property of the code, which is why it has to be correct rather than merely unreached.

#769 — the ALL_ARCHETYPES source parser's escape hatches

The parse is literally "arrow, space, double-quote". Added an arrow-count assert inside the existing test — every => in the parsed region must be the start of a => " arm (19/19 today).

Each of the four spellings was applied to models.rs and run individually, rather than reasoned about:

spelling cargo fmt normalizes with the new assert assert removed
"AIR"=>"airship" yes RED left: 20, right: 19 not measured
"AIR" =>"airship" yes RED left: 20, right: 19 not measured
"AIR" => { "airship" } no RED left: 20, right: 19 not measured
"AIR" => AIRSHIP (named const) no RED left: 20, right: 19 GREEN, 10 passed / 0 failed

That last row is the point of the issue: a named-const arm is cargo fmt-immune, silently drops an archetype out of the parsed set, and the property test then compares a short list against a short list and passes. Note the scope correction made during the work: only an added arm escapes silently; converting an existing arm to a non-literal spelling is caught loudly by the set-equality assert. And this repo runs no cargo fmt or clippy job in CI (.github/workflows/test.yml has no-local-detail and test), so fmt is a convention here, not a gate.

Sibling parser, as the issue asked. every_static_placement_call_site_in_pass_rs_decides_floating_explicitly matches static_placement( with the name and paren adjacent, so an added call site written static_placement (…) is invisible: measured GREEN at 10 passed / 0 failed with such a fifth call site present. An assert!(!PASS_RS.contains("static_placement (")) closes it (RED with the guard). A space added to an existing site was already caught by the calls.len() == 4 assert; only an added site escaped.

False statements in tracked files, corrected

  • models.rs:718's visual_scale = 2 * y_bottom * arch_scale * node_scale was never the code's formula. The whole 6-line block is deleted, not rewritten: it was orphaned above archetype_correction's doc, its node_scale=100 line is also stale, and its surviving true content duplicates archetype_scale's own in-body comment.
  • The gpu.rs:180 mis-citation (that line documents GpuSkinnedModel::y_bottom; the static path's is gpu.rs:141-142) — it appeared once in the repo, in the comment merged with fix(#756): exempt floating spawns from the grounding lift, via one shared static-placement fn #761, and now cites the static line.
  • Both y_extent field docs said "used to compute visual_scale". After this change that is true only in the standalone render_model viewer bin; both now say so and name the reading line.
  • [R2] Four artifacts that overstated the pin are corrected: the test's name, its doc block, StaticPlacement::y_bottom's doc, and — by retraction, since it cannot be amended — 3305320's commit body.
  • [R2] The skins == 0 mechanism above, in both models.rs and the floating_placement.rs module doc.

What I did NOT verify

  • No live end-to-end run. No client was launched. Everything above is source-traced, GLB-measured or unit-tested. Per the blast-radius finding there is no in-game observable to run: the grounded static arm is unreachable for entity spawns today.
  • [R2] The unrepresentable form was costed, not prototyped. I did not write the newtype and count the construction sites that would break; the scope claim above rests on reading render_model.rs:853 and the public-field definition of GpuStaticModel, not on a compile.
  • [R2] The joint-count scan is a snapshot of one machine's model directory, read from the GLB JSON chunks. I did not verify what the asset producer will bake next, and the 127/128 margin is exactly why that matters.
  • The standalone viewer is not converged. src/bin/render_model.rs still hand-writes the pre-renderer: the grounded static-model arm lifts the model a full model height above its stored z, not onto it #768 formula at :1097/:1266/:1271. It has no floating concept and took neither Floating spawns get the grounding lift — boats are drawn ~14u above the water on a 9.95u-tall hull #756 nor renderer: the grounded static-model arm lifts the model a full model height above its stored z, not onto it #768. I did not state how far off that puts a model on the turntable, because the viewer's arch_scale comes from a CLI-supplied archetype name and I did not run it. Converging it is not a pure deletion — visual_scale also feeds its camera distance, focus height and marker placement.
  • The y_bottom clamp residual is real and unfixed. y_bottom is -y_min only when y_min < 0, else 0.0. For a model whose vertices all sit above its local origin the corrected arm lifts by 0 and the bottom lands y_min * mesh_scale above the stored z. No shipped model exercises it (boat.glb's y_min is -3.982317), and y_bottom is shared with the skinned path, so I left the datum alone. Documented at the fix site.
  • [R2] Four residual holes in the pass.rs call-site parser are documented, not closed (three before, plus one the reviewer prompted): a call site in another file — "four call sites" is a count of call sites in pass.rs, not a bound on callers; a fifth producer of the floating flag, Scene::inject_test_billboards, which hardcodes floating: false (src/scene.rs:248) but is gated to zone == "testzone" (src/app.rs:1267-1269) and whose 16-race table (src/scene.rs:189-205) has no SHP entry, so no static-arm model reaches it by asset and zone facts, not by construction; a call whose argument list contains ); before its own close (no such call exists); and a trailing comma after false, which would fail loud rather than silent.
  • The floating arm's "the right lift is zero" remains Floating spawns get the grounding lift — boats are drawn ~14u above the water on a 9.95u-tall hull #756's inference from coord.rs, unchanged and still not measured against a running server.

Test runs — re-measured on the pushed head 01e96a3

  • cargo test -p eqoxide-renderer --locked: Finished \test` profile … in 8m 34s. **215 passed / 0 failed / 11 ignored = 226.** 10 test result:lines, all canonical, allok.; cross-checked against **10** running N testsheaders declaring 226 (theRunning lines are a different thing and were not used as the count). 1 of the 10 is a well-formed all-zero line (the empty doc-test binary) — canonical, reported separately.tests/floating_placement.rs` goes 9 → 11 tests (an earlier revision of this body said 9 → 10).
  • cargo test --workspace --locked: Finished \test` profile … in 8m 25s. **1630 passed / 0 failed / 45 ignored = 1675.** 50 test result:lines, all canonical; **50**running N tests` headers declaring 1675 — agreement. 15 well-formed all-zero lines, reported separately.
  • scripts/check-no-local-detail.sh OK; scripts/check-wrapped-literals.py OK (165 tracked .rs files).

🤖 Generated with Claude Code

https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV

djhenry and others added 2 commits July 28, 2026 02:08
…ight above it

#768 — `static_placement`'s grounded arm passed
`visual_scale = 2 * y_extent * mesh_scale` alongside `y_bottom`, and
`entity_model_matrix_heading` lifts by `visual_scale * 0.5 + y_bottom * mesh_scale`.
The real lift was therefore `(y_extent + y_bottom) * mesh_scale`, and since the
lowest vertex sits at local `y = -y_bottom` the two `y_bottom` terms cancelled and
the model's bottom landed exactly `y_extent * mesh_scale` above the stored z.

Measured on `boat.glb` through the production matrix (stored z 4, heading 0):
drawn origin z 17.927488 / lowest vertex z 13.945171 before, 7.982317 / 4.000000
after — a 9.945171 over-lift, equal to `y_extent * mesh_scale`.

Fixed by removing the `visual_scale` field from `StaticPlacement` and the
`y_extent` parameter from `static_placement`, so the second lift term is not
expressible rather than merely absent. The four `pass.rs` call sites pass a
literal `0.0` for `visual_scale`. No archetype constant changed: `visual_scale`
never reaches the scale matrix.

#769 — `all_archetypes_is_every_archetype_race_to_archetype_can_return` parses
`race_to_archetype` for `=> "`, so four arm spellings return an archetype the
parser cannot see; two of them `cargo fmt` will not normalize, and this repo runs
no fmt job in CI. Added an arrow-count assert (19/19 today), measured RED against
all four spellings individually. The sibling `pass.rs` call-site parser had an
analogous hole (`static_placement (` with a space), now closed; three residual
holes in that parser are documented rather than papered over.

Also corrects three false or now-stale statements in tracked files: the
`gpu.rs:180` mis-citation (the static field is `gpu.rs:141-142`), the orphaned
`archetype_scale` doc block whose `visual_scale = 2 * y_bottom * arch_scale *
node_scale` line was never the code's formula, and the two `y_extent` field docs
that pointed at placement math the renderer no longer does.

Closes #768
Closes #769

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV
…pass a zero visual_scale

Self-review found an overstated mechanism claim I had written: that dropping
`StaticPlacement::visual_scale` "keeps the second lift term from coming back".
It does not. `model` is in scope at all four `pass.rs` call sites and
`GpuStaticModel::y_extent` is public, so `2.0 * model.y_extent * p.mesh_scale`
can still be handed to `entity_model_matrix_heading` there — restoring the exact
pre-#768 over-lift in the real render path.

Measured: making that edit at the entity-pass call site leaves every behavioural
test in this crate GREEN (10 passed / 1 failed, the 1 being the new pin). The
placement tests cannot see it — they build the matrix themselves and pass the
literal `0.0` rather than reading what `pass.rs` passes.

Closed with a device-free source-text pin,
`every_static_entity_matrix_in_pass_rs_passes_a_zero_visual_scale`: the four
matrix calls fed by a `StaticPlacement` (identified by `p.y_bottom`) must each
pass `, 0.0, p.mesh_scale` after whitespace normalization. Same technique and
same caveats as the call-site pin beside it, including a `#769`-class anti-escape
for `entity_model_matrix_heading (` with a space.

The claim in `StaticPlacement`'s doc is corrected to say what is actually
guaranteed and by what.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV
@djhenry

djhenry commented Jul 28, 2026

Copy link
Copy Markdown
Owner Author

Follow-up commit 3305320 — a defect I found in my own claim, and the mutation ledger

The defect

I had written, in StaticPlacement's doc, that dropping the visual_scale field "keeps [the second lift term] from coming back rather than a comment asking future readers not to add one." That is false. Dropping the field removes the term from the shared helper only. model is in scope at all four pass.rs call sites and GpuStaticModel::y_extent is public, so 2.0 * model.y_extent * p.mesh_scale can still be passed as entity_model_matrix_heading's visual_scale — which is exactly the pre-#768 over-lift, in the real render path.

Measured, not reasoned: making that edit at the entity-pass call site left every behavioural test in this crate green. The placement tests cannot see it, because they build the matrix themselves and pass the literal 0.0 rather than reading what pass.rs passes.

Closed with a device-free source-text pin (~25 lines, no GPU device), every_static_entity_matrix_in_pass_rs_passes_a_zero_visual_scale, and the doc sentence now states what is guaranteed and by what.

Mutation ledger — every guard reverted, run, and restored

Each mutation was applied to the pushed tree, run with cargo test -p eqoxide-renderer --test floating_placement --locked, then the file was restored from a pre-mutation copy and re-checksummed (sha256) before the next one. Baseline on the final tree: 11 passed / 0 failed.

# mutation result assert values
M1 static_placement grounded arm returns the pre-#768 total lift (y_bottom + 9.945171, i.e. + y_extent * mesh_scale for boat.glb) RED 9 passed / 2 failed floating_placement.rs:178:9 lowest vertex z 13.945171, stored z 4 (delta 9.945171)
" " floating_placement.rs:202:5 expected 3.9823, got 13.927488
M4 entity-pass call site passes 2.0 * model.y_extent * p.mesh_scale as visual_scale (the pre-#768 lift, in the render path) RED 10 passed / 1 failed floating_placement.rs:496:9 panic prints the offending call verbatim
M2 an existing arm rewritten "SHP" => BOAT_ARCH (named const) RED 10 / 1 floating_placement.rs:358:5 left: 19 right: 18
M2′ same, with the new arrow-count guard deleted RED 10 / 1 floating_placement.rs:371:5 set-equality: boat missing from the parsed list
M3 an added arm "AIR" => AIRSHIP (named const, cargo fmt-immune) RED 10 / 1 floating_placement.rs:358:5 left: 20 right: 19
M3′ same, with the arrow-count guard deleted GREEN 11 / 0 the silent escape: race_to_archetype can now return airship, ALL_ARCHETYPES does not contain it, and nothing fails
M5 a fifth static_placement (…) call site (space before the paren) added to pass.rs RED 10 / 1 floating_placement.rs:426:5 anti-escape message
M5′ same, with the sibling anti-escape deleted GREEN 11 / 0 the silent escape: an unreviewed call site, calls.len() still 4

Two corrections to what I reported earlier, both found by re-running rather than re-reading:

  1. M2 (converting an existing arm) is not a silent escape — the set-equality catches it loudly on its own (M2′). Only an added arm (M3) is silent, because the missing archetype then appears in neither list. My earlier note implying all four spellings hide silently was wrong; the guard's value is specifically against added arms.
  2. M1 is a behavioural mutant reproducing the pre-fix lift arithmetically, not a literal git revert of the fix: a true revert restores a 5-argument static_placement and the new test no longer compiles. The mutant reproduces the pre-fix numbers exactly (13.945171 / 13.927488), which is the evidence that matters.

Under M1, a_grounded_spawn_with_the_same_bounds_is_still_lifted — the test #761 shipped for this arm — stayed ok. That is the direct evidence that #768 was invisible to the existing suite.

What is still NOT pinned by a red-going test

  • Nothing in gpu.rs. That file's whole contribution to this PR is 4 added / 1 removed /// doc lines, zero code — a stale sentence saying the field is used for visual_scale, which the renderer no longer does. There is no behaviour to grade and no device excuse being made.
  • The render_model viewer's divergence (src/bin/render_model.rs). Stated in the doc, not tested.
  • The y_bottom clamp residual (y_min >= 0 ⇒ lift 0). No shipped model reaches it.

Test runs on the final tree (3305320)

cargo test -p eqoxide-renderer --locked — one Finished `test` profile line (31.75s); 10 test result: lines, of which 0 were non-canonical; section reconciliation 9 Running + 1 Doc-tests = 10 ✅ (2 of those 9 Running headers were spliced onto a preceding test … ok line — the splice hazard is real in these logs, it just did not hit a summary line); passed 215 + failed 0 + ignored 11 + measured 0 + filtered 0 = 226 ✅. floating_placement is 9 → 11 tests.

cargo test --workspace --locked — measured on 7c62e9a (this follow-up adds one test and doc text in eqoxide-renderer only): one Finished `test` profile line (6m 02s); 50 test result: lines, 0 non-canonical; reconciliation 35 Running + 2 spliced Running + 13 Doc-tests = 50 ✅; passed 1629 + failed 0 + ignored 45 + measured 0 + filtered 0 = 1674 ✅. A workspace re-run on 3305320 is in flight; the crate figures above are on the exact head.

This proves arithmetic consistency of the logs, not completeness of the suite.

@djhenry

djhenry commented Jul 28, 2026

Copy link
Copy Markdown
Owner Author

Independent review — PR #773 (fix-768-ground-lift, head 3305320)

Verdict: REQUEST CHANGES. Two tracked-file claims are false or materially incomplete as measured. The code is correct and I am requesting no code change — both fixes are doc sentences, plus a hand-written squash body.

Reviewed in my own worktree at 3305320. Note for anyone re-deriving the diff: daab1ef..3305320 shows six unrelated files as reverse deltas; the branch's merge base is b0246bc. From the real base the change is exactly four files under crates/eqoxide-renderer/, +282/−81.


BLOCKING 1 — the new pin closes one channel, not "the other half" (measured, M4′)

The pin every_static_entity_matrix_in_pass_rs_passes_a_zero_visual_scale inspects the entity_model_matrix_heading( call text. It does not look at what is passed into static_placement. So the identical over-lift is still one edit away, through the y_bottom argument instead of the visual_scale argument.

At the entity static call site in pass.rs:

// static_placement(archetype, model.y_bottom, [...], b.floating)
   static_placement(archetype, model.y_bottom + model.y_extent, [...], b.floating)

This restores exactly the pre-#768 lift (y_extent + y_bottom) * mesh_scale in the real render path — the same 9.945171u over-lift on boat.glb the PR quotes.

Measured on 3305320 with only that edit: crate GREEN, 215 passed / 0 failed / 11 ignored, all 10 test result: lines ok. — including the new pin, including every_static_placement_call_site_in_pass_rs_decides_floating_explicitly, and including a_grounded_static_model_is_drawn_with_its_lowest_vertex_on_the_stored_z (which builds its own matrix and never reads pass.rs).

The overstatement is in the framing, and it is in tracked files:

  • tests/floating_placement.rs:456 — "The other half of renderer: the grounded static-model arm lifts the model a full model height above its stored z, not onto it #768, pinned as source text". The disclosure block at :468-471 lists three residuals (source-text-not-semantics, whitespace, other files) but not this one, which is the largest: it is the same one-edit reintroduction the pin was written to close, relocated one argument to the left.
  • src/models.rs:922-927 — "Dropping the field … does NOT make it unwritable … That half is held by a source-text pin instead". This sentence is literally true about the visual_scale channel, but it sits under y_bottom's doc, which opens "There is deliberately no visual_scale field … this field is the only lift a static placement can express." Read together it reads as coverage.
  • Commit 3305320 body — "Closed with a device-free source-text pin".

Requested: one sentence in the pin's disclosure block (and a clause in the models.rs field doc) stating that the pin closes the visual_scale argument only; that the same over-lift can be reintroduced by adding + model.y_extent to the y_bottom argument at the same four call sites; and that this was measured to leave the whole crate green. This repo's dominant defect class is a reasoned-but-unmeasured mechanism claim in a tracked file, and the PR is otherwise scrupulous about exactly this — the sibling pins disclose their M5′-class residuals; this one should too.

BLOCKING 2 — skins == 0 is not the predicate that selects the static path

renderer.rs:663-664:

let use_skinned = asset.skin.as_ref()
    .is_some_and(|s| s.joint_count > 0 && s.joint_count <= 128);

The static arm is taken when NOT 0 < joint_count <= 128. skins == 0 is sufficient but not necessary: a model with skins == 1 and more than 128 joints also renders Static.

tests/floating_placement.rs:37 (skins = 0 → takes the STATIC model path) is fine on its own. The problem is claim 1 at :52-60, which establishes only "every loadable model except boat.glb reported skins == 1" and is then load-bearing for the PR's "no live consumer". skins == 1 does not imply the skinned path.

I re-ran the scan with joint counts over the same 50 loadable names (18 archetypes + 29 race_* + the three _f variants; all 50 present). The conclusion survives — exactly one takes the static arm today, boat at 0 joints. But the margin is not what the doc implies:

model joints cap
race_pcfroglok 127 128
race_huf, humanoid_f 110 128
7 more rigs 109 128

A rebake that adds two joints to the froglok rig puts a PC race on the grounded static arm, where floating() is false — i.e. the "no live consumer" property is one asset bake away from being false, and the model directory is a live sync target (the doc says so itself at :48-50).

Requested: state the real predicate in claim 1 and record the 127/128 margin. The verdict "no live consumer today" is correct and I am not asking for it to be withdrawn.


NON-BLOCKING

  1. PR-body figures are stale. The body reports 214 passed and floating_placement 9 → 10 — those are 7c62e9a's numbers. On the head 3305320 I measure 215 passed and 9 → 11. Worth fixing because the PR body is the squash-body source.
  2. The new pin has an undisclosed p-binding dependency. It selects calls with c.contains("p.y_bottom") and asserts ", 0.0, p.mesh_scale", so an added fifth static matrix site whose placement binding is not named p is invisible to both the count assert and the value assert. Same class as the M5′ residual the sibling pin discloses. Reasoned, not measured — I did not run it.
  3. archetype_scale is left with no /// doc after the orphaned block deletion. Deleting the block is right (its formula was the wrong one); the surviving true content is duplicated in-body at models.rs:733-734 with the correct height = y_extent * arch_scale.
  4. I started a --workspace run on the real head in the background; it had not reached a Finished line when I posted, so I publish no workspace figure on 3305320. The crate-scoped run below is on the head.

Attacked and found nothing

  • gpu.rs is genuinely zero code. The entire hunk is 1 removed and 4 added /// lines on GpuStaticModel::y_extent.
  • visual_scale is consumed at exactly camera.rs:59 and camera.rs:85, both as visual_scale * 0.5 inside Mat4::from_translation, never in from_scale. Confirms "no archetype constant changed".
  • The fifth caller of the grounded arm exists, and it is not an entity spawnscene.rs:189-205, inject_test_billboards(), which hardcodes floating: false where from_game_state passes e.floating(). It is gated to zone == "testzone" (app.rs:1268) and its race list contains no boat/SHP entry, so it cannot reach the static arm and the chain holds. But "four call sites" is a pass.rs count, not a caller count, and a sixth caller would not be caught by the count pin (which parses pass.rs only — disclosed).
  • Fixture constants re-measured from the baked GLB, exact to the digit: POSITION min [-14.881587, -3.982317, -8.304960], max [22.851353, 5.962854, 8.413661]; one mesh row.mod (3 primitives), one node with no TRS, no skins key. The derived y_bottom = 3.982317 / y_extent = 9.945171 follow.
  • The 50-name loadable scan reproduces: all 50 present, exactly one unskinned.
  • The test: the ALL_ARCHETYPES source parser misses four match-arm spellings, two of which cargo fmt will not normalize #769 arrow-count guard is un-balanceable by adding an arm: adding a => " arm raises both sides, and its parsed region extending into archetype_correction's doc comment fails loud, not silent (M3 reddens at :358:5, left: 20 right: 19).
  • Every line number cited in every tracked doc I checked is accurate, including renderer.rs:610 for the _f.glb preference and gpu.rs:141-142.
  • The no-local-detail guard and the wrapped-literal guard both exit 0 on the head.

Merge-order interaction with #761 (0964fdb, already on main, same function, same test file)

Cleared, by measurement not reading. Under M1 (full revert of the grounded arm plus the pre-#768 call-site spelling), #761's a_grounded_spawn_with_the_same_bounds_is_still_lifted stays ok — it asserts lifted at all, not lifted by how much, so it is orthogonal to what #768 changes. That is the PR's strongest single claim and it holds.

The floating arm is unchanged by construction: before and after, the total lift is 0.0 * 0.5 + 0.0 * mesh_scale. a_floating_spawn_gets_no_grounding_lift… is green on the head and stays green under M1, confirming it does not ride on the grounded arm.

Mutation reproduction (all run by me on 3305320, not read from the PR)

# mutation result evidence
M1 revert the grounded lift + call sites to pre-#768 RED 8/3 :178:9 "lowest vertex z 13.945171, stored z 4 (delta 9.945171)"; :202:5 "expected 3.9823, got 13.927488"; :496:9 pin. a_grounded_spawn_with_the_same_bounds_is_still_lifted ok
M3 add an arrow-invisible arm to race_to_archetype RED 10/1 :358:5, left: 20 right: 19
M3′ the => " spelling the guard can see GREEN 11/0 — correctly silent, as disclosed
M4 pass 2.0 * model.y_extent * p.mesh_scale as visual_scale RED 10/1 :496:9, offending call quoted in the panic
M4′ same over-lift via the y_bottom argument GREEN 215/0 new — see BLOCKING 1
M5 static_placement ( with a space RED 10/1 :426:5 anti-escape
M5′ an added call site with a differently-named binding GREEN 11/0 — correctly silent, as disclosed

I did not independently reproduce M2 / M2′; I have no figures for them and make no claim either way.

Mutations were applied by script with an occurrence-count assert before each write, and reverted from checksummed copies (no git checkout/stash). Worktree verified pristine afterwards: git status --porcelain empty at 330532037edf04e89675e17c6a4d24cf0f19d010.

Test-log figures — crate run on the head 3305320

cargo test -p eqoxide-renderer --locked

  1. Finished \test` profile [unoptimized + debuginfo] target(s) in 7m 02s` (cold; the PR's 31.75s was incremental)
  2. 10 test result: lines — 10 expected (9 Running targets + Doc-tests eqoxide_renderer)
  3. 0 non-canonical — all 10 matched ^test result: (ok|FAILED)\. N passed; N failed; N ignored; N measured; N filtered out; finished in …s$; no Running fragment spliced onto any line in my run
  4. 215 passed + 0 failed + 11 ignored + 0 measured + 0 filtered out = 226
  5. running total (sum of every running N header) = 226

Limit: this proves arithmetic consistency, not completeness — a wholly lost test binary drops its running N and its summary together and the identity still balances.

tests/floating_placement.rs = 11 tests (9 → 11).

Acceptance-test scope

Per the fleet skill's explicit exception, I state it rather than skip it silently: there is nothing to E2E here. The grounded static arm has exactly one live consumer class (boat/SHP entities) and the change is a pure vertical placement correction with no in-game observable an agent can query — no API surface, no log line, no state field moves. The mutation-checked suite is the ceiling of verification for this change, which is why BLOCKING 1 matters: the guard is the verification.


If the two doc sentences land, squash with this — do NOT use GitHub's default body

The default concatenates both commit bodies, and 7c62e9a's body contains a claim this branch's own later commit already retracts ("so the second lift term is not expressible rather than merely absent" — it is expressible, at the call sites; 3305320 says so, and M4′ shows a second way). Landing that sentence in main would be exactly the tracked-file falsity this repo treats as blocking.

Subject

fix(#768,#769): ground a static model ON its stored z, not a model height above it (#773)

Body

A grounded static model was drawn a full rendered model height above its stored
z. `static_placement` passed `visual_scale = 2 * y_extent * mesh_scale`, and
`entity_model_matrix_heading` adds `visual_scale * 0.5` on top of
`y_bottom * mesh_scale` — so the lift was `(y_extent + y_bottom) * mesh_scale`
instead of `y_bottom * mesh_scale`. Measured on `boat.glb`: 9.945171u of
over-lift, exactly `y_extent * mesh_scale`.

Fixed by removing the `visual_scale` field from `StaticPlacement` and the
`y_extent` parameter from `static_placement`; the four `pass.rs` static call
sites now pass a literal `0.0` for `visual_scale`. No archetype constant
changed — `visual_scale` never reaches the scale matrix (`camera.rs:59`, `:85`
are its only two consumers, both `* 0.5` inside `from_translation`).

Scope of the guarantee: the term is no longer expressible through
`StaticPlacement`, and the source-text pin
`every_static_entity_matrix_in_pass_rs_passes_a_zero_visual_scale` keeps the
call sites from re-passing it as `visual_scale`. It is NOT unwritable: adding
`+ model.y_extent` to the `y_bottom` argument at the same call sites restores
the identical over-lift and leaves this crate green.

#769 — `all_archetypes_is_every_archetype_race_to_archetype_can_return` parsed
`race_to_archetype` for `=> "`, so an added arm using a spelling the parser
cannot see would pass silently. Guarded by an arrow count.

No shipped pixel moves today: the only loadable model that takes the static
render path is `boat.glb`. The predicate is `!(0 < joint_count <= 128)`, not
`skins == 0`; the closest other rig is `race_pcfroglok` at 127 joints.

Reviewed independently: mutations reproduced on the head, no-live-consumer
chain re-derived, merge-order interaction with #761 cleared by measurement.

Not approving via the review button — the fleet shares one identity and self-approval is blocked. This comment is the verdict.

… TWO channels

RETRACTION. Commit 3305320's subject and body say the second lift term was
"Closed with a device-free source-text pin" and call it "the other half" of
#768. Both are false as written, and the earlier commit cannot be amended here
(no rebase, no force-push), so the correction lives in this commit.

PR #773's round-1 reviewer found a second channel and I reproduced it before
changing anything:

    static_placement(archetype, model.y_bottom + model.y_extent,
                     [model.x_center, model.z_center], b.floating)

restores the identical pre-#768 over-lift (y_extent + y_bottom) * mesh_scale,
and the crate stayed GREEN — 215 passed / 0 failed / 11 ignored, 10 of 10
`test result:` lines ok, including 3305320's own pin, which reads the matrix
call and not the placement call. Dropping the `visual_scale` FIELD constrains
the struct, not what a caller passes as `y_bottom`: an f32 is an f32.

What this commit changes (docs + one test; no production code):

- tests/floating_placement.rs: the pin is renamed
  `every_static_entity_matrix_in_pass_rs_passes_a_zero_visual_scale` ->
  `every_static_placement_in_pass_rs_is_written_exactly_as_reviewed` and now
  grades BOTH channels. Channel 2 is pinned by requiring each of the four
  `pass.rs` calls to match one of two reviewed argument lists exactly, because
  source text cannot bound an expression. Measured: the mutation above now
  fails on that assert (10 passed / 1 failed), printing the offending call;
  the channel-1 mutation reddens this test and nothing else in the crate
  (--no-fail-fast: 214 passed / 1 failed / 11 ignored).
- Its doc replaces "the other half" with an explicit list of what the pin does
  NOT do: source text not semantics, `pass.rs` only, and "four call sites" is a
  count of call sites in one file, not a bound on callers.
- A stronger pin was considered and declined with a reason, recorded on
  `StaticPlacement::y_bottom`: making the over-lift unrepresentable means
  `static_placement` taking the model's bounds as an opaque value only the
  loader can mint, but `GpuStaticModel` is built field-by-field from another
  crate (src/bin/render_model.rs:853), so that is a cross-crate API change
  wider than #768.
- The sibling parser's residual-hole list gains a fourth entry: a fifth
  producer of the `floating` flag outside pass.rs. `Scene::inject_test_billboards`
  hardcodes `floating: false` (src/scene.rs:248); it is gated to
  `zone == "testzone"` (src/app.rs:1267-1269) and its race table
  (src/scene.rs:189-205) has no SHP entry, so no static-arm model reaches it
  today — by asset and zone facts, not by construction.
- CORRECTED MECHANISM for the no-live-consumer finding. The static arm is
  selected by `!(0 < joint_count <= 128)` (renderer.rs:663-664), so `skins == 0`
  is sufficient, not necessary, and the earlier `skins`-only scan did not
  establish which arm a model takes. Re-scanned all 50 loadable names reading
  joint counts: still exactly one static model (boat.glb, 0 joints) and nothing
  over the cap — but race_pcfroglok.glb is at 127 of 128 and 11 rigs are at
  >= 109, on a model directory rebaked outside this repo. A two-joint rebake
  puts a PC race, for which floating() is false, on the grounded arm. The arm
  has to be correct rather than merely unreached.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV
@djhenry

djhenry commented Jul 28, 2026

Copy link
Copy Markdown
Owner Author

Round 2 pushed as 01e96a3 (docs + one test; no production code changed).

M4′ reproduced before anything was written. static_placement(archetype, model.y_bottom + model.y_extent, [model.x_center, model.z_center], b.floating) at the entity site restores the exact pre-#768 lift and the crate stayed GREEN — 215 passed / 0 failed / 11 ignored, 10 of 10 test result: lines ok., 10 running N tests headers declaring 226, Finished in 3m 11s. The finding stands as written.

Stronger pin: partially taken, and the rest declined with a reason. Source text cannot bound an expression, so the pin now requires each of the four pass.rs calls to equal one of two reviewed argument lists exactly (REVIEWED_ARGS). Under M4′ it is now RED — 10 passed / 1 failed, printing found: archetype, model.y_bottom + model.y_extent, …. The channel-1 mutation reddens that test and nothing else in the crate (--no-fail-fast: 214 passed / 1 failed / 11 ignored). Making the bad state unrepresentable would mean static_placement taking the model's bounds as an opaque newtype only the loader can mint; GpuStaticModel is a public-field struct built field-by-field outside the render path (src/bin/render_model.rs:853), so that is a cross-crate API change wider than #768. Declined on scope, and said so in the doc rather than implied.

All four overstating artifacts corrected: the test is renamed every_static_placement_in_pass_rs_is_written_exactly_as_reviewed; its doc replaces "the other half" with an explicit "what this still does not do" list; StaticPlacement::y_bottom now names two channels and says the pin is not a type-level guarantee and does not reach another file; and 3305320's "Closed with a device-free source-text pin" is retracted in 01e96a3's body (no rebase/force-push available to amend it).

B2 mechanism corrected. The gate is !(0 < joint_count <= 128) (renderer.rs:663-664) — skins == 0 is sufficient, not necessary. Re-scanned all 50 loadable names reading joint counts: still exactly one static model (boat.glb, 0 joints), nothing over the cap, but race_pcfroglok.glb is at 127 of 128 and 11 rigs are ≥ 109, on an externally rebaked directory. Recorded in both models.rs and the module doc.

Fifth caller. Added as a fourth residual hole: Scene::inject_test_billboards hardcodes floating: false (src/scene.rs:248), gated to zone == "testzone" (src/app.rs:1267-1269), and its 16-race table (src/scene.rs:189-205) has no SHP entry — the chain holds by asset and zone facts, not by construction. "Four call sites" is now stated as a pass.rs count, not a bound on callers.

Body re-measured on 01e96a3: crate 215 / 0 / 11 = 226 (10 canonical result lines, 10 running N headers declaring 226, 0 non-canonical, 1 well-formed all-zero doc-test line, Finished 8m 34s); workspace 1630 / 0 / 45 = 1675 (50 canonical result lines, 50 headers declaring 1675, 0 non-canonical, 15 all-zero lines, Finished 8m 25s). floating_placement is 9 → 11 tests (the old body's "9 → 10" was stale). check-no-local-detail.sh OK, check-wrapped-literals.py OK.

@djhenry

djhenry commented Jul 28, 2026

Copy link
Copy Markdown
Owner Author

Independent review — PR #773, round 2 (head 01e96a3)

Verdict: APPROVED. Both round-1 blocking findings are resolved, and resolved by correction rather than by rewording. Every figure published in the body and the reply reproduces on my own run. I attacked the new pin with three fresh evasions; two of them stay green, but both land squarely inside the residual class this round explicitly discloses — which is the difference between round 1 and round 2. Non-blocking items and two follow-ups below.

I re-reviewed the whole head, not the delta.


The round-2 claim, measured first

"Docs + one test only, zero production-code change" — true, by measurement rather than by reading the diff:

file 330532001e96a3
src/pass.rs byte-identical, md5 358778e06017d73af12f6dbd63407fe4
src/gpu.rs byte-identical
src/models.rs changed — but with //-comment lines and blank lines stripped, the two revisions are line-for-line identical at 1301 lines each, and every changed line in the delta begins with //
tests/floating_placement.rs changed; test count 11 → 11, with exactly one name in the symmetric difference (…passes_a_zero_visual_scale…is_written_exactly_as_reviewed)

So the +385/−92 growth is documentation and one rewritten test. Nothing else moved.


B1 — resolved. Three fresh evasions constructed; here is what each did

I built the round-1 evasion again and three new ones. All applied by script with an occurrence-count assert before each write, reverted from md5-verified copies, worktree confirmed clean at 01e96a3 afterwards.

mutation at the entity site result
channel 2model.y_bottom + model.y_extent (my round-1 M4′) RED, floating_placement 10 passed / 1 failed, panic at floating_placement.rs:543:9, message printing found: archetype, model.y_bottom + model.y_extent, … against Expected one of [...]
channel 1visual_scale = 2.0 * model.y_extent * p.mesh_scale, --no-fail-fast 214 passed / 1 failed / 11 ignored, the single failure being the pin, panic at :569:9
E1 — shadow model with a same-field-named local, call moved into a block RED — but incidentally: the call no longer ends in );, so the extractor over-ran into the next statement and the argument text stopped matching
E1b — the same shadow, written so the call still ends in ); GREEN — 215 passed / 0 failed / 11 ignored
E2 — corrupt the loader instead: y_bottom = -y_min-y_min + (y_max - y_min) GREEN — 215 passed / 0 failed / 11 ignored

E1b is the interesting one. All four extracted argument lists are byte-identical to REVIEWED_ARGS — I printed them — yet the entity site computes the exact pre-#768 over-lift. Rebinding model to a local carrying the same field names is textually invisible, and so is a wrapper fn static_placement in pass.rs shadowing the import (same class; reasoned, not measured — E1b is the measured instance).

This is not blocking, and the distinction matters. The doc at floating_placement.rs:509-510 states the residual in the general form that covers both: "It is source text, not semantics. It proves the argument is written that way, not that the call is reached, and not that model.y_bottom itself holds what the loader intended." E1b and E2 are instances of exactly that sentence. Round 1 was blocking because the artifacts asserted a coverage the pin did not have; round 2 states the coverage it does have and lists what it does not. A pin that is honest about being text is doing its job; a pin that claims to be a type is not.

Two things I can add to the account:

  • E2 is worth a follow-up on its own terms. The loader reduction at models.rs:485-488y_min, y_max, y_bottom, y_extent — is the producer of the number this whole PR is about, and corrupting it leaves the entire crate green. That is a coverage gap independent of the pin: no test grades the reduction. Suggest filing it.
  • The over-run direction is safe, and completes residual Door spawn names decode to garbage / empty in arena #3. The sibling parser's residual list (:438-440) documents a call whose argument list contains ); before its own close, i.e. early termination. The opposite — a call whose close is not followed by ;, so the slice runs on — is undocumented, and I measured it: it fails loud (E1 above). Worth one clause, since it turns an unknown into a known-safe direction.

The declined newtype — the cost estimate holds, and here is a sharper reason

Spot-checked the construction sites myself. GpuStaticModel is built at exactly two places: renderer.rs:751 and src/bin/render_model.rs:851 (the y_bottom:/y_extent: field line the body cites as :853 is inside that literal — accurate citation). Two sites is not much churn on its own, so "every construction site would change" is the weaker half of the argument.

The decisive half is one the body does not make, and it is stronger than the one it does: tests/floating_placement.rs is an integration test. It compiles against eqoxide-renderer as an external crate, so it cannot see #[cfg(test)] helpers. An "only the loader can mint it" newtype therefore forces either a pub mint — which pass.rs can call too, degrading the guarantee to a naming convention — or a feature flag. And a device-free test cannot sidestep this by passing a real GpuStaticModel, because that struct owns wgpu::BindGroups and needs a device. Ruling: declining tier 1 here is correct, and "documented as declined" is adequate.

There is, however, a middle option neither of us costed, which I raise as a suggestion rather than a request (reasoned, not prototyped — I did not compile it): give GpuStaticModel a plain bounds: ModelBounds { y_bottom, y_extent, x_center, z_center } and let static_placement(archetype, &model.bounds, floating) read the field itself. ModelBounds stays freely constructible, so the integration tests keep working and nothing becomes unforgeable — but there is no longer a lift argument at the call site to do arithmetic in, so channel 2 in its realistic form (+ model.y_extent slipped into a call) stops being expressible rather than being pinned. It would not stop E1b, and it is a real refactor; it is a better follow-up issue than a round-3 request.

B2 — resolved; every figure re-measured

Gate re-read at renderer.rs:663-664: use_skinned = asset.skin.as_ref().is_some_and(|s| s.joint_count > 0 && s.joint_count <= 128). Static arm is !(0 < joint_count <= 128); skins == 0 sufficient, not necessary. ✓ as corrected.

Re-scanned the 50 loadable names reading len(skins[0].joints):

  • 50 names, all present; exactly one on the static arm — boat, 0 joints; nothing above the cap.
  • race_pcfroglok at 127; 11 rigs at ≥ 109, next highest 110. ✓ all three figures.
  • I was wrong in round 1 when I said 10 rigs at ≥109 — that came from stitching two partial scans. The author's 11 is right; the corrected count is race_pcfroglok 127, humanoid_f and race_huf 110, then eight at 109.

floating() chain re-derived: is_boat_race (race_class.rs:32-34) → eq_race_to_code returns "SHP" (:36-39) → Entity::is_boatfloating() at game_state.rs:192coord::skips_wire_z_offset. ✓

On disposition — you asked me to say plainly. A documented note is not sufficient on its own, but it is also not this PR's debt. The 128-joint cap and its silent downgrade to the static path predate #768; #773 only discovered and wrote them down, and fixing them here would be exactly the scope creep the author correctly refused for the newtype. So: not blocking, but file it. On the honesty question specifically: the failure would be an inconsistency between two of the client's own outputs — the position API says the model is on the ground, the render puts it a model height above — which is the agent-honesty shape even though no state field lies. The cheap honest form is not a placement fix at all: it is a tracing::warn! at renderer.rs:663 when a model has a skin but falls to the static arm because it exceeded the cap. Two lines, no behaviour change, converts a silent downgrade into a logged one. Worth its own issue.

The fifth caller — re-derived independently

Workspace-wide, static_placement is called at exactly four places, all in pass.rs (:1109, :1268, :1680, :1716); nothing else in any crate or bin calls it, and StaticPlacement is constructed only inside static_placement itself (models.rs:1032, :1034). Billboard.floating is produced at exactly two places: scene.rs:337 (e.floating()) and scene.rs:248 (hardcoded false). The hardcode is called only under if self.scene.zone == "testzone" at app.rs:1267-1269, and the race table at scene.rs:189-205 is 16 entries with no SHP. ✓ Every citation checked and exact; the "by asset and zone facts, not by construction" framing is the right one.

One observation, non-blocking: that chain and the 127/128 hazard share a trigger. A rebake that pushed any of the 16 archetypes in that table onto the static arm would also feed a hardcoded floating: false — though in testzone that is the correct value, so it is a coupling to note rather than a defect.

The four overstating artifacts — checked for re-instantiation

  • Test renamed; its doc names both channels with the measured figures and carries an explicit "what this still does not do" list (:508-515). No re-instantiation.
  • StaticPlacement::y_bottom (models.rs:922-935) now says "TWO ways back to it, not one", names both, and says "NOT a type-level guarantee". Closest thing to a residual is the phrase "What holds them today is a source-text pin" — given E1b, the pin holds the spellings, not the values — but the very next sentence bounds it to "each of the four call sites … spelled exactly as reviewed", and the test doc discloses the value gap. Precision nit, not a false claim.
  • 3305320's body is retracted in 01e96a3's body, and the retraction is accurate about what it retracts: it quotes "Closed with a device-free source-text pin" and "the other half", says both are false as written, and gives the reproduction. It does not overclaim the new pin in weaker words.
  • PR body checked line by line — this is where the last several merges' findings landed, and it stays live at its URL. The [R2] markers are honest, the "What I did NOT verify" section is expanded rather than trimmed, and the two claims I could falsify in round 1 are both gone.

Figures I could not reconcile, and one I could

  • PR body, "Mutation check": "measured 9 passed; 2 failed" for reverting the fix. My round-1 revert measured 8 passed / 3 failed. Both are reachable: 9/2 if the revert is confined to the helper, 8/3 if the call-site text is reverted too, which additionally reddens the pin. The body does not say which. Under-specified rather than wrong — one clause naming the mutation's extent would settle it. Non-blocking.
  • The 538:9 panic location in the round-2 brief I was given: I measured 543:9 for the channel-2 mutation, and 538:9 does not appear anywhere in the author's reply comment (grep count 0). :538 is the placements.len() == 4 count assert; :543 is the REVIEWED_ARGS assert inside the loop, at 8-space indent. The relayed number appears to be an artifact of the relay, not of this PR. Nothing in a tracked file or in the body cites a line number here.
  • The earlier workspace run's corrupt line reconciles. 1630 passed + 41 parsed ignored = 1671; recovering the corrupted line's 4 ignored gives 1630 + 45 = 1675. ✓ arithmetic. And I can corroborate the phenomenon independently — see below.

Stop-early discipline — confirmed, and no surviving figure is contaminated

I reproduced the catch: running the channel-2 mutation without --no-fail-fast produced 2 test result: lines and 2 running N headers — 2 of 10 binaries. The methodological note is real and correctly stated.

Auditing every figure that survives in the body and the docs: the crate figure reports 10 lines, the workspace 50, so neither is a stop-early run; the channel-1 figure states --no-fail-fast explicitly; and every remaining figure (channel 2, the #769 spelling table, the sibling-parser probe, the revert) is scoped to the floating_placement binary and makes no crate-wide claim. No crate-wide claim rests on a stop-early run.

Test-log figures — re-measured on 01e96a3, published as five each

Crate, cargo test -p eqoxide-renderer --locked:

  1. Finished \test` profile [unoptimized + debuginfo] target(s) in 5m 44s` (mine; the body's 8m 34s is the same target under different machine load — build wall-clock is not a claim)
  2. 10 test result: lines — 10 expected
  3. 0 non-canonical; 1 well-formed all-zero (the empty doc-test binary), reported separately
  4. 215 passed + 0 failed + 11 ignored + 0 measured + 0 filtered out = 226
  5. 10 running N headers declaring 226

tests/floating_placement.rs = 11 tests. 9 → 11 ✓ (the old body's 9 → 10 is gone).

Workspace, cargo test --workspace --locked:

  1. Finished \test` profile [unoptimized + debuginfo] target(s) in 18m 39s`
  2. 50 test result: lines
  3. 0 non-canonical; 15 all-zero, reported separately
  4. 1630 passed + 0 failed + 45 ignored + 0 measured + 0 filtered out = 1675
  5. 50 running N headers declaring 1675

Every published figure matches, including the 15 all-zero lines.

Limit, restated: this proves arithmetic consistency, not completeness.

And I hit the splice myself. My workspace log contains exactly one corrupted line — a Running tests/shadow_caster_selection.rs … written into the middle of a test shadow_caster_slot_budget_is_sixty_four ... progress line. So the interleaving is real and reproducible, not a one-off. Two consequences worth recording: it landed on a progress line rather than a summary line, so the summary set stayed canonical and the totals are unaffected; and it is why an ^ *Running count reads 36 in my log while the true count is 37 (36 + the spliced one + 13 Doc-tests = 50 = headers = summaries). The author's decision to count running N headers rather than Running lines is the right one, and this is a second measured reason for it.

CI

Checked myself on 01e96a3: no-local-detail pass (8s), test pass (2m 53s). Those are the only two jobs — .github/workflows/test.yml defines no-local-detail and test, no fmt or clippy job, so the body's claim that fmt is a convention here and not a gate is correct. 165 tracked .rs files ✓.

Merge order

#773 is disjoint from both. Its four files are all under crates/eqoxide-renderer/; #774 touches eqoxide-command/src/nav.rs, eqoxide-http/src/observe.rs, eqoxide-ipc/src/lib.rs, eqoxide-nav/src/walker.rs, eqoxide-net/src/action_loop.rs, docs/http-api.md; #775 touches eqoxide-command/src/slot.rs, eqoxide-http/src/guild.rs.

Textual disjointness is not the check that matters for this PR, though, because #773 ships source-text pins — a PR that edits a file a pin parses can redden it without touching a file #773 changed. So I enumerated the pins' inputs: floating_placement.rs include_str!s exactly ../src/pass.rs and ../src/models.rs, both inside eqoxide-renderer. Neither #774 nor #775 touches either. No behavioural overlap either — nav/walker/observe and slot/guild share no assumption with static-model placement.

#773 slots anywhere in #775 → #774 → #778, and it neither pins an assumption those PRs change nor changes one they pin. Base is still main with merge-base b0246bc; no rebase, no force-push, confirmed.

Not measured — accepted as disclosed

No live client (and per the blast-radius finding there is no in-game observable to run — per the fleet skill's exception, stated rather than skipped); the newtype costed by reading; the joint scan a snapshot of one directory; render_model unconverged and not run; the y_bottom clamp residual documented not fixed. All four are in the body's "What I did NOT verify" and none is load-bearing for a claim made elsewhere in it.


Squash message — hand-written; do not use GitHub's default

The default body concatenates all three commits, which would land 7c62e9a's "not expressible rather than merely absent" and 3305320's "Closed with a device-free source-text pin" / "the other half" in main as unqualified assertions. 01e96a3 retracts them, but a reader of main's history sees the concatenation, not the ordering.

Subject

fix(#768,#769): ground a static model ON its stored z, not a model height above it (#773)

Body

A grounded static model was drawn a full rendered model height above its stored
z. `static_placement` passed `visual_scale = 2 * y_extent * mesh_scale`, and
`entity_model_matrix_heading` adds `visual_scale * 0.5` on top of
`y_bottom * mesh_scale`, so the lift was `(y_extent + y_bottom) * mesh_scale`
instead of `y_bottom * mesh_scale`. Measured on `boat.glb`: 9.945171u of
over-lift, exactly `y_extent * mesh_scale`.

Fixed by removing the `visual_scale` field from `StaticPlacement` and the
`y_extent` parameter from `static_placement`; the four `pass.rs` static call
sites pass a literal `0.0` for `visual_scale`. No archetype constant changed —
`visual_scale` never reaches the scale matrix (`camera.rs:59`, `:85` are its
only two consumers, both `* 0.5` inside `from_translation`).

Scope of the guarantee, stated precisely because two earlier commit messages on
this branch overstated it and are retracted by the third: the over-lift is NOT
unrepresentable. There are two ways back to it — passing
`2.0 * model.y_extent * p.mesh_scale` as `visual_scale`, or passing
`model.y_bottom + model.y_extent` as `y_bottom` — and both were measured to
leave every behavioural test in this crate green. What holds them is a
source-text pin over `pass.rs`, `every_static_placement_in_pass_rs_is_written_
exactly_as_reviewed`, which requires each of the four calls to be spelled as
reviewed. It bounds four call sites in one file. It is not a type-level
guarantee, it does not reach another file, and it does not constrain what the
names in those spellings denote. Making the state unrepresentable would mean
`static_placement` taking the model's bounds as a value only the loader can
mint; declined here on scope, with the reason recorded on
`StaticPlacement::y_bottom`.

#769 — `all_archetypes_is_every_archetype_race_to_archetype_can_return` parsed
`race_to_archetype` for `=> "`, so an ADDED arm using a spelling the parser
cannot see would pass silently; an existing arm converted to one is caught
loudly. Guarded by an arrow count (19/19). The sibling `pass.rs` parser had the
same class of hole for an added `static_placement (…)` site; guarded likewise.

No shipped pixel moves today: the only loadable model on the static render path
is `boat.glb`, and `boat` is selected only for race code "SHP", for which
`floating()` is true. The predicate is `!(0 < joint_count <= 128)`
(`renderer.rs:663-664`), not `skins == 0`. The margin is one asset bake wide:
`race_pcfroglok` sits at 127 of 128, and that directory is baked outside this
repo — which is why the arm has to be correct rather than merely unreached.

Reviewed independently over two rounds. Round 1 found the pin covered one of
two channels; round 2 corrected it in docs and one test with no production-code
change, verified byte-identical `pass.rs`/`gpu.rs` and comment-only `models.rs`.

Follow-ups worth filing (not conditions of this merge)

  1. The loader reduction at models.rs:485-488 is ungraded — corrupting y_bottom there leaves the whole crate green. It produces the number renderer: the grounded static-model arm lifts the model a full model height above its stored z, not onto it #768 is about.
  2. A warn! when a model with a skin falls to the static arm because it exceeded the 128-joint cap, so the race_pcfroglok 127/128 trigger is loud instead of silent.
  3. The ModelBounds-parameter shape above, as a cheaper alternative to the declined newtype.

Not approving via the review button — the fleet shares one GitHub identity and self-approval is blocked. This comment is the verdict.

@djhenry
djhenry merged commit 3d60bfd into main Jul 28, 2026
2 checks passed
@djhenry
djhenry deleted the fix-768-ground-lift branch July 28, 2026 10:06
djhenry added a commit that referenced this pull request Jul 28, 2026
…t writers; unwrap a doc span

Round 6 was REQUEST CHANGES on two findings, both again in prose this PR
wrote. The reviewer re-verified the code has not moved.

B14 -- my 12-term concept sweep did not contain `permanent*`, and that one
missing term yields four survivors, three on lines round 6 ADDED:

  - docs/http-api.md contradicted itself inside one section: line 888
    "recovering it needs a client restart", line 915 "permanently degraded".
    A restart recovers it, so it is not permanent -- and this is the surface
    where "session-scoped" was defended as accurate. "Permanently" is not
    "session-scoped"; it is strictly stronger.
  - observe.rs was B12's shape at six lines' range, inside one comment block
    round 6 added: "the lifetime is the fine WORKER's, not the process" and
    six lines later "permanently degraded".
  - two test docs asserted the same permanence as fact.

Three OTHER uses of the word stay: they name the lie a second worker would
tell, which is why this is a finding and not a quibble -- the PR was saying
in the same four files both that the degradation is permanent and that
"permanent" is what it would be lying about. Round 6's own diff deleted this
word from one walker assert message, so the concept was recognised that round
and fixed in one of five factual uses.

All four now use the reviewer's suggested shape: "degraded to the coarse 8u
route, and nothing on any nav route recovers it" -- a claim about WRITERS,
checkable against retire_to_idle and Walker::new, where "permanently" was
checkable against nothing.

THE METHOD CHANGED, NOT THE LIST. Adding permanent* to a 13-term list would
reproduce the method that just failed. The sweep was keyed on the PREDICATE
-- the open-ended set of English phrasings for "this outlives the worker" --
which cannot be enumerated. Two measured failures on this PR: a field-NAME
grep missed 21 of 24; a 12-term PREDICATE list missed 4 more.

Round 7 sweeps by SUBJECT, which is a closed set: local_planner_dead,
nav_local_planner_dead, LocalPlanner, fine planner, fine-planner, fine worker,
fine tier, FINE 2u, planner_dead, coarse 8 ?u -- over crates/, docs/, src/.
224 anchor lines, +/-6-line window, 1096 prose lines. Validated against a
corpus I did not choose: the reviewer's 16 terms plus my original 12 give 431
tree-wide hits, of which 36 fall inside the subject window and 395 outside.
All 36 read; every one is either about a different field (the COARSE planner,
net_thread_dead, transport lifetime) or is one of this round's corrections.
The control's vocabulary found nothing inside the subject the subject sweep
did not already contain. No "found N, fixed N" figure is restated.

B15 -- two unbalanced doc spans, in the one touched file no guard reaches.
Measured against the TRUE merge base daab1ef (diffing against main mixes in
other PRs' observe.rs edits): ipc/lib.rs 12->12, walker.rs 0->0, observe.rs
4->6. The pair was one code span, `.filter(|l| l.state != "threaded")`, broken
across a /// wrap -- cargo doc renders the break inside the span and #773
records the fragment as un-greppable. Now on its own line, with a comment
saying why. observe.rs is back to the merge base's 4; the remaining pairs
(1418/1419, 3820/3821) are pre-existing and out of scope.

Reach proved by RUNNING the guard, not by reading it: steering.rs copied aside
(md5 dac4b3b0...), observe.rs added to cited_in, real guard run -> exactly
four span hits at the two pre-existing pairs, test RED, `0 passed; 1 failed;
0 ignored; 0 measured; 231 filtered out`; restored by copy-back with the md5
re-verified identical. No git stash, no git checkout.

N1 filed as #787 (agent-honesty, severity:low): "exactly one fine worker per
process" is load-bearing for four sentences here and pinned by nothing. True
today, verified independently; the B9 test cannot be the pin because building
a second Walker is its method. The three doc sites now cite #787.

Doc and comment only; zero non-comment lines changed. Four crates re-run:
0 `error` lines, 8 result lines vs 8 headers, 0 non-canonical, 1 all-zero
anchored (4 naive -- fourth confirmation of the anchored rule),
882 + 0 + 19 + 0 = 901 = header sum, 0 FAILED. Per crate 216/247/37/380,
identical to rounds 5 and 6.

SQUASH GUIDANCE -- write the squash body from the FINAL state of the tree, not
by assembling commit bodies. That rule is the mechanism; the list below is a
cross-check on it, and the list has needed extending three rounds running
while the rule has not. Do NOT carry:
  - "a clear on a route that cannot happen is untestable" (2fd3da9);
  - the `pending`/`post_if_idle` claim: e73a0e1, ef99227, 1204a0e;
  - "drive_walk returns early in three places" (2fd3da9) -- it is five;
  - "SESSION-scoped ... never cleared" and "it can never outlive the thread it
    is reporting on" (d56f069's parents);
  - 1204a0e's "a latched, session-scoped client fault meaning steering has
    permanently degraded" and "a new session-scoped NavStatus::
    local_planner_dead" -- both corrected scope words plus B14's, one body;
  - "24 found, 24 fixed" as a completeness claim, and any per-round test total
    as the merged tree's: this branch is based on daab1ef, before #755.
DO carry ef99227's retraction paragraph.

Closes #766. Refs #787.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant