fix(#746): delete ControllerView.moving — dead field, no reader anywhere

[djhenry]

What

Deletes ControllerView.moving (crates/eqoxide-ipc/src/lib.rs) and its two write sites in
src/app.rs's render frame loop. Closes #746.

Evidence that no reader exists

The field's own doc comment (pre-existing, written for #746) already stated the conclusion, but I
re-derived it independently rather than trust the comment:

• grep -n "moving" crates/eqoxide-ipc/src/lib.rs — one declaration (pub moving: bool,), plus
  doc-comment prose mentioning "moving" in unrelated contexts (a desired_azimuth doc, an
  EMA "moving average" doc, a test-freeze doc). No reader.
• grep -rn "ControllerView" --include=*.rs . across the whole tree — every hit is either the
  type definition/impl, a doc comment, the ControllerShared alias, the one construction site
  (ControllerView::default() in src/main.rs), or src/movement.rs's re-export. No struct
  literal ever names moving:, and no destructuring pattern (ControllerView { moving, .. })
  exists anywhere.
• grep -n "\.moving" src/app.rs (field access, not the concept) — exactly two hits, both
  writes (v.moving = false; and v.moving = intent.wish_dir[...] || !self.on_ground;),
  both through the Arc<Mutex<ControllerView>> lock guard v. Neither is a read.
• grep -n "moving" src/app.rs (the broader concept, to catch coincidental matches) turns up ~30
  hits. I read every one. The overwhelming majority are a different, local let moving: bool
  that feeds select_player_action (around line 1356) and a structurally identical local in the
  remote-entity animation path (around line 2626) — these are plain locals computed from
  last_moved_at/speed, not reads of the ControllerView field, and select_player_action's own
  moving: bool parameter (line ~2482) is likewise unrelated. The rest are prose ("moving
  average", "moving the formula", "moving NPCs", etc.) with nothing to do with this field.
• grep -rn "moving" crates/eqoxide-core/src/game_state.rs crates/eqoxide-http/src/observe.rs crates/eqoxide-net/src/action_loop.rs — zero hits that touch ControllerView::moving. Nothing
  mirrors it into GameState, nothing serializes it, and ControllerView itself derives only
  Clone, Copy, Debug, Default — no Serialize, so it is not reachable via the /observe JSON
  either. (Contrast hold/afloat_stall, which are deliberately mirrored into GameState and
  published over HTTP — this field had no such path.)

The compiler oracle

Per the task's request, I ran this as an actual measurement, not just a grep. In a separate
throwaway worktree branched from the same origin/main commit, I deleted only the field
declaration (leaving both app.rs write sites untouched) and built the workspace on the remote
builder (cargo build --workspace --locked).

Result: exactly two compile errors, both error[E0609]: no field \moving` on type
`std::sync::MutexGuard<'_, ControllerView>`, pointing at src/app.rs's two v.moving = ...`
lines — the same two lines identified by grep, and nothing else in the workspace failed to
compile. This is the proof the issue asked for: if any reader existed, removing the field would
have broken it too, and none did. Full compiler output:

error[E0609]: no field `moving` on type `std::sync::MutexGuard<'_, ControllerView>`
    --> src/app.rs:1387:19
     |
1387 |                 v.moving = false;
     |                   ^^^^^^ unknown field
     |
     = note: available fields are: `pos`, `heading`, `initialized`, `landed_fall_height`

error[E0609]: no field `moving` on type `std::sync::MutexGuard<'_, ControllerView>`
    --> src/app.rs:1717:23
     |
1717 |                     v.moving = intent.wish_dir[0] != 0.0 || intent.wish_dir[1] != 0.0 || !self.on_ground;
     |                       ^^^^^^ unknown field
     |
     = note: available fields are: `pos`, `heading`, `initialized`, `landed_fall_height`

For more information about this error, try `rustc --explain E0609`.
error: could not compile `eqoxide` (lib) due to 2 previous errors

Note the available fields list itself: hold/afloat_stall (the two fields with defined
clear paths) don't even appear there — they're private to the crate, invisible from app.rs's
call site, which independently confirms they are read only through
ControllerView::publish_disclosures/disclosures, never as raw field access the way moving
was.

I then discarded that throwaway worktree (git worktree remove --force, branch deleted; it was
never committed to fix-746-moving) and made the real change in this branch: field + both
writers removed together, so this branch's own build is clean start to finish.

The semantic disagreement between the writers

The two writers computed genuinely different things, which is itself part of the finding — had
anyone read the field, they'd have gotten an inconsistent answer depending on which writer fired
last:

• src/app.rs:~1387 (camera-init / first-seed arm): v.moving = false; — always false, regardless
  of actual state, because nothing has stepped the controller yet.
• src/app.rs:~1717 (steady-state per-frame publish, gated on camera_initialized):
  v.moving = intent.wish_dir[0] != 0.0 || intent.wish_dir[1] != 0.0 || !self.on_ground; — a
  horizontal-wish-or-airborne predicate, recomputed unconditionally on every publish even on
  rendered frames that did not step the controller (no collision, mid zone-load). That
  unconditional recompute of !self.on_ground is the concrete failure mode described in the
  field's pre-existing doc comment and in PR revert(#746): un-publish player.moving — its clear is not continuous, so it lies across a zone load #792's independent review: a character that crosses a
  zone line airborne would report a confident moving: true for the whole multi-second zone load,
  because the previous zone's on_ground state survives untouched through the gap. That is the
  connected: true (connected/last_packet_age_ms are published ONLY by the render loop, which sleeps when the world dies — a dead connection reports connected:true forever #343) shape the issue warns about.

Why deletion, not a reader

Per the issue's own framing and the "Options" section, deletion was the preferred close, and I
agree with the reasoning: the second writer has no clear-on-stop (unlike hold/afloat_stall,
which are explicitly cleared via clear_hold() on the not-stepped path), so any reader wired to
it inherits a false-positive window with no defined boundary. Giving it a reader would require
first giving it a real writer with a defined clear path — which is a new feature, not a fix for
this ticket — so I deleted it instead and left a note in the neighboring afloat_stall doc
comment (which used to contrast itself against "the moving field's shape (see its doc above)")
pointing at the history instead of a doc that no longer exists.

Five-figure test results

Full workspace run on this branch (cargo test --workspace --locked --no-fail-fast via the
remote builder), stdout/stderr captured separately. Completion was judged by the launching
process actually exiting (polled kill -0 on the PID until it failed), not by the
Finished `test` profile line, which prints when the test binaries finish building — that
line does appear in this run's stderr, at line 292 of 347, followed by 55 more lines of test
execution output in stdout after it, confirming it was not mistaken for the completion signal.

• Finished `test` profile [unoptimized + debuginfo] target(s) in 11m 02s (stderr; compile
  sentinel only, reported per the task's instruction, not treated as completion proof)
• 55 running [0-9]+ tests? headers vs 55 test result: lines — equal (all in stdout;
  0 of either in stderr)
• 0 non-canonical result lines (all 55 match
  test result: (ok|FAILED)\. N passed; N failed; N ignored; N measured; N filtered out; finished in T)
• 14 all-zero targets (field-anchored: 0 passed; 0 failed; 0 ignored, not a raw 0 passed
  substring match, so a 0 passed; N ignored target — there are several — does not count)
• 1798 + 0 + 47 + 0 = 1845, matching both the sum of the 55 result lines' fields and the sum
  of the 55 running N tests header counts (1845 either way)

This is an exact match to the origin/main baseline in every figure (55=55, 0 non-canonical,
14 all-zero, 1798+0+47+0=1845) — a pure dead-storage removal, as expected: nothing added, nothing
newly exercised, nothing broken.

NOT verified, and not claimed

• No live client run. This is dead-storage removal with no live-observable surface change; the
  task brief explicitly says a live run is not required here, and I did not launch one.
• I did not add a test pinning "the field does not exist" — Rust's own compiler is the guard here
  (re-adding the field and a reader compiles fine; the property being protected is "no reader
  exists", which is what the grep + compiler-oracle steps above established, not something a unit
  test can assert about future code).
• I did not audit whether some other, unrelated dead field exists elsewhere in ControllerView
  or GameState — this PR is scoped to ipc: ControllerView.moving has no reader anywhere — a dead field that becomes a false observable the moment anyone wires it up #746's exact field.

[djhenry]

APPROVED

Independent review of #823 (closes #746). I tried hard to refute this — attacked the doc-comment-from-memory claim, the "no reader" claim, the semantic-disagreement claim, the compiler-probe premise, and the reported test figures. All five held up against source/history. No blocking or non-blocking findings.

1. The doc comment written "from memory" — checked against git history, not re-read

The new sentence on afloat_stall (crates/eqoxide-ipc/src/lib.rs) says the deleted moving field "recomputed !on_ground unconditionally on every publish, so on a rendered-but-not-stepped frame (no collision, mid zone-load) it republished the PREVIOUS zone's answer as if it were current."

I did not take this on faith. Recovered the pre-deletion doc via git log -p -- crates/eqoxide-ipc/src/lib.rs (it was added in commit 82420d5/3924e9a, the #792-round revert(#746) commits, both of which independently derived the same mechanism from a live reproduction) and diffed it against the new sentence — same claim, same mechanism, nothing added or softened.

Then I verified the mechanism against the actual pre-deletion code at 7428809:src/app.rs, not the doc comments describing it:

• self.on_ground = self.controller.on_ground; (line 1680, pre-deletion) runs unconditionally, after the if let Some(c) = self.collision.as_deref() { ...; self.controller.step(...) } else { self.controller.clear_hold(); } branch — i.e. on the else (no collision, mid zone-load) arm, step() is never called, self.controller.on_ground is never refreshed, and this line copies the stale value into self.on_ground anyway.
• The second writer, v.moving = intent.wish_dir[0] != 0.0 || intent.wish_dir[1] != 0.0 || !self.on_ground; (line 1717), runs on every rendered frame once camera_initialized, with no gate on whether step() fired that frame.

So a body that crosses a zone line airborne (on_ground == false at the moment of crossing) keeps publishing !on_ground == true for the entire zone-load window, because nothing ever re-touches self.on_ground until the new zone's collision loads and step() runs again. The doc's claim is accurate, not embellished.

2. "No reader anywhere" — tried to break it three ways, all clean

• ControllerView { ... } struct-literal / destructuring search: the only construction site in the tree is ControllerView::default() (src/main.rs:333); no ControllerView { moving, .. } pattern exists anywhere, so a .moving-only grep would not have missed a destructure.
• Derive list at crates/eqoxide-ipc/src/lib.rs:93: #[derive(Clone, Copy, Debug, Default)] — no Serialize, confirmed at source, not taken from the PR body's assertion. Not reachable via /observe JSON.
• select_player_action's moving: bool parameter (src/app.rs:2482, a local last_moved_at.elapsed() < 250ms latch) and the ~30 other "moving" hits in src/app.rs are all unrelated locals/prose — read every one, none touch ControllerView::moving.

3. Compiler-probe premise — the "did it typecheck the whole workspace" question

Can't independently re-run the author's throwaway single-field-deletion probe (it was discarded), but I ran my own full-workspace build+test from scratch on the remote builder against this exact PR head, which is strictly stronger evidence: a full cargo build --workspace (implicit in cargo test --workspace) that compiles every crate cleanly with the field AND both writers gone. Zero compile errors anywhere in the workspace. If any reader had existed anywhere, this build would have failed on it.

4. PR #821 conflict check

#821 holds src/app.rs:786-798 (its diff hunk is @@ -786,12 +786,18 @@). #823's two edits are at src/app.rs:1385/1387 and 1714/1717 (pre/post-deletion line numbers respectively) — no overlap.

5. Test figures — independently re-derived on the merged tree, not accepted from the PR body

PR head (811e147) sits directly on origin/main (7428809) already — confirmed git merge-base HEAD 7428809 == 7428809 == origin/main, so no merge was needed.

First run attempt was a false start worth flagging against myself: I backgrounded rbuild ... test --workspace with a bare & (no captured exit code) and it returned after only 1 of 55 test binaries (eqoxide lib, 211/212 tests) — almost certainly an SSH disconnect mid-run, not a code issue, but I did not judge it by the process having exited; I re-ran it capturing the exit code explicitly and got a complete run this time (exit 0, full Doc-tests tail for all 13 crates present).

Full-workspace cargo test --workspace --locked --no-fail-fast, stdout/stderr captured separately, headers matched with running [0-9]+ tests? (not plural-only — this run does contain 2 singular running 1 test headers; a plural-only regex would have under-counted 53 vs the true 55):

• 55 running N tests? headers = 55 test result: lines (0 in stderr on both counts)
• 0 non-canonical result lines
• 14 all-zero targets (field-anchored: 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out;)
• 1798 passed + 0 failed + 47 ignored + 0 filtered = 1845, matching the sum of the 55 headers' running N tests counts (also 1845)
• 0 FAILED/error[/panicked at lines anywhere in stdout or stderr
• Finished `test` profile ... in 8.83s (compile sentinel, not treated as completion — judged by exit code 0 + the final test result: line + the Doc-tests tail for all 13 crates)

This is an exact match to the baseline figures (55=55, 0 non-canonical, 14 all-zero, 1798+0+47+0=1845) — reproduced independently, not copied from the PR body.

Conclusion

Every claim I attacked — the doc comment's mechanism, the "no reader" completeness, the semantic-disagreement between the two writers, and the test figures — checked out against source and independently-run evidence rather than the PR's own prose. This is a clean, correctly-scoped dead-field deletion. No blocking or non-blocking findings.