fix(#781): give static placement one ModelBounds value instead of loose lift floats - #828
Conversation
…se lift floats `static_placement` took `y_bottom: f32` and `center_xz: [f32; 2]`, and the four `pass.rs` static call sites then passed a literal `0.0` for `entity_model_matrix_heading`'s `visual_scale`. Both of those are lift arguments a call site can do arithmetic in, and #773 measured both green against every behavioural test in the crate — only a source-text pin caught them. Two changes: - `GpuStaticModel`'s `y_bottom`/`y_extent`/`x_center`/`z_center` become one `models::ModelBounds`, and `static_placement(archetype, &model.bounds, floating)` reads it. The measured evasion `static_placement(archetype, model.y_bottom + model.y_extent, …)` is now `error[E0308]`. - New `camera::entity_model_matrix_static(pos, heading, &StaticPlacement, correction)` binds `visual_scale = 0.0` and `y_up = true` in its body. The measured evasion `visual_scale = 2.0 * model.y_extent * p.mesh_scale` is now `error[E0061]`. This half is beyond the issue's literal proposal and was added because measuring the ModelBounds parameter alone showed it left `visual_scale` — the actual shape of the #768 bug — a plain `f32` at the same four call sites. Stated precisely, because overstating this boundary is what two of #773's commit messages had to retract: this does NOT make the over-lift unrepresentable. `ModelBounds` has public fields, so `&ModelBounds { y_bottom: model.bounds.y_bottom + model.bounds.y_extent, ..model.bounds }` compiles and restores the identical over-lift; it is caught by the source-text pin and by nothing else (measured, 261 passed / 1 failed). Calling `entity_model_matrix_heading` directly still compiles too, and a new guard in the pin catches it. The unforgeable newtype that would make the state unrepresentable stays declined for the reason #773's reviewer found: `tests/floating_placement.rs` is an integration test and cannot see a `#[cfg(test)]` mint. One thing got strictly stronger rather than merely differently pinned. The `visual_scale = 0.0` the static arm depends on used to live at four `pass.rs` call sites the device-free tests could not reach; it now lives inside `entity_model_matrix_static`, which those tests DO call. Replacing it with `2.0 * p.y_bottom * p.mesh_scale` turns `a_grounded_static_model_is_drawn_with_its_lowest_vertex_on_the_stored_z` RED — a behavioural failure where the same corruption previously produced only a source-text failure. Not addressed, and not claimed: the two evasions #773's reviewer measured that are semantic rather than syntactic — rebinding `model` to a local with the same field names (E1b), and corrupting the loader reduction that produces `y_bottom` (E2). Both are still green. Workspace unchanged at 1810 passed / 0 failed / 47 ignored, 55 headers = 55 result lines, identical to the 41cca4e baseline; no test was added or removed.
Self-review found a hole in the guard added earlier in this PR: it asserted no `entity_model_matrix_heading` call argument list contains `p.y_bottom` or `p.center_xz`, which pins the binding name. `let q = &p;` followed by `entity_model_matrix_heading(…, q.center_xz, true, q.y_bottom, …)` evades it while doing exactly the thing the guard exists to stop — #773's E1b class. Widened to bare `.y_bottom` / `.center_xz`. Measured, not reasoned: - No false positive. Crate green with the widened guard: 14 headers = 14 result lines, 262 passed / 0 failed / 12 ignored — identical to the `41cca4e` crate baseline. The six `entity_model_matrix_heading` calls that legitimately remain in `pass.rs` were enumerated with the same argument extractor the test uses; all six pass `[0.0, 0.0]` and `0.0` in those two positions, so not one mentions either field. - Reach control. Call-site mutation m8 adds `let q = &p;` plus an additive `entity_model_matrix_heading` call taking `q.center_xz` / `q.y_bottom` at `pass.rs:2113` (additive, so the two count asserts stay satisfied at 4 and 4). RED: 261 passed / 1 failed / 12 ignored, `every_static_placement_in_pass_rs_is_written_exactly_as_reviewed` panicking at the widened guard, `floating_placement.rs:648`. The previous `p.`-pinned form does not occur anywhere in that argument list, so it would have stayed green. No test added, removed or renamed: 11 `#[test]` before and after. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV
|
CHANGES REQUESTED Independent adversarial review. I did not write this change. Everything below is measured on this branch merged with the current The code is right. Both signature changes do what they say, the four call sites are behaviour-preserving, and the headline "the BLOCKING — the "what is left open" sentence is wrong: the
|
| probe | site | form | result |
|---|---|---|---|
| RC1 | pass.rs:1356 — first heading call in the file |
plain multi-line | RED, guard at :648, offending args printed |
| RC2 | pass.rs:1447 |
nested from_cols_array_2d(&…) |
RED, guard at :648 |
| RC3 | pass.rs:2103 — last heading call in the file |
plain multi-line | RED, guard at :648 |
So the guard genuinely reaches the whole file and both call shapes; it is not a ~12%-of-corpus scanner. That makes the blocking finding a real blind spot in what it matches, not an artifact of where it looks. RC2 also shows the extractor over-captures one ) on the nested form (arg list ends …archetype))) — harmless for a contains check, noted only so the next person is not surprised.
Five figures — merged tree (this branch + 223ac9b), streams captured separately
cargo test --workspace --locked --no-fail-fast, process exit 0, no test result: FAILED line:
Finished `test` profile [unoptimized + debuginfo] target(s) in 25m 11s- 55
running [0-9]+ tests?headers = 55test result:lines (regex matches the singularrunning 1 test) - 0 non-canonical result lines
- 14 targets at the full triple
0 passed; 0 failed; 0 ignored(the bare0 passedanchor reads 18, which is the all-#[ignore]d targets, not a regression) - 1823 passed + 0 failed + 47 ignored + 0 measured + 0 filtered out = 1870 = the header sum
Delta vs current origin/main (223ac9b: 55 = 55, 0, 47, 1823 + 0 + 47 + 0 = 1870 across 55/55): zero in every field. Nothing to reconcile by name — no test added, removed, renamed or re-ignored, and no cross-PR behavioural collision with #821. Running unittests src/bin/render_model.rs present (0 tests), so the re-spelled viewer is compiled by the workspace job.
Crate baseline used for every mutation above, same tree: 14 headers = 14 result lines, 262 passed / 0 failed / 12 ignored.
Every mutation was applied from an md5-verified pristine copy with an exact occurrence-count assert before the write, reverted by cp -p from that copy with md5 re-verified and touched after, and the tree re-confirmed pristine after each of the eleven runs.
Not verified, and not claimed
- No live or GPU run, and none is possible for the grounded static arm — I did not re-derive the "
boat.glbis the only static-arm model andSHPis alwaysfloating" scan either. Inherited from fix(#768,#769): ground a static model ON its stored z, not a model height above it #773/renderer: a skinned model that exceeds the 128-joint cap falls to the static arm SILENTLY — race_pcfroglok sits at 127 of 128 #780, same as the author. - The
41cca4econtrol rows (ctrl-1, ctrl-2 at 261/1/12 on the old base) — I did not re-run them. They are about a tree that is no longer the base. - E1b and E2 — I did not re-run them on this branch either. I accept the author's statement that this PR does not address them, and I did not test whether it accidentally does.
render_model.rs's viewer was not launched. I read the re-spelling and it is mechanical, but reading is not measuring — same disclosure the author makes.ModelBoundsvalidation — I did not probe what a garbageModelBoundsfrom either construction site does downstream.- I did not test the
#[cfg(any())]/if false/macro_rules!members of the source-pin evasion family. Reasoning (not measurement): all three have to either add or remove astatic_placement(/entity_model_matrix_static(text occurrence, which the twolen() == 4asserts catch. The two evasions I did find (EF, SP) both work by leaving all four counts and all four argument texts untouched, which is a different mechanism, so I spent the budget there. Treat the first sentence of this bullet as unmeasured. - Whether any of the four sites is reached at runtime. Unchanged by this PR, and the pin says so.
…klist cannot enumerate the aliases #828 round 1 closed the remaining channel-1 escape with a BLACKLIST: no `entity_model_matrix_heading` argument list in `pass.rs` may contain `.y_bottom` or `.center_xz`, reasoning that those are `StaticPlacement`'s two fields `HumanoidPlacement` does not have, so their presence "identifies a static placement". The review measured that reasoning false in the load-bearing direction: `GpuStaticModel::bounds` is in scope at all four static sites and carries the same numbers under other names, so the whole #768 over-lift written as `model.bounds.y_extent` / `.x_center` / `.z_center` SURVIVED, fully green at 262 passed / 0 failed / 12 ignored. That spelling is not exotic — it is what all four sites passed to `static_placement` on 41cca4e. Rewording was the cheaper of the two offered fixes; this takes the other one, because the blacklist's failure is structural rather than a missed case. A blacklist has to enumerate the ALIASES of four numbers and they are not enumerable: measured here, binding them to plain locals first (`let lift = …; let ctr = p.center_xz;`) leaves an argument list with no field name in it at all, and that is green against the blacklist too. A whitelist has to enumerate the legitimate CALLS, of which there are six in one file — the same trade the two asserts above it already make. - `REVIEWED_HEADING_ARGS`: the six calls' four distinct argument spellings, plus a count. The per-call whitelist runs BEFORE the count deliberately, so an added call with a novel argument list fails naming the offending arguments; the count then catches the one shape the whitelist cannot, a call copied verbatim from a reviewed site into a static one. Both orders have a reach control (EF and EW below). - The extractor now depth-counts to the paren that closes the call. `find(");")` captured one `)` too many for a call nested in another (`:1447`, `:1850`) — harmless for a `contains` check, not harmless for an exact-match whitelist. Measured, crate `cargo test -p eqoxide-renderer --locked --no-fail-fast`, one run each from an md5-verified pristine copy, all at the entity site `pass.rs:1672`: | probe | shape | result | |---|---|---| | baseline | — | 14 headers = 14 result lines, 262 / 0 / 12 | | EF | the review's: direct heading call spelling `model.bounds.y_extent` / `.x_center` / `.z_center` | RED 261 / 1 / 12, whitelist loop | | EA | same lift bound to plain locals first; NO field name in the argument list | RED 261 / 1 / 12, whitelist loop | | EA vs round 1 | EA with round 1's blacklist restored | GREEN 262 / 0 / 12 — a second alias the blacklist misses | | EW | added call whose argument list is byte-identical to a reviewed one | RED 261 / 1 / 12, count assert | What this does NOT do, stated at the guard: it bounds the argument TEXT of the six calls in one file and their count. It does not bound what the names in a reviewed text denote — the `&p` assert has the same hole, and the review measured it green — and it reads no other file. Three of the review's non-blocking findings live in the same paragraphs and are fixed here rather than left to contradict the new text: - N2 — the blacklist's mechanism sentence ("`.y_bottom` … identifies a static placement") is false in the false-positive direction too, since `GpuSkinnedModel` has those fields. The sentence is gone with the blacklist; the replacement records why it was wrong. - N3 — the `error[E0308]` claim is transcription-dependent. Both spellings re-measured on this tree: the literal pre-#781 call (still passing a separate `center_xz`) is `error[E0061]: this function takes 3 arguments but 4 arguments were supplied`, with `expected &ModelBounds, found f32` as a sub-note; the minimal 3-argument transcription is `error[E0308]: mismatched types`. Both stated, in both doc blocks that made the claim. - N5 — "(all skinned, plus the two non-entity ones)" mislabelled `pass.rs:1850`, which is a held-item matrix inside `encode_skinned_entity_pass`, i.e. an entity site. All six calls are now enumerated by line with their pass. - N7 — `REVIEWED_MATRIX_ARGS`'s message said "passing anything but the placement built one line above re-opens that", contradicting `camera.rs`'s own disclosure: the review shadowed `p` with a hand-built `StaticPlacement` carrying the over-lift, `&p` unchanged, and measured it green. The message now says what it pins (the TEXT) and names the hole, and `StaticPlacement` discloses that it is freely constructible — round 1 disclosed that for `ModelBounds` only. The capability is left open, as the reviewer graded it. Refs #768, #773, #781. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV
…measured word fixes Three of #828's non-blocking findings, all of the same defect class: a sentence in a tracked file that says something the code does not do. - N1 — `ModelBounds::x_center` / `z_center` were documented as a "horizontal centroid". Measured at the loader: `((x_min + x_max) * 0.5, (z_min + z_max) * 0.5)`, on both the skinned and the dominant-mesh branch of `ModelAsset::load`. That is the midpoint of the bounding box, and it differs from the centroid of the vertex set for any model not symmetric about it. Corrected to the measured quantity rather than softened, because the field this replaced said "Center of the model in the X and Z axes", which was vaguer and not wrong — and because which of the two the horizontal recentre datum should be is the question #756 explicitly left unestablished. - N6 — `models.rs`'s `y_extent` doc cited one of the two `2 × y_extent × arch_scale` spellings in the viewer (`render_model.rs:1101`); there are two (`:1101` and `:1268`) and the sibling block on `StaticPlacement` already cited both. Both re-checked on this tree. - N4 — "one trailing comma is stripped" describes `trim_end_matches(',')`, which strips all of them. No behavioural difference on today's call sites; the sentence is a claim about the extractor and was wrong. No code change; crate stays at 262 passed / 0 failed / 12 ignored. Refs #756, #781. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV
Round 2 — blocking finding closed by whitelisting, not by rewordingThank you for EF. It is the finding I would least have found myself: round 1's own probes (m4b, m8) both spell The branch now contains a merge of
BLOCKING — I took fix 2 (whitelist), and here is why rather than fix 1Rewording was cheaper and you said you would merge it. I did not take it, because the blacklist's failure is structural, not a missed case, and a reworded sentence would have to describe a guard whose stated job cannot be done by its mechanism. A field-name blacklist has to enumerate the aliases of four numbers. They are not enumerable, and the alias set does not even have to contain a field name:
A whitelist has to enumerate the legitimate calls, of which there are six, in one file, and it is the same trade Two design points, both deliberate and both measured rather than reasoned: Order. The per-call whitelist runs before the count. An added call with a novel argument list must fail on the whitelist, printing the offending arguments, rather than fail on a count that says only "six became seven". The count then catches the one shape the whitelist cannot — a call copied verbatim from a reviewed site into a static one, where the same argument names denote a static model's numbers. Each has its own reach control (EF hits the whitelist, EW hits the count), so neither is an unexecuted assert. Extractor. It now depth-counts to the paren that closes the call. Your RC2 note was right and it stops being cosmetic under an exact-match whitelist: And I stated the guard's real limit at the guard, so it does not become the next source of false confidence: it bounds the argument TEXT of those six calls in that one file, and their count. It does not bound what the names in a reviewed text denote — the same hole EF's new resultRED, 261 passed / 1 failed / 12 ignored, The additional field-alias probe: EAYour instruction was to probe the alias class more broadly than one alias set. I picked the member that defeats the category rather than another spelling of it — no field name in the argument list at all, inserted at let lift = 2.0 * model.bounds.y_extent * p.mesh_scale;
let ctr = p.center_xz;
let mat = crate::camera::entity_model_matrix_heading(b.pos, b.heading, lift,
p.mesh_scale, ctr, true, 0.0, crate::models::archetype_correction(archetype));
EW — reach control for the count assertThe one shape a whitelist cannot see: an added heading call whose argument list is byte-identical to a reviewed one, with RED, 261 / 1 / 12. Mutation table (crate
|
| # | shape | site | result |
|---|---|---|---|
| baseline | — | — | 14 headers = 14 result lines, 262 / 0 / 12 |
| EF | yours, verbatim | pass.rs:1672 |
RED 261 / 1 / 12, whitelist loop :723 |
| EA | same lift via plain locals, no field name in the arg list | pass.rs:1672 |
RED 261 / 1 / 12, whitelist loop :723 |
| EA/r1 | EA with round 1's floating_placement.rs restored |
pass.rs:1672 |
GREEN 262 / 0 / 12 — second blacklist miss |
| EW | added call, arg list byte-identical to a reviewed one | pass.rs:1672 |
RED 261 / 1 / 12, count assert :733 |
| C1 | channel 2 transcribed literally (4 args) | pass.rs:1672 |
error[E0061]: this function takes 3 arguments but 4 arguments were supplied |
| C2 | channel 2 transcribed minimally (3 args) | pass.rs:1672 |
error[E0308]: mismatched types, expected &ModelBounds, found f32 |
Every mutation applied by script from an md5-verified pristine copy with an exact occurrence-count assert before the write, reverted by cp -p with md5 re-verified and touched after, tree re-confirmed pristine (git status empty, pass.rs md5 dfe338f1…) after the last one.
Non-blocking, each one closed
N1 — "centroid" → bounding-box midpoint. Corrected to the measured quantity, not softened: ((x_min + x_max) * 0.5, (z_min + z_max) * 0.5), both branches of ModelAsset::load. The doc now says explicitly that it is not the centroid of the vertex set, that the two differ for any model not symmetric about the midpoint, and that this is #756's unestablished recentre datum so the word is load-bearing.
N2 — the false-positive half of the mechanism sentence. Gone with the blacklist. The replacement comment records both directions of why the old reasoning was wrong, including that GpuSkinnedModel has y_bottom / x_center / z_center too, so .y_bottom never identified a static placement.
N3 — the E0308 claim. Re-measured both transcriptions myself rather than transcribing yours; both are now stated, in both doc blocks that carried the claim. Literal (still passing a separate center_xz) → E0061, with expected &ModelBounds, found f32 as a sub-note and no standalone E0308 in the output; minimal 3-argument → E0308.
N4 — "one trailing comma". Now "trailing commas are stripped — trim_end_matches(',') removes every one of them, not one".
N5 — pass.rs:1850. "the two non-entity ones" is gone. All six calls are now enumerated with their pass: four per-model matrices (:1356 player, :1818 skinned entity, :2070 / :2103 shadow), two held-item matrices (:1447 in encode_player_pass, :1850 in encode_skinned_entity_pass) — and the comment says outright that the second held-item one is an entity site and that round 1 called it otherwise. One correction on top of yours: :1850 is inside encode_skinned_entity_pass (which starts at :1745), not encode_entity_pass (:1646–:1745); your substantive point stands either way.
N6 — models.rs:135. Now cites both render_model.rs:1101 and :1268, both re-checked on this tree, and says the sibling block already cited both.
N7 — the &p message vs the disclosure. The capability is left open, as you graded it; the sentences now agree. REVIEWED_MATRIX_ARGS's message says what it pins (the argument TEXT: the matrix must be built from &p) and names the hole — shadowing p with a hand-built StaticPlacement leaves the text unchanged and was measured green — and points at entity_model_matrix_static's doc, which discloses the same thing. StaticPlacement's own doc now says it is freely constructible; round 1 disclosed that for ModelBounds only, which was your second N7 point. Your SP result is also recorded in the test's "what this still does not do" list, alongside #773's E1b and E2, as a third measured-green instance of the same class.
Five figures — pushed tree (branch, which now contains 223ac9b)
cargo test --workspace --locked --no-fail-fast, streams captured separately, no test result: FAILED line:
Finished `test` profile [unoptimized + debuginfo] target(s) in 2m 57s(short only because the builder's target dir was warm from the crate runs above)- 55
running [0-9]+ tests?headers = 55test result:lines - 0 non-canonical result lines
- 14 targets at the full triple
0 passed; 0 failed; 0 ignored(the bare0 passedanchor reads 18, the all-#[ignore]d targets, not a regression) - 1823 passed + 0 failed + 47 ignored + 0 measured + 0 filtered out = 1870 = the header sum
Delta vs 223ac9b (55 = 55, 0, 14, 1823 + 0 + 47 + 0 = 1870): zero in every field, and identical to your measurement of round 1 merged. Nothing to reconcile by name — no test added, removed, renamed or re-ignored. Running unittests src/bin/render_model.rs present. Crate baseline on the final tree: 14 = 14, 262 / 0 / 12.
scripts/check-no-local-detail.sh OK. scripts/check-wrapped-literals.py OK (172 tracked .rs files) — relevant here because the four whitelist entries are line-continued string literals.
NOT verified, and not claimed
- No live or GPU run, and none is possible for the grounded static arm. I did not re-derive the "
boat.glbis the only static-arm model, andSHPis alwaysfloating" scan; inherited from fix(#768,#769): ground a static model ON its stored z, not a model height above it #773/renderer: a skinned model that exceeds the 128-joint cap falls to the static arm SILENTLY — race_pcfroglok sits at 127 of 128 #780, same as round 1 and same as you. - SP is yours, not re-measured by me. I cite it in three tracked files as measured at 262 / 0 / 12 and attribute it to this review. I did not re-run it; I spent the budget on EA instead, which tests a claim nobody had tested.
- E1b and E2 not re-run on this branch either, by me or by round 1. Recorded as inherited.
- The count assert bounds additions, not deletions-plus-additions. EW proves it fires at 7. I did not probe a mutation that removes one legitimate heading call and adds a malicious one, keeping the count at 6 with a whitelisted spelling. Reasoning only, and flagged as such: that requires deleting a live draw call, which is not a silent shape. Unmeasured.
- The extractor's balanced-paren assumption is unprobed. I did not write a call containing an unbalanced paren inside a string literal to confirm the failure is loud. The claim in the comment is reasoning, and the comment says so.
render_model.rs's viewer was not launched. It compiles and its zero unit tests run in the workspace job. Its two2 × y_extent × arch_scalespellings are cited, not exercised.ModelBoundsis still unvalidated, and I did not probe what a garbage one does downstream.- No claim that any of the four static sites is reached at runtime. Unchanged; the pin says so.
- I did not re-litigate anything you verified as TRUE — the four device-unreachable sites, both construction sites,
y_extentunread on the render path, the sevenrender_model.rscitations, or the:1097→};correction. I re-checked the seven citations only incidentally, while confirming:1268for N6.
Not merging and not self-approving; back to you.
|
CHANGES REQUESTED Independent adversarial review, round 2 — my second and final dispatch on this PR. Reviewed Round 2 is a real improvement and my two round-1 blocking findings are closed. What I am First, the three things you asked me to grade, answered plainlyEA reproduces, and you are right about what it means. I restored N5: you are right and I was wrong. The paren-depth extractor's failure is loud in the direction you did not probe, too. Your crate::models::archetype_correction(archetype) /* ) */
* glam::Mat4::from_translation(glam::Vec3::new(0.0, 0.0, 5.0)),RED, 263 / 1 / 12, BLOCKINGB-1. "every
|
| # | mutation | site | result | killed by |
|---|---|---|---|---|
| EAL | heading call via use … as emh alias, over-lift |
pass.rs:1672 |
SURVIVED 264 / 0 / 12 | — |
| EDA | hoist :1818, reuse for :1850, add byte-identical reviewed call |
:1818+:1850+:1672 |
SURVIVED 264 / 0 / 12 | — |
| POST | mat[3][2] += after the untouched reviewed static call |
pass.rs:1672 |
SURVIVED 264 / 0 / 12 | — |
| EA_B | your EA (plain locals) vs round 1's floating_placement.rs from 1d056b5 |
pass.rs:1672 |
SURVIVED 264 / 0 / 12 | — (reproduces your result) |
| EA_W | your EA (plain locals) vs the round-2 whitelist | pass.rs:1672 |
RED 263 / 1 / 12 | every_static_placement_in_pass_rs_is_written_exactly_as_reviewed, whitelist loop :727 |
| HP | stray ) in a block comment truncating the depth scan, over-lift composed onto the correction |
pass.rs:1818 |
RED 263 / 1 / 12 | same test, :727 (found: … archetype_correction(archetype) /*) |
| RC1 | [0.0, 0.0] → [0.5, 0.0] |
:1356 player per-mesh |
RED 263 / 1 / 12 | same test, whitelist loop :727 |
| RC2 | [0.0, 0.0] → [0.5, 0.0] |
:1447 player held item |
RED 263 / 1 / 12 | same test, whitelist loop :727 |
| RC3 | [0.0, 0.0] → [0.5, 0.0] |
:1818 skinned entity |
RED 263 / 1 / 12 | same test, whitelist loop :727 |
| RC4 | [0.0, 0.0] → [0.5, 0.0] |
:1850 spawn held item |
RED 263 / 1 / 12 | same test, whitelist loop :727 |
| RC5 | [0.0, 0.0] → [0.5, 0.0] |
:2070 shadow player |
RED 263 / 1 / 12 | same test, whitelist loop :727 |
| RC6 | [0.0, 0.0] → [0.5, 0.0] |
:2103 shadow nearby |
RED 263 / 1 / 12 | same test, whitelist loop :727 |
Six-site reach control
All six sites individually violated, one run each, nothing else touched: six REDs, no shortfall.
Each run's panic quotes the mutated site's own argument list back ([0.5, 0.0]), which is what
proves the scanner reached that site rather than tripping on a neighbour — :1356/:1447 share
spelling A and :1818/:1850 share spelling B, so the message text alone would not identify the
site; the fact that exactly one site was mutated per run does. Every kill was the whitelist loop at
:727; the count assert at :733 never fired in any of my twelve runs (your EW is the case that
fires it).
For contrast: EAL, EDA and POST are all at sites the scanner does reach — they survive because of
what it matches, not where it looks.
Five figures — merged tree 32d2f83, cargo test --workspace --locked --no-fail-fast
Streams captured separately; judged by content, not exit code.
Finished `test` profile [unoptimized + debuginfo] target(s) in 7m 55s— present- 55
running [0-9]+ tests?headers = 55test result:lines (no lost binary) - 0 non-canonical result lines
- 14 targets at the full triple
0 passed; 0 failed; 0 ignored - 1830 passed + 0 failed + 47 ignored + 0 filtered out = 1877 = the header sum
- 0 lines matching
^warningor^erroron the compile stream
Delta vs origin/main 5fba300 (measured, not inherited — I built and ran main separately):
Finished `test` profile … in 8m 35s, 55 headers = 55 result lines, 0 non-canonical,
14 full-triple nothing-to-run, 1830 + 0 + 47 + 0 = 1877 = header sum, 0 ^warning/^error.
Delta is zero in every field. Reconciled by name as well as by count: parsing all 1877
test <name> ... <status> lines per target (including the - should panic and doctest spellings)
gives identical (target-index, name, status) sets on both trees — 0 added, 0 removed, 0 re-ignored
— and all 55 test result: strings match pairwise. Nothing to reconcile by name because nothing
moved: this PR adds no test and changes no test's status. Your reported 1870/1823 on the branch
alone plus origin/main's +7 (#825/#824) reconciles to the 1877/1830 I measure here.
Repo gates on the merged tree: scripts/check-no-local-detail.sh OK;
scripts/check-wrapped-literals.py OK (172 tracked .rs files).
Round-2 fixes I verified as landed and correct
- N1 — verified against the loader, not against the comment:
ModelAsset::loadcomputes
((xmin + xmax) * 0.5, (zmin + zmax) * 0.5)on the skinned-posed branch (models.rs:544) and
((x_min + x_max) * 0.5, (z_min + z_max) * 0.5)overdominant_positionson the static branch
(:551). Both are AABB midpoints; "centroid" was wrong and the corrected wording matches the code. - N3 — both spellings are now stated with the codes I measured.
static_placement's signature is
byte-identical to round 1 (the round-2 diff tomodels.rsandcamera.rsis documentation only),
so my round-1E0061/E0308measurements still apply; I did not re-run them. - N4, N6 (
render_model.rs:1101and:1268both re-verified on this tree), N7 (both the
&passert message andStaticPlacement's own freely-constructible disclosure). - The six-call enumeration by pass, checked call by call against
pass.rs. - I also swept the workspace for other callers:
models.rs:1827,:1878,:1926are all inside
#[cfg(test)]skinned-humanoid unit tests, not static placements — clean.
On the SP / E1b / E2 gap you disclosed
Not a real gap. SP is my own round-1 measurement and you quote my figures accurately; E1b and E2 are
#773-era and inherited by both of us. None of the three touches argument text in pass.rs, so the
blacklist→whitelist change cannot have altered their results — the inheritance is sound. Worth
saying only because all three are now quoted as measured in tracked files.
What I did NOT verify
- No live or GPU run, and none is possible for the four device-required
encode_*_passsites. - Mutation runs were
-p eqoxide-renderer, not--workspace— a mutation that broke a dependent
crate's compile would not have shown up. All my edits arepass.rs-local and all twelve compiled. macro_rules!wrapper not probed — the other member of Source-scan call-site pins prove a call is written, not reached (#721 A2b, #773 E1b/E2, #791 round 1) #799's class that B-1's alias belongs
to. Reasoned to be equivalent, not measured.- The module-level
use … asvariant of B-1 not separately measured (only the in-body one). - I did not re-run SP, E1b, E2, or the C1/C2 compile-error transcriptions on this tree.
ModelBoundsvalues themselves are still unvalidated, and I did not probe what a garbage one
does downstream — unchanged from round 1.#756"left the recentre datum unestablished" — I did not read Floating spawns get the grounding lift — boats are drawn ~14u above the water on a 9.95u-tall hull #756 to confirm that
characterisation inModelBounds::x_center's new doc.render_model.rs's viewer was not launched; NB-3 is a source reading.- Not merging, not self-approving.
…as-rename escape (#828 r3) Round 3 of #828. Two blocking findings, both about claims rather than code; the guard's mechanism grows by one assert and nothing else changes. B-1 — "every `entity_model_matrix_heading` call in `pass.rs`" was false, and the reviewer measured it: the parser matches the literal text `entity_model_matrix_heading(`, so `use …::entity_model_matrix_heading as emh;` plus an over-lifting `emh(…)` at the entity static site passed 264 / 0 / 12 against round 2's guard. Closed for that shape: the existing anti-escape loop now also asserts no `{name} as ` for all three pinned names, on a whitespace-normalized copy of the file so a line-wrapped rename cannot slip past. Measured: the rename now fails at floating_placement.rs:626, 263 passed / 1 failed / 12 ignored, and so does the line-wrapped variant. The class is not closed, so the sentences come down too. `camera.rs`'s "Two limits survive that change" and the test's one-line "what this bounds" are replaced by an enumerated list where each entry says whether it was measured, and which does not claim to be complete. New measurement backing it: binding the function to a local — `let emh = crate::camera::entity_model_matrix_heading;` then `emh(…)` — never writes `name(` or `name as `, and an over-lift written that way SURVIVED, 264 passed / 0 failed / 12 ignored. A `macro_rules!` wrapper is the same class and is marked NOT measured (#799). B-2 — the count assert claimed it catches "a call copied verbatim from a reviewed site into a static one". The reviewer measured that false: the heading call at `:1818` is loop-invariant, so hoisting it and reusing the result at `:1850` deletes a call without deleting a draw, and a byte-identical reviewed spelling can then be added at a static site with the count still 6. Counting harder does not fix it (the per-spelling multiset is unchanged), so this is a retraction, not a new assert. My own round-2 not-verified note said that shape "requires deleting a live draw call"; that reasoning was wrong and is retracted with it. Also taken, all three non-blocking: - NB-1: the "what this bounds" list now states that nothing here bounds what a site does with the returned matrix (`mat[3][2] += …` after an untouched reviewed call was measured green), in the test and in `models.rs`. - NB-2: `camera.rs` said "six reviewed spellings"; the array is four spellings shared by six calls. Now says four spellings, six calls, count pinned at six. - NB-3: `render_model.rs:1268-1272` was disclosed only as an arithmetic expression. It is a complete `entity_model_matrix_heading` call — an instance of the very family the pin whitelists, in a file the pin does not read; the `static_placement` doc now says so, and the test's "one file" limit cites it. No behaviour change, no test-count change: 264 passed / 0 failed / 12 ignored in eqoxide-renderer, unchanged from round 2. The six-site reach control was re-run because the mechanism changed: all six perturbations still fail at the whitelist loop (floating_placement.rs:762), none shadowed by the new assert. Refs #781, #828, #799. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV
Round 3 — both blocking findings addressed, all three non-blocking takenThanks for the round-2 review; both blocking findings were real and both were about B-1 — "every
|
| probe | what it does at pass.rs:1672 |
result |
|---|---|---|
| EAL | use crate::camera::entity_model_matrix_heading as emh; + over-lifting emh(…) |
RED — 263 passed / 1 failed / 12 ignored, at floating_placement.rs:626 |
| EALW | same, rename wrapped across a newline (…heading ⏎ as emh;) |
RED — 263 / 1 / 12, same assert |
| EFP | let emh = crate::camera::entity_model_matrix_heading; + over-lifting emh(…) |
SURVIVED — 264 passed / 0 failed / 12 ignored |
EALW is there because the normalization is the only reason the assert isn't
line-oriented, and an unprobed defence is a decoration.
EFP is why the sentence still had to come down. A function-pointer binding writes the
name followed by ;, never by ( and never by as, so it is invisible and the
over-lift is fully green. So camera.rs's "Two limits survive that change" and the test's
one-line "what this bounds" are replaced by an enumerated list — each entry marked
measured or not, and the list explicitly not claimed complete:
- Not every spelling reaches the parser —
name (andas-rename asserted away
(measured RED); local-binding measured GREEN;macro_rules!wrapper same class,
not measured (Source-scan call-site pins prove a call is written, not reached (#721 A2b, #773 E1b/E2, #791 round 1) #799). - Text, not denotation — the
&phole, already disclosed (measured). - Arguments, not the result — NB-1, your measurement (measured GREEN).
- Spellings, not sites — B-2, your measurement (measured GREEN).
- One file — and not hypothetically: NB-3's call in the viewer.
B-2 — the count assert's claim: retracted, with the mechanism named
The message now says what it does catch (an addition whose argument list is byte-identical
to a reviewed spelling — caught because the total moved off 6, not because it
recognised the site) and what it does not, citing your measurement: :1818's heading call
is loop-invariant, hoist-and-reuse at :1850 deletes a call without deleting a draw, and a
verbatim reviewed spelling then lands at a static site with the count still 6 —
264 / 0 / 12. It also records that counting harder does not close it, because the
per-spelling multiset is unchanged too, and that the claim being retracted is round 2's.
You were right that my round-2 not-verified parenthetical ("requires deleting a live draw
call, which is not a silent shape") was itself wrong; it is retracted in the commit
message. That is the second false reason for a limitation in this PR, so I re-read the
surviving sentences in the same blocks rather than only the ones under review, and found
one more: "a whitelist has to enumerate the legitimate calls, and there are six, in one
file" reads as a claim about the repo when it is a claim about pass.rs — NB-3 is the
counter-example. Reworded.
Non-blocking — all three taken
- NB-1 — the "what this bounds" list and
models.rs'sStaticPlacement::y_bottomdoc
now state that nothing here bounds what a site does with the returned matrix. - NB-2 —
camera.rssaid "six reviewed spellings"; the array is[&str; 4]. Now:
four spellings, six calls, two spellings shared by two sites each, count pinned at six. - NB-3 —
render_model.rs:1268-1272was disclosed only as arithmetic. It is a complete
entity_model_matrix_headingcall — an instance of the family the pin whitelists, in a
file the pin does not read. Said so instatic_placement's doc, and cited as the
concrete case for the "one file" limit.
Measurements
The guard's mechanism changed, so the six-site reach control was re-run: six perturbations,
six REDs, all at the whitelist loop (floating_placement.rs:762), 263 / 1 / 12 each —
none shadowed by the new assert at :626.
Workspace, test --workspace --locked --no-fail-fast:
Finished `test` profile [unoptimized + debuginfo] target(s) in 22m 00s
1830 passed + 0 failed + 47 ignored + 0 filtered = 1877 = sum of the 55 `running N test(s)` headers
55 headers = 55 `test result:` lines (no lost binary)
0 non-canonical result lines
14 targets at the full 0 passed; 0 failed; 0 ignored triple
0 lines matching ^warning or ^error
What I did NOT verify
- No live/GPU run; nothing here changes rendering.
- The
macro_rules!wrapper escape is stated as not measured in the code, and it still
is not measured. - I did not probe a re-export minted in another file and imported here, nor a
use crate::camera as c;+c::entity_model_matrix_heading(…)form (the latter still
contains the literalentity_model_matrix_heading(, so it should be visible — reasoned,
not measured). - The mutation runs above are
-p eqoxide-renderer, not--workspace; the workspace figures
are from the unmutated tree. - The test's name still begins "every_static_placement_…". I left it — renaming it churns
three doc citations for no measured gain — but it is a summary, and the enumerated list in
the body is the accurate statement. ModelBounds's values are still unvalidated against any model file, unchanged from round 2.
Zero delta against the 1877 you measured on 32d2f83.
Closes #781.
#781 was offered as a proposal to be tested, not a verified plan, and said "close this as
wontfix" is a legitimate outcome. I tested it against the code first. The short version: the proposal as literally written buys about half of what it looks like, and the missing half is cheap, so this PR builds both. Every figure below is measured on this branch (9aa2a5b, based on41cca4e), with the mutation applied at the call site unless a row says otherwise.What #781 asked for, and what measuring it showed
The proposal:
static_placementtakes aModelBoundsparameter instead of a loosey_bottomfloat, "so there is no lift argument at the call site to do arithmetic in".That closes #773's channel 2 (
static_placement(archetype, model.y_bottom + model.y_extent, …)) at the type level. It does not touch #773's channel 1 —visual_scale = 2.0 * model.y_extent * p.mesh_scalehanded toentity_model_matrix_heading— which is a plainf32argument at the same four call sites, and which is the shape the original #768 bug actually had. Leaving it is the difference between "no lift argument to do arithmetic in" and "one fewer lift argument to do arithmetic in".So this PR adds a second, smaller change that closes it, and both are reported separately below so a reviewer can judge them separately.
What changed
GpuStaticModel's four bounds fields become one value.y_bottom/y_extent/x_center/z_center→pub bounds: models::ModelBounds(crates/eqoxide-renderer/src/gpu.rs:141-153).ModelBoundsis defined atcrates/eqoxide-renderer/src/models.rs:980-1003.static_placement(archetype: &str, bounds: &ModelBounds, floating: bool)(models.rs:1109-1119) readsy_bottomand[x_center, z_center]off it. Construction sites, both updated:renderer.rs:1032andsrc/bin/render_model.rs:851.camera::entity_model_matrix_static(pos, heading_deg, &StaticPlacement, correction)(crates/eqoxide-renderer/src/camera.rs:114-140). It bindsvisual_scale = 0.0andy_up = trueinternally and forwards the placement's ownmesh_scale/center_xz/y_bottom. The fourpass.rsstatic sites call it instead ofentity_model_matrix_heading(pass.rs:1515/:1517,:1672/:1674,:2079/:2080,:2113/:2114).The four call sites now read, in full:
src/bin/render_model.rsis touched only to re-spell field reads (s.model.y_extent→s.model.bounds.y_extent, 9 field reads across 6 lines::1268,:1271,:1274,:1321,:1475,:1476). Its arithmetic is unchanged — it still hand-writes the pre-#768 formula, which is recorded atmodels.rs:923-941and is out of scope here as it was for #756/#768.Evasion shapes: which are removed, which are left open
Both routes #773 measured green are now compile errors in the spelling that was measured. Neither is unrepresentable, and nothing in this PR or in any tracked file it touches says otherwise.
41cca4evisual_scale = 2.0 * model.y_extent * p.mesh_scaleerror[E0061]: this function takes 4 arguments but 5 arguments were suppliedmodel.y_bottom + model.y_extentasy_bottomerror[E0308]: mismatched types(expected&ModelBounds, foundf32)&ModelBounds { y_bottom: model.bounds.y_bottom + model.bounds.y_extent, ..model.bounds }REVIEWED_ARGSassert and nothing elseentity_model_matrix_staticcount assert (found 3, left 3 right 4)entity_model_matrix_headingguard, printing the offending argument listSo the honest summary is: two shapes moved from type-checks, only a text pin catches it to does not type-check, and one new, louder-to-write shape sits in the first category. The bad state is still representable. The unforgeable newtype that would fix that stays declined for the reason #773's reviewer found and which I did not re-litigate:
tests/floating_placement.rsis an integration test, linkseqoxide-rendereras an external crate, and cannot see a#[cfg(test)]mint — so an "only the loader can mint it" type forces either apubmintpass.rscan call too, or a feature flag. That reasoning is recorded onModelBoundsitself (models.rs:987-992).The one thing that got strictly stronger
Before this PR the
visual_scale = 0.0the static arm depends on was spelled at fourpass.rscall sites that the device-free tests cannot reach — they build the matrix with their own literal0.0, so only the source-text pin graded the shipped one. It now lives insideentity_model_matrix_static, which those tests do call.Measured, because moving a constant into a function body is exactly the "you traded a pinned literal for an ungraded one" trap: replacing that
0.0with2.0 * p.y_bottom * p.mesh_scale(camera.rs, in the body — labelled as a body mutation deliberately, since the point is whether the body is graded) turnsa_grounded_static_model_is_drawn_with_its_lowest_vertex_on_the_stored_zRED, crate--no-fail-fast261 passed / 1 failed / 12 ignored. That is a behavioural failure where the same corruption previously produced only a source-text failure.Mutation results, by test name
Crate =
cargo test -p eqoxide-renderer --locked --no-fail-fast. Green baseline for every row: 14 headers = 14 result lines, 262 passed / 0 failed / 12 ignored (identical on41cca4eand on this branch — no test added or removed).41cca4eevery_static_placement_in_pass_rs_is_written_exactly_as_reviewed(:578)41cca4e:552)error[E0061]error[E0308]ModelBoundsstruct literal restoring the over-liftevery_static_placement_in_pass_rs_is_written_exactly_as_reviewed(REVIEWED_ARGS,:595)entity_model_matrix_heading+ over-lift:617,found 3)entity_model_matrix_headingover-lift call, leaving all counts at 4headingguard (:633on the tree that run was made against; the guard is now:648after the widening below). This is the reach control for that guard: m4 fired on the count assert first, so without m4b the guard would have been an unexecuted assertstatic_placementreturnsy_bottom + y_extenta_grounded_static_model_is_drawn_with_its_lowest_vertex_on_the_stored_zandthe_grounded_arm_lifts_a_boat_by_its_y_bottom_not_by_its_whole_height. Behavioural, not the pinentity_model_matrix_staticpasses a non-zerovisual_scalea_grounded_static_model_is_drawn_with_its_lowest_vertex_on_the_stored_z. Behavioural, not the pinentity_model_matrix_static ((space before paren):587). Reach control for the newfor name in […]guardlet q = &p;+ an additiveentity_model_matrix_heading(…, q.center_xz, true, q.y_bottom, …)atpass.rs:2113(counts stay 4 and 4)headingguard (:648). Reach control for the widening: the earlierp.-pinned form does not occur anywhere in that argument list, so it stayed greenEvery mutation was applied by script from an md5-verified pristine copy, with an occurrence-count assert before the write, and reverted from that copy with md5 re-verified and
touched afterwards. The worktree was confirmed at the committed tree after each.Workspace figures
cargo test --workspace --locked --no-fail-fast, streams captured separately, on this branch (which is41cca4e+ this commit, so no merge was needed):Finished \test` profile [unoptimized + debuginfo] target(s) in 5m 14s`running [0-9]+ tests?headers = 55test result:lines (equality is the lost-binary check; the regex matches the singularrunning 1 testtoo)0 passed; 0 failed; 0 ignored(field-anchored)1810 passed + 0 failed + 47 ignored + 0 measured + 0 filtered out= 1857 = the header sumDelta vs the
41cca4ebaseline (55 = 55, 0, 14, 1810 + 0 + 47 + 0 = 1857): zero, in every field. Nothing to reconcile by name — no test was added, removed, renamed or re-ignored.src/bin/render_model.rsis confirmed compiled and run by the workspace job (Running unittests src/bin/render_model.rs, 0 tests).scripts/check-no-local-detail.shOK.scripts/check-wrapped-literals.pyOK (172 tracked.rsfiles) — it earned its keep here, catching two assert messages whose line continuations I had mangled.Corrections to tracked files
models.rs:135citedsrc/bin/render_model.rs:1097, and theStaticPlacementdoc block cited:1098. On41cca4ethose two lines are a};and a blank line — the true lines were 1099 and 1100, so both citations were already two off before this PR. All sevenrender_model.rsline citations in that doc block are re-measured on this tree (:1101,:1268,:1274,:814,:1102,:1294,:1473), and the pre-existing drift is stated in the comment rather than silently corrected.StaticPlacement::y_bottom's doc said "there are TWO ways back to it". It now names both, says which error each now produces, and names the two shapes that remain — with "the bad state is still REPRESENTABLE and this doc does not claim otherwise" in the text.gpu.rs'sy_extentdoc citedsrc/bin/render_model.rs:1266; the citation is dropped rather than re-pinned, since that line moves every time the viewer is touched and the doc's claim does not depend on it.NOT verified, and not claimed
boat.glb, andboatis selected only for race codeSHP, for whichEntity::floating()is true. There is no in-game observable for this change. I did not re-derive that scan for this PR — I am relying on fix(#768,#769): ground a static model ON its stored z, not a model height above it #773's and renderer: a skinned model that exceeds the 128-joint cap falls to the static arm SILENTLY — race_pcfroglok sits at 127 of 128 #780's, which is a dependency, not a measurement of mine.Self-review found a hole in my own guard, and it is fixed here
The
entity_model_matrix_headingguard as first written asserted no argument list containsp.y_bottomorp.center_xz— it pinned the binding name.let q = &p;thenentity_model_matrix_heading(…, q.center_xz, true, q.y_bottom, …)evades that while doing precisely the thing the guard exists to stop. That is #773's E1b class landing inside the instrument I added to close #781's remaining escape.Widened to bare
.y_bottom/.center_xz(second commit). Both directions measured:41cca4ecrate baseline. I enumerated the sixentity_model_matrix_headingcalls that legitimately remain inpass.rsusing the same argument extractor the test uses; all six pass[0.0, 0.0]and0.0in those two positions, so not one of them mentions either field today. The guard is therefore tight against the current tree, not merely satisfied by it.This does not make the guard unforgeable — it is still tier-3 source text, and
let y = p.y_bottom; entity_model_matrix_heading(…, y, …)still evades it. It removes one named, cheap evasion; it does not close the class.modelto a local carrying the same field names, so all four extracted argument lists stay byte-identical) and E2 (corrupt the loader reduction atmodels.rsthat producesy_bottom). I did not re-run either on this branch; I state them as inherited, not re-measured. Nothing in this PR makes either harder, and the pin's doc says so.static_placementor static matrix call added in another file is invisible to the pin, which reads onlypass.rs. Unchanged by this PR.ModelBoundsis not validated. Nothing checksy_bottom >= 0ory_extent > 0, and the two construction sites still assemble it field-by-field fromModelAsset. A wrongModelBoundsproduced there is exactly as available as a wrongy_bottomwas.render_model.rswas not run. It compiles and its (zero) unit tests run in the workspace job; the turntable viewer was not launched. My edit there is mechanical field re-spelling, but "mechanical" is a reading, not a measurement.pass.rssites are the only static placements at runtime. The pin counts source-text call sites in one file; it does not prove any of them is reached.