From a22e3e56758034452774a4567ca41d4acaed06e8 Mon Sep 17 00:00:00 2001 From: dhenry Date: Sat, 1 Aug 2026 04:53:04 -0400 Subject: [PATCH 1/3] fix(#753): make the floating exemption unable to bypass floor-memo invalidation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit smooth_entity_motion's floating-entity floor-snap exemption was a pure early-exit: `if b.floating { ... } else { match collision { ... } }` skipped the whole match, including the None arm's `m.floor_at = [f32::NAN; 3]` invalidation, whenever an entity was floating. A zone reload always drives `collision` through None before the new zone's Some(new) arrives, so a reload landing while an entity was floating (levitate toggle, boat ride — Entity::floating() is re-derived from the live flymode every frame, not a one-time spawn flag, per #578) left the memo cache silently pointing at the old zone's geometry. A later grounded frame at a bit-identical position could then serve a z computed against collision that was no longer loaded. Restructured so there is a single `match collision` that always runs, regardless of `b.floating`; the floating flag now only gates whether the Some(col) arm *applies* the snap (the #194 boat behavior — keep the server-sent z), never whether the None arm's invalidation is reachable. This makes the bypass structurally impossible rather than adding a second "remember to invalidate when floating" guard next to the first one. Added a regression test driving the exact call-site sequence (grounded -> floating across a collision None/Some(new) transition -> grounded again at the same position) and confirmed it fails without the fix (serves the stale pre-reload floor) and passes with it. --- src/app.rs | 100 +++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 86 insertions(+), 14 deletions(-) diff --git a/src/app.rs b/src/app.rs index 78be4cb1..717cf638 100644 --- a/src/app.rs +++ b/src/app.rs @@ -2691,24 +2691,41 @@ fn smooth_entity_motion( // every-entity loop, and the compared position is bit-identical frame to frame // unless the entity actually moved (near: the glide has settled; far: the raw // server pos only changes on a sparse update), so only re-raycast on movement (#152). - if b.floating { - // Boats/ships float on the water surface: keep their server-sent z, do NOT snap to the - // floor. The server skips FixZ for boats too (Mob::FixZ: `if (GetIsBoat()) return;`) - // because they're GravityBehavior::Floating; floor_z would find the seabed/dock a few - // units down in shallow harbor water and yank the ship underwater (#194). - } else { - match collision { - Some(col) => { + // + // #753: the `collision` match runs UNCONDITIONALLY — including while `b.floating` — so + // the `None` arm's cache invalidation can never be bypassed by the floating exemption. + // The original shape nested this whole match inside `if !b.floating`, so a zone reload + // that happened to land while an entity was floating (levitate toggle mid-flight, a boat + // ride — `floating()` is re-derived from the LIVE flymode every frame, #578, not a + // one-time spawn classification) drove `collision` through `None` and back to `Some(new)` + // without the invalidation ever running, because `if b.floating` short-circuited past it + // entirely. A later grounded frame landing at a bit-identical `b.pos` then trusted + // `m.floor_z`, computed against the OLD zone's collision. Restructuring so there is a + // single match with no floating-gated branch around it makes that bypass structurally + // impossible rather than relying on a second place remembering to invalidate too: + // `b.floating` now only gates whether the snap is *applied* (the boat/#194 behavior — + // keep the server-sent z), never whether the cache stays honest. + match collision { + Some(col) => { + if !b.floating { if b.pos != m.floor_at { m.floor_at = b.pos; m.floor_z = col.floor_z(b.pos[0], b.pos[1], b.pos[2]); } b.pos[2] = m.floor_z; } - // No collision loaded (zone (re)loading): invalidate the cache so the snap is - // recomputed against the NEW zone geometry once it arrives, not served stale. - None => m.floor_at = [f32::NAN; 3], + // Boats/ships float on the water surface: keep their server-sent z, do NOT snap to + // the floor. The server skips FixZ for boats too (Mob::FixZ: `if (GetIsBoat()) + // return;`) because they're GravityBehavior::Floating; floor_z would find the + // seabed/dock a few units down in shallow harbor water and yank the ship underwater + // (#194). Deliberately don't write `floor_at`/`floor_z` here either: memoizing a + // floor the entity was never actually snapped against would just be a subtler + // version of the same stale-but-plausible hazard this fix closes. } + // No collision loaded (zone (re)loading): invalidate the cache so the snap is + // recomputed against the NEW zone geometry once it arrives, not served stale. Runs + // regardless of `b.floating` — see above. + None => m.floor_at = [f32::NAN; 3], } } @@ -3108,9 +3125,10 @@ mod tests { let now = std::time::Instant::now(); let surface_z = 4.0_f32; - // Floating (boat): z untouched, and still untouched on the second frame. The floating - // arm never touches the memo cache (it's comment-only), so there's nothing for a later - // frame to resurrect from — the second frame is belt-and-braces, not load-bearing. + // Floating (boat): z untouched, and still untouched on the second frame. While floating, + // the `Some(col)` arm deliberately skips writing `floor_at`/`floor_z` too (#753), so + // there's nothing for a later frame to resurrect from — the second frame is + // belt-and-braces, not load-bearing. let mut motion: HashMap = HashMap::new(); for _ in 0..2 { let mut boat = bb(9, [10.0, 0.0, surface_z]); @@ -3131,6 +3149,60 @@ mod tests { bbs[0].pos[2]); } + /// #753: a zone-geometry change that happens WHILE an entity is floating must still + /// invalidate the floor-snap memo, so a later grounded frame — even one that lands back on + /// the exact bit-identical position — re-raycasts against the CURRENT collision instead of + /// serving a z computed against geometry that is no longer loaded. + /// + /// Sequence, `b.pos` held bit-identical throughout so the ONLY things that change are + /// `b.floating` and `collision` — exactly the call-site shape the bug needs: + /// 1. Grounded on `col_a` (floor z=0): caches the snap. + /// 2. Entity starts floating (levitate toggle / boat) at the SAME instant a zone reload + /// drops `collision` to `None` — the real-world trigger (`self.collision = None` always + /// precedes a zone swap, `src/app.rs` zone-reload path). + /// 3. Still floating, the new zone's collision (`col_b`, floor z=5) arrives. + /// 4. Entity lands (floating clears) at the SAME position. A correct memo must re-raycast + /// against `col_b` and report z=5 — NOT the pre-reload col_a value of z=0. + #[test] + fn floating_across_a_zone_reload_does_not_resurrect_the_old_zones_floor() { + let col_a = flat_collision_at(0.0); + let col_b = flat_collision_at(5.0); // different height — any stale serve is detectable + let now = std::time::Instant::now(); + // z=10 sits ABOVE both floors (0 and 5) so the downward raycast can find either one; + // x/y/z is bit-identical across every frame below. + let p = [10.0, 0.0, 10.0]; + + let mut motion: HashMap = HashMap::new(); + + // 1. Grounded on the OLD zone's collision — caches floor_at=p, floor_z=0. + let mut bbs = vec![bb(9, p)]; + smooth_entity_motion(&mut motion, &mut bbs, [0.0; 3], Some(&col_a), now, 1.0 / 60.0); + assert!(bbs[0].pos[2].abs() < 1e-3, "precondition: grounded on col_a at z=0"); + + // 2. Floating starts exactly as the zone reload drops collision to None (the real + // trigger path: `self.collision = None` always precedes a zone swap). + let mut boat = bb(9, p); + boat.floating = true; + let mut bbs = vec![boat]; + smooth_entity_motion(&mut motion, &mut bbs, [0.0; 3], None, now, 1.0 / 60.0); + + // 3. Still floating, the NEW zone's collision arrives. + let mut boat = bb(9, p); + boat.floating = true; + let mut bbs = vec![boat]; + smooth_entity_motion(&mut motion, &mut bbs, [0.0; 3], Some(&col_b), now, 1.0 / 60.0); + + // 4. Lands at the SAME position. Must re-raycast against col_b (z=5), not resurrect the + // pre-reload col_a value (z=0) from a memo that was never invalidated across the change. + let mut bbs = vec![bb(9, p)]; + smooth_entity_motion(&mut motion, &mut bbs, [0.0; 3], Some(&col_b), now, 1.0 / 60.0); + assert!((bbs[0].pos[2] - 5.0).abs() < 1e-3, + "grounded frame after a floating zone-reload transition must re-raycast against the \ + CURRENT collision (col_b, z=5), got z={} — a stale memo would report the pre-reload \ + col_a value of z=0", + bbs[0].pos[2]); + } + #[test] fn first_zone_in_triggers_load() { // current_zone starts empty; arriving in a real zone must load it. From c796b9caa28d471e1b49fe3f786985dbd87290b1 Mon Sep 17 00:00:00 2001 From: dhenry Date: Sat, 1 Aug 2026 09:06:13 -0400 Subject: [PATCH 2/3] =?UTF-8?q?fix(#753):=20address=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20correct=20two=20comment=20mechanism=20claims,=20str?= =?UTF-8?q?engthen=20regression=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses PR #834 review (independent reviewer, CHANGES REQUESTED): - Rewrite the #753 comment above `match collision`: drop the unmeasured "consequence clause" (a live end-to-end resurrection was never measured), and explicitly name the two other mechanisms that also touch this cache (motion.retain's per-frame purge, begin_zone_in's entity purge) as unaddressed by this fix, rather than implying the restructure alone closes the whole hazard. - Rewrite the "deliberately don't write floor_at/floor_z" comment in the floating arm: the reviewer measured the named hazard unpinned (mutation M7 survives, whole suite green) and the fix's own unconditional None arm forecloses it anyway. State the real rationale (diff minimalism, preserve pre-#753 behavior) instead of a claim nothing here actually tests. - Test: col_a moved from flat_collision_at(0.0) to flat_collision_at(-3.0) so a stale-serve failure is distinguishable from EntityMotion's own zero-initialized floor_z (LOW finding 3). - Test: pin the bit-identity the test's discriminating power depends on with an explicit assert_eq!(motion[&9].floor_at, p, ...) after step 1, rather than relying on it silently (LOW finding 4). No production-code behavior changes in this commit — comment and test-only. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV --- src/app.rs | 63 ++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 40 insertions(+), 23 deletions(-) diff --git a/src/app.rs b/src/app.rs index 717cf638..cf1b794c 100644 --- a/src/app.rs +++ b/src/app.rs @@ -2693,18 +2693,23 @@ fn smooth_entity_motion( // server pos only changes on a sparse update), so only re-raycast on movement (#152). // // #753: the `collision` match runs UNCONDITIONALLY — including while `b.floating` — so - // the `None` arm's cache invalidation can never be bypassed by the floating exemption. - // The original shape nested this whole match inside `if !b.floating`, so a zone reload - // that happened to land while an entity was floating (levitate toggle mid-flight, a boat - // ride — `floating()` is re-derived from the LIVE flymode every frame, #578, not a - // one-time spawn classification) drove `collision` through `None` and back to `Some(new)` - // without the invalidation ever running, because `if b.floating` short-circuited past it - // entirely. A later grounded frame landing at a bit-identical `b.pos` then trusted - // `m.floor_z`, computed against the OLD zone's collision. Restructuring so there is a - // single match with no floating-gated branch around it makes that bypass structurally - // impossible rather than relying on a second place remembering to invalidate too: - // `b.floating` now only gates whether the snap is *applied* (the boat/#194 behavior — - // keep the server-sent z), never whether the cache stays honest. + // the `None` arm's cache invalidation can never be skipped by the floating exemption. The + // original shape nested the whole match inside `if !b.floating`, so while an entity was + // floating (a levitate toggle, a boat ride — `floating()` is re-derived from the LIVE + // flymode every frame, #578, not a one-time spawn flag) the `None` arm was unreachable no + // matter what `collision` did. Confirmed: `[f32::NAN; 3]` — the invalidation — appears + // nowhere else in this file, and `self.collision` has exactly two production writers + // (`Some` on load completion, `None` on reload start), so every real collision swap + // passes through `None`. `b.floating` now only gates whether the snap is *applied* (the + // boat/#194 behavior — keep the server-sent z), never whether the `None` arm is reachable. + // + // NOT measured: whether a floating entity's cache entry can actually survive a live + // `Some(A) -> None -> Some(B)` zone swap end to end — `motion.retain` (below) drops an + // absent entity's entry the first frame it's missing from the billboard list, and + // `begin_zone_in` clears `world.entities` before `self.collision` goes `None` + // (`crates/eqoxide-core/src/game_state.rs`). This restructure closes the code-shape hazard + // (a floating exemption able to bypass an invalidation) regardless of whether that live + // sequence is reachable today. match collision { Some(col) => { if !b.floating { @@ -2718,9 +2723,11 @@ fn smooth_entity_motion( // the floor. The server skips FixZ for boats too (Mob::FixZ: `if (GetIsBoat()) // return;`) because they're GravityBehavior::Floating; floor_z would find the // seabed/dock a few units down in shallow harbor water and yank the ship underwater - // (#194). Deliberately don't write `floor_at`/`floor_z` here either: memoizing a - // floor the entity was never actually snapped against would just be a subtler - // version of the same stale-but-plausible hazard this fix closes. + // (#194). Left write-free (not memoizing `floor_at`/`floor_z` here) to keep this + // fix's diff minimal and preserve pre-#753 behavior exactly — memoizing while + // floating is a plausible alternate design (mutation-checked as M7 in the #753 PR + // review: the suite stays green under it), but changing it is unrelated to this + // fix's scope. } // No collision loaded (zone (re)loading): invalidate the cache so the snap is // recomputed against the NEW zone geometry once it arrives, not served stale. Runs @@ -3156,28 +3163,38 @@ mod tests { /// /// Sequence, `b.pos` held bit-identical throughout so the ONLY things that change are /// `b.floating` and `collision` — exactly the call-site shape the bug needs: - /// 1. Grounded on `col_a` (floor z=0): caches the snap. + /// 1. Grounded on `col_a` (floor z=-3): caches the snap. /// 2. Entity starts floating (levitate toggle / boat) at the SAME instant a zone reload /// drops `collision` to `None` — the real-world trigger (`self.collision = None` always /// precedes a zone swap, `src/app.rs` zone-reload path). /// 3. Still floating, the new zone's collision (`col_b`, floor z=5) arrives. /// 4. Entity lands (floating clears) at the SAME position. A correct memo must re-raycast - /// against `col_b` and report z=5 — NOT the pre-reload col_a value of z=0. + /// against `col_b` and report z=5 — NOT the pre-reload col_a value of z=-3. + /// + /// `col_a`'s height is deliberately non-zero (review finding 3, PR #834): `EntityMotion`'s + /// own zero-init for `floor_z` is also 0.0, so a `col_a` at z=0 couldn't tell "served the + /// stale col_a raycast" apart from "served the never-initialised default" from the failure + /// value alone. -3 makes the two cases distinguishable by the number in the panic message. #[test] fn floating_across_a_zone_reload_does_not_resurrect_the_old_zones_floor() { - let col_a = flat_collision_at(0.0); + let col_a = flat_collision_at(-3.0); let col_b = flat_collision_at(5.0); // different height — any stale serve is detectable let now = std::time::Instant::now(); - // z=10 sits ABOVE both floors (0 and 5) so the downward raycast can find either one; + // z=10 sits ABOVE both floors (-3 and 5) so the downward raycast can find either one; // x/y/z is bit-identical across every frame below. let p = [10.0, 0.0, 10.0]; let mut motion: HashMap = HashMap::new(); - // 1. Grounded on the OLD zone's collision — caches floor_at=p, floor_z=0. + // 1. Grounded on the OLD zone's collision — caches floor_at=p, floor_z=-3. let mut bbs = vec![bb(9, p)]; smooth_entity_motion(&mut motion, &mut bbs, [0.0; 3], Some(&col_a), now, 1.0 / 60.0); - assert!(bbs[0].pos[2].abs() < 1e-3, "precondition: grounded on col_a at z=0"); + assert!((bbs[0].pos[2] + 3.0).abs() < 1e-3, "precondition: grounded on col_a at z=-3"); + // Pin the bit-identity the rest of this test depends on (review finding 4, PR #834): the + // memo really did cache the raycast at exactly `p`. Steps 2-4 reuse `p` unchanged, so if + // `m.display`/`m.floor_at` ever drifted by an epsilon, step 4 would re-raycast for the + // wrong reason and this test would stop pinning #753 while still passing. + assert_eq!(motion[&9].floor_at, p, "memo must key on the exact position it raycast at"); // 2. Floating starts exactly as the zone reload drops collision to None (the real // trigger path: `self.collision = None` always precedes a zone swap). @@ -3193,13 +3210,13 @@ mod tests { smooth_entity_motion(&mut motion, &mut bbs, [0.0; 3], Some(&col_b), now, 1.0 / 60.0); // 4. Lands at the SAME position. Must re-raycast against col_b (z=5), not resurrect the - // pre-reload col_a value (z=0) from a memo that was never invalidated across the change. + // pre-reload col_a value (z=-3) from a memo that was never invalidated across the change. let mut bbs = vec![bb(9, p)]; smooth_entity_motion(&mut motion, &mut bbs, [0.0; 3], Some(&col_b), now, 1.0 / 60.0); assert!((bbs[0].pos[2] - 5.0).abs() < 1e-3, "grounded frame after a floating zone-reload transition must re-raycast against the \ CURRENT collision (col_b, z=5), got z={} — a stale memo would report the pre-reload \ - col_a value of z=0", + col_a value of z=-3", bbs[0].pos[2]); } From 545922bddb9906ad9948faf6aa0ef968d43c62cf Mon Sep 17 00:00:00 2001 From: dhenry Date: Sat, 1 Aug 2026 10:00:39 -0400 Subject: [PATCH 3/3] fix(#753): correct a false "Confirmed" claim about [f32::NAN; 3] occurrences MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewer measured the sentence false: `[f32::NAN; 3]` appears at three locations in src/app.rs, not the claimed "nowhere else" — src/app.rs:2592 is the motion.entry(..).or_insert_with(..) initialiser using the same literal on entry creation, and src/app.rs:2735 is the None arm this comment describes. Not pedantry: NaN != anything, so entry re-creation is itself a second invalidation path, and it's the exact path the next paragraph (motion.retain) already leans on. Replaced with the reviewer's verbatim correction: the only OTHER occurrence is the entry initialiser (invalidates only on entry creation), and nothing else invalidates a live entry. No behavior change — comment text only. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV --- src/app.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/app.rs b/src/app.rs index cf1b794c..576613f3 100644 --- a/src/app.rs +++ b/src/app.rs @@ -2697,11 +2697,13 @@ fn smooth_entity_motion( // original shape nested the whole match inside `if !b.floating`, so while an entity was // floating (a levitate toggle, a boat ride — `floating()` is re-derived from the LIVE // flymode every frame, #578, not a one-time spawn flag) the `None` arm was unreachable no - // matter what `collision` did. Confirmed: `[f32::NAN; 3]` — the invalidation — appears - // nowhere else in this file, and `self.collision` has exactly two production writers - // (`Some` on load completion, `None` on reload start), so every real collision swap - // passes through `None`. `b.floating` now only gates whether the snap is *applied* (the - // boat/#194 behavior — keep the server-sent z), never whether the `None` arm is reachable. + // matter what `collision` did. Confirmed: the only other `[f32::NAN; 3]` in this file is + // the `motion.entry(..).or_insert_with(..)` initialiser (~2592), which invalidates only + // on entry *creation*; nothing else invalidates a live entry. And `self.collision` has + // exactly two production writers (`Some` on load completion, `None` on reload start), so + // every real collision swap passes through `None`. `b.floating` now only gates whether + // the snap is *applied* (the boat/#194 behavior — keep the server-sent z), never whether + // the `None` arm is reachable. // // NOT measured: whether a floating entity's cache entry can actually survive a live // `Some(A) -> None -> Some(B)` zone swap end to end — `motion.retain` (below) drops an