diff --git a/src/app.rs b/src/app.rs index 78be4cb1..576613f3 100644 --- a/src/app.rs +++ b/src/app.rs @@ -2691,24 +2691,50 @@ 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 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: 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 + // 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 { 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). 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 + // regardless of `b.floating` — see above. + None => m.floor_at = [f32::NAN; 3], } } @@ -3108,9 +3134,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 +3158,70 @@ 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=-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=-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(-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 (-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=-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] + 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). + 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=-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=-3", + bbs[0].pos[2]); + } + #[test] fn first_zone_in_triggers_load() { // current_zone starts empty; arriving in a real zone must load it.